USHIFT-4117: Auto Recovery: Restore #4061

pmtk · 2024-10-14T15:34:20Z

Adds two new options to microshift restore command:

--auto-recovery to run the procedure of restoring the most recent backup from provided directory
--save-failed to additionally make a copy of current MicroShift data to "failed" subdirectory

openshift-ci-robot · 2024-10-14T15:34:24Z

@pmtk: This pull request references USHIFT-4117 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

pmtk · 2024-10-14T15:34:27Z

/test ?

openshift-ci · 2024-10-14T15:34:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-10-14T15:35:00Z

@pmtk: The following commands are available to trigger required jobs:

/test e2e-aws-footprint-and-performance
/test e2e-aws-tests
/test e2e-aws-tests-arm
/test e2e-aws-tests-bootc
/test e2e-aws-tests-bootc-arm
/test e2e-aws-tests-bootc-periodic
/test e2e-aws-tests-bootc-periodic-arm
/test e2e-aws-tests-cache
/test e2e-aws-tests-cache-arm
/test e2e-aws-tests-periodic
/test e2e-aws-tests-periodic-arm
/test images
/test ocp-full-conformance-rhel-eus
/test ocp-full-conformance-rhel-eus-arm
/test ocp-full-conformance-serial-rhel-eus
/test ocp-full-conformance-serial-rhel-eus-arm
/test test-rpm
/test test-unit
/test verify

The following commands are available to trigger optional jobs:

/test test-rebase

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-microshift-main-e2e-aws-tests
pull-ci-openshift-microshift-main-e2e-aws-tests-arm
pull-ci-openshift-microshift-main-e2e-aws-tests-bootc
pull-ci-openshift-microshift-main-e2e-aws-tests-bootc-arm
pull-ci-openshift-microshift-main-images
pull-ci-openshift-microshift-main-test-unit
pull-ci-openshift-microshift-main-verify

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

pmtk · 2024-10-14T15:36:00Z

/test e2e-aws-tests e2e-aws-tests-bootc verify

openshift-ci-robot · 2024-10-14T15:38:38Z

@pmtk: This pull request references USHIFT-4117 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set.

In response to this:

Adds two new options to microshift restore command:

--auto-recovery to run the procedure of restoring the most recent backup from provided directory

--save-failed to additionally make a copy of current MicroShift data to "failed" subdirectory

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

ggiguash · 2024-10-15T05:05:22Z

pkg/util/atomic_copier.go

+	}
+)
+
+// AtomicCopier performs a two operation: copies source path to an intermediate location,


To be more precise, it's not any intermediate location. It's the same destination path, but with temporary name. Can we make it clearer?

ggiguash · 2024-10-15T05:07:58Z

pkg/util/atomic_copier.go

+	intermediatePath string
+}
+
+func (c *AtomicCopier) CopyToIntermediate() error {


Should we allow optional cpArgs override here?

The default copy technique and args need to be documented.

Good idea, but I don't think we need to this right now. Previous cp implementation wasn't touched for over a year until now. I'll move the file to backup related package so it doesn't need to be generic :D

ggiguash · 2024-10-15T05:11:05Z

pkg/util/atomic_copier.go

+	c.intermediatePath = fmt.Sprintf("%s.tmp", c.Destination)
+	if exists, err := PathExists(c.intermediatePath); err != nil {
+		return err
+	} else if exists {
+		if err := os.RemoveAll(c.intermediatePath); err != nil {
+			klog.Errorf("Failed to remove intermediate path %q which already existed: %v", c.intermediatePath, err)
+			return fmt.Errorf("failed to remove %q: %w", c.intermediatePath, err)
+		}
+	}


I'm not living in peace with this approach of adding .tmp suffix. It seems to be too common for the generic util code.
Can we do something like mktemp and return a fatal error if the destination exists? I'm a bit wary of deleting files while copying them.

ggiguash · 2024-10-15T05:13:34Z

pkg/util/atomic_copier.go

+		}
+		return copyErr
+	}
+	klog.InfoS("Made an intermediate copy", "cmd", cmd)


Do you mean this?

Suggested change

klog.InfoS("Made an intermediate copy", "cmd", cmd)

klog.InfoS("Completed an atomic file copy operation", "cmd", cmd)

Not really, not yet at least. The "atomic file copy operation" will be complete after rename, it's just a first part.

ggiguash · 2024-10-15T05:13:44Z

pkg/util/atomic_copier.go

+		}
+		return copyErr
+	}
+	klog.InfoS("Made an intermediate copy", "cmd", cmd)


Do you mean this?

Suggested change

klog.InfoS("Made an intermediate copy", "cmd", cmd)

klog.InfoS("Completed an atomic file copy operation", "cmd", cmd)

ggiguash · 2024-10-15T05:19:11Z

pkg/util/atomic_copier.go

+
+	// Delete the destination if it's a non-empty directory.
+	// This is a limitation of the POSIX's rename.
+	if err := removeDirIfExists(dest); err != nil {


I think this should be a fatal error. We should not delete leftover paths as a byproduct of copy operation.

This is not leftover path - this is final path. And without this mv /var/lib/microshift.tmp /var/lib/microshift doesn't work because of Rename() limitations in POSIX and UNIX.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html

If new names an existing directory, it shall be required to be an empty directory.

$ mkdir 1 2; touch 1/a 2/b $ mv 1 2 $ ls 2/ $ ls 2 1/ b

From mv man (same thing in the cp as they use the same library underneath):

-T, --no-target-directory treat DEST as a normal file

$ mkdir 1 2; touch 1/a 2/b $ mv -T 1 2 mv: cannot move '1' to '2': File exists

ggiguash · 2024-10-15T05:21:03Z

pkg/util/atomic_copier.go

Nit: what are we copying here? Is it atomic_dir_copy.go, or more generic atomic_file_copy.go?

I think it should be capable of handling both, but it's used for copying dirs, specifically data and backup dirs, so I'll rename it. Thx

ggiguash · 2024-10-15T05:23:49Z

pkg/admin/data/data_manager.go

-	tmpDest := fmt.Sprintf("%s.tmp", dest)
-	if exists, err := pathExists(tmpDest); err != nil {
+	copier := util.AtomicCopier{Source: src, Destination: dest}
+	if err := copier.CopyToIntermediate(); err != nil {


What's the reason for exposing the intermediate / final implementation of the object?
Should we just have a public Copy function and hide the implementation specifics?

Do you mean the "ToIntermediate" part of the name?

test/suites/backup/auto-recovery.robot

openshift-ci · 2024-10-17T00:19:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmtk, raz126

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pmtk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/cmd/admin.go

openshift-ci · 2024-10-18T13:01:16Z

@pmtk: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 14, 2024

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2024

pmtk force-pushed the auto-recovery/restore branch from 19024a5 to 0da4ef8 Compare October 14, 2024 15:35

pmtk changed the title ~~USHIFT-4117: Auto recovery/restore~~ USHIFT-4117: Auto Recovery: Restore Oct 14, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2024

ggiguash reviewed Oct 15, 2024

View reviewed changes

pmtk force-pushed the auto-recovery/restore branch from 0da4ef8 to e08cafb Compare October 15, 2024 11:42

pmtk marked this pull request as ready for review October 15, 2024 11:44

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 15, 2024

openshift-ci bot requested review from jerpeter1 and jogeo October 15, 2024 11:45

pmtk force-pushed the auto-recovery/restore branch from e08cafb to 4a5f7dc Compare October 15, 2024 12:05

pmtk added 8 commits October 15, 2024 16:23

Add AtomicCopier for copy&rename

d3cda91

Read Storage and get backups

1ed3a06

Auto-recovery state management

484b7e3

Extract getVersion() for later reuse

06c1f48

Get substorage path for a subdir

6c75f88

Auto-recovery restore command

dd1d4e0

Don't print rpm-ostree status output in cli

6977237

Add tests to validate restore --auto-recovery

283fad8

pmtk force-pushed the auto-recovery/restore branch from 4a5f7dc to 283fad8 Compare October 15, 2024 14:24

raz126 approved these changes Oct 17, 2024

View reviewed changes

test/suites/backup/auto-recovery.robot Show resolved Hide resolved

agullon reviewed Oct 18, 2024

View reviewed changes

pkg/cmd/admin.go Outdated Show resolved Hide resolved

Make --save-failed run by default (opt-out)

71016e9

pmtk force-pushed the auto-recovery/restore branch from 0b8430d to 71016e9 Compare October 18, 2024 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USHIFT-4117: Auto Recovery: Restore #4061

USHIFT-4117: Auto Recovery: Restore #4061

pmtk commented Oct 14, 2024 •

edited

Loading

openshift-ci-robot commented Oct 14, 2024 •

edited by openshift-ci bot

Loading

pmtk commented Oct 14, 2024

openshift-ci bot commented Oct 14, 2024

openshift-ci bot commented Oct 14, 2024

pmtk commented Oct 14, 2024

openshift-ci-robot commented Oct 14, 2024 •

edited by openshift-ci bot

Loading

ggiguash Oct 15, 2024

ggiguash Oct 15, 2024

ggiguash Oct 15, 2024

pmtk Oct 15, 2024

ggiguash Oct 15, 2024

ggiguash Oct 15, 2024

pmtk Oct 15, 2024

ggiguash Oct 15, 2024

ggiguash Oct 15, 2024

pmtk Oct 15, 2024

ggiguash Oct 15, 2024

pmtk Oct 15, 2024

ggiguash Oct 15, 2024

pmtk Oct 16, 2024

openshift-ci bot commented Oct 17, 2024

openshift-ci bot commented Oct 18, 2024

	klog.InfoS("Made an intermediate copy", "cmd", cmd)
	klog.InfoS("Completed an atomic file copy operation", "cmd", cmd)

USHIFT-4117: Auto Recovery: Restore #4061

Are you sure you want to change the base?

USHIFT-4117: Auto Recovery: Restore #4061

Conversation

pmtk commented Oct 14, 2024 • edited Loading

openshift-ci-robot commented Oct 14, 2024 • edited by openshift-ci bot Loading

pmtk commented Oct 14, 2024

openshift-ci bot commented Oct 14, 2024

openshift-ci bot commented Oct 14, 2024

pmtk commented Oct 14, 2024

openshift-ci-robot commented Oct 14, 2024 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 17, 2024

openshift-ci bot commented Oct 18, 2024

pmtk commented Oct 14, 2024 •

edited

Loading

openshift-ci-robot commented Oct 14, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 14, 2024 •

edited by openshift-ci bot

Loading