USHIFT-1083 `microshift admin data backup` command by pmtk · Pull Request #1819 · openshift/microshift

pmtk · 2023-05-18T18:15:56Z

No description provided.

dhellmann

This looks like a good start.

How can we make sure MicroShift is not running before taking the backup? Is that actually a requirement or does cp work ok in that case?

pkg/admin/backup.go

pmtk · 2023-05-18T20:38:04Z

How can we make sure MicroShift is not running before taking the backup? Is that actually a requirement or does cp work ok in that case?

That would be the thing with ps aux I had in previous PR, thanks for the reminder

pmtk · 2023-05-21T07:43:55Z

/retest

pkg/admin/run_check.go

pmtk · 2023-05-22T15:21:47Z

/assign @dhellmann @pacevedom

pmtk · 2023-05-23T11:40:09Z

/retest

pmtk · 2023-05-25T11:56:45Z

/test e2e-openshift-conformance-reduced-arm

dhellmann

The overall logic looks good. I have a couple of specific points inline, and I think it would be worth reviewing the logging advice at https://docs.openstack.org/oslo.log/latest/user/guidelines.html#overall-logging-principles and thinking about how to apply that here, both for the use of different log levels and for the "unit of work" approach described for info log messages.

pkg/admin/data/backup.go

pkg/admin/data/run_check.go

dhellmann · 2023-05-25T18:59:37Z

pkg/admin/data/manager.go

+	Restore(BackupConfig) error
+}
+
+func NewManager() *manager {


Why wouldn't the config be passed in when creating the manager?

I'm planning to use it like this (src):

a.dataManager.Backup(data.BackupConfig{ Storage: config.BackupsDir, Name: prevBootHealth.DeploymentID, })

When I'll need to restore backup for another deployment, I don't want to create another manager.
Should manager take backupsDir and Backup/Restore functions take name of the backup?

I think you're right that the backup directory could be held in the manager, and then the functions could take arguments that are more dynamic, like the name to restore from. A method like List() would also use the backup directory, but not the name value in BackupConfig.

Changed it so the manager has "storage" as field and Backup/Restore only take BackupName

dhellmann · 2023-05-26T18:12:19Z

pkg/admin/data/manager.go

+	Restore(BackupConfig) error
+}
+
+func NewManager() *manager {


I think you're right that the backup directory could be held in the manager, and then the functions could take arguments that are more dynamic, like the name to restore from. A method like List() would also use the backup directory, but not the name value in BackupConfig.

dhellmann · 2023-05-26T18:17:55Z

pkg/cmd/admin.go

+		},
+	}
+	v := version.Get()
+	data.PersistentFlags().String(


What's the use case for making the storage location configurable from the command line?

I'd say nice to have for RPM systems where backups are manual. But i don't have concrete use case other than NFS (or different partition) mount for backups, but then CoW goes out of window

OK, that makes sense. In that case I think we're going to not want to overwrite an existing backup, so we should think about how to handle that. Maybe just a check in the CLI code, before we get into the low-level code, where we do want old backups overwritten.

Added BackupExists() and --force

dhellmann · 2023-05-26T18:24:11Z

pkg/admin/data/backup.go

+	}
+
+	if err := renamePath(dest_tmp, dest); err != nil {
+		klog.Errorf("Renaming path failed - renaming %s back and deleting %s: %v", dest_old, dest_tmp, err)


dest_old will only exist if the original location existed when this function started, so if the rename on line 60 fails for some other reason then the logic in this section is going to produce more errors.

What are the situations that could result in the location already existing? We can definitely write code to produce a unique name, so I think we only have to worry about the collision if the name comes from the user via the command line? What if we just refuse to take the backup in that case? Or don't let the user specify the full backup location (give us a directory and we pick a name in that directory)?

Same code will be used in automated ostree backing up so 3rd boot into the same healthy deployment will overwrite backup taken during 2nd boot (and since backup name is 'deploymentID' the dir will already exist)

We might want to move some of that overwrite logic "up" the call stack to a place where the code knows whether it is being invoked by the user or by automation. That will make the edge cases easier to reason about.

Added function to interface BackupExists(BackupName) and it's called in the CLI. It will refuse to overwrite a backup unless --force is provided

klog includes filename in the logs, but if we add more manager.go then it'll be harder to know which one logged the message

dhellmann

/lgtm

/hold for QE verification

dhellmann · 2023-05-30T18:41:40Z

pkg/admin/data/data_manager.go

+	}
+
+	if err := renamePath(tmp, dest); err != nil {
+		klog.Errorf("Renaming path failed - renaming %s back and deleting %s: %v", old, tmp, err)


It's going to be interesting to set up a test for this error case.

Since this is so unlikely, do you think that "recovery actions" (renamePath and removePath) are unnecessary? I would suspect that if one rename fails, then other might fail as well but since go might return error I thought this is the way to handle it (try to revert)

We talked on slack about a way to generate unique names to avoid having to rename anything. Let's land this version and make that change in another PR.

pkg/admin/data/data_manager.go

pmtk · 2023-05-31T08:02:05Z

/label tide/merge-method-squash

dhellmann

/lgtm
/hold

@jogeo, please remove the hold when you've had a chance to verify the change.

dhellmann · 2023-05-31T13:48:23Z

pkg/admin/data/data_manager.go

+	}
+
+	if err := renamePath(tmp, dest); err != nil {
+		klog.Errorf("Renaming path failed - renaming %s back and deleting %s: %v", old, tmp, err)


We talked on slack about a way to generate unique names to avoid having to rename anything. Let's land this version and make that change in another PR.

dhellmann · 2023-05-31T14:01:08Z

pkg/admin/data/data_manager.go

+		}
+		klog.InfoS("Backup storage directory created", "path", dm.storage)
+	} else {
+		klog.InfoS("Backup storage directory already existed", "path", dm.storage)


This indicates an attempt to write over an existing backup, right? I would expect that to be an error.

Not really, that's the "parent directory" where backups are stored. Maybe you have how can we name it better ("backup storage") or maybe just simply delete this log (and keep only one about creating /var/lib/microshift-backups/)?

Ah, I see what's going on now.

openshift-ci · 2023-05-31T16:38:45Z

@pmtk: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-openshift-conformance-reduced-arm	`a6e672c`	link	false	`/test e2e-openshift-conformance-reduced-arm`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

dhellmann

/lgtm
/hold

Holding for @jogeo to verify again

dhellmann · 2023-05-31T18:16:48Z

pkg/admin/data/data_manager.go

+		}
+		klog.InfoS("Backup storage directory created", "path", dm.storage)
+	} else {
+		klog.InfoS("Backup storage directory already existed", "path", dm.storage)


Ah, I see what's going on now.

openshift-ci · 2023-05-31T18:21:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dhellmann,pmtk]

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

jogeo · 2023-06-01T00:03:01Z

I've preformed some early testing on this PR and it behaves as expected.
/unhold

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 May 18, 2023

openshift-ci bot requested review from dhellmann and pacevedom May 18, 2023 18:16

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2023

dhellmann reviewed May 18, 2023

View reviewed changes

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

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

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

pmtk changed the title ~~WIP Command to back up the data~~ USHIFT-1083 microshift admin backup command May 20, 2023

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 May 20, 2023

ggiguash reviewed May 22, 2023

View reviewed changes

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

openshift-ci bot assigned dhellmann and pacevedom May 22, 2023

pmtk added 10 commits May 25, 2023 08:28

microshift admin backup

1fd95f7

configurable backup dest dir

c505f6b

use config var for default value

da262e5

more logs and logic for backup

3332008

ensure microshift is not running

3fa1bd7

move backup under data subcommand

c69fe38

DataManager interface

f3cd8ca

check if svc is inactive instead of ps aux

52fe5d3

fix ireturn

59b7c7b

rename --dest and make sure args are not empty

8548161

pmtk force-pushed the upgrade-ostree-backup-new branch from 4af7f85 to 8548161 Compare May 25, 2023 06:29

rename DataManager to Manager

6fa81a2

dhellmann reviewed May 25, 2023

View reviewed changes

catch empty args early

d8a68cb

pmtk added 2 commits May 26, 2023 10:24

change backup unit of work start log

e90d88a

move microshift running check

aced3e6

dhellmann reviewed May 26, 2023

View reviewed changes

pmtk added 2 commits May 29, 2023 12:38

--force to overwrite backup via cli

25dd11b

rename manager.go for better logs

4bb597f

klog includes filename in the logs, but if we add more manager.go then it'll be harder to know which one logged the message

pmtk force-pushed the upgrade-ostree-backup-new branch from 1d04379 to 4bb597f Compare May 29, 2023 10:40

interface impl assertion and better 'Restore not impl' log

b5d84fe

pmtk changed the title ~~USHIFT-1083 microshift admin backup command~~ USHIFT-1083 microshift admin data backup command May 29, 2023

dhellmann reviewed May 30, 2023

View reviewed changes

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels May 30, 2023

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 31, 2023

fix linter

aeb0b2e

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 31, 2023

separate std out and err

5816743

dhellmann reviewed May 31, 2023

View reviewed changes

simplify by requiring non-existing backup name

a6e672c

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2023

dhellmann reviewed May 31, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2023

openshift-merge-robot merged commit 44de746 into openshift:main Jun 1, 2023

pmtk deleted the upgrade-ostree-backup-new branch June 1, 2023 06:17

microshift-rebase-script bot mentioned this pull request Jul 17, 2024

NO-ISSUE: rebase-main-4.17.0-0.nightly-2024-07-16-202442_amd64-2024-07-16_arm64-2024-07-16 #3645

Merged

This was referenced Nov 14, 2025

NO-ISSUE: rebase-release-4.21-4.21.0-0.nightly-2025-11-13-042845_amd64-2025-11-13_arm64-2025-11-13 #5743

Closed

NO-ISSUE: rebase-release-4.21-4.21.0-0.nightly-2025-11-13-042845_amd64-2025-11-13_arm64-2025-11-15 #5750

Merged

Conversation

pmtk commented May 18, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmtk commented May 18, 2023

Uh oh!

pmtk commented May 21, 2023

Uh oh!

Uh oh!

pmtk commented May 22, 2023

Uh oh!

pmtk commented May 23, 2023

Uh oh!

pmtk commented May 25, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmtk May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmtk commented May 31, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmtk May 27, 2023 •

edited

Loading