Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

volumes: Add support for volume export which allows exporting content to external path. #11290

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

flouthoc
Copy link
Collaborator

Adds support for transferring data between systems and backing up systems via exporting volume content to external specified tar.
Use cases: Recover from disasters or move data between machines.

@flouthoc
Copy link
Collaborator Author

@mheon PTAL


Allow content of volume to be exported into external tar.`
exportCommand = &cobra.Command{
Use: "export [options]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is wrong

Suggested change
Use: "export [options]",
Use: "[options] VOLUME",

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luap99 this is needed for correct formatting of podman volume --help.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, yes export has to stay my bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VOLUME part is missing, this is causing the completion test to fail.

cmd/podman/volumes/export.go Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Show resolved Hide resolved
test/e2e/volume_create_test.go Show resolved Hide resolved
utils/utils.go Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Outdated Show resolved Hide resolved
@flouthoc flouthoc force-pushed the volume-export branch 3 times, most recently from e0c6d59 to c25d2ad Compare August 20, 2021 09:59
docs/source/markdown/podman-volume-export.1.md Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Outdated Show resolved Hide resolved
cmd/podman/volumes/export.go Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 20, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021
@rhatdan
Copy link
Member

rhatdan commented Aug 20, 2021

Once this feature is created, you will need to write a blog on it.

Are you planning podman volume import?

@flouthoc
Copy link
Collaborator Author

@rhatdan yes volume import is just after this.

@flouthoc flouthoc force-pushed the volume-export branch 3 times, most recently from 23791da to 33370b2 Compare August 20, 2021 10:51
@flouthoc flouthoc requested review from Luap99 and rhatdan August 20, 2021 10:56
@flouthoc
Copy link
Collaborator Author

@Luap99 @rhatdan could you please take a look again resolved all feedback points.

flags := exportCommand.Flags()

outputFlagName := "output"
flags.StringVarP(&cliExportOpts.Output, outputFlagName, "o", "/dev/stdout", "Output=[path]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use /dev/stdout as path. This will fail on windows. Actually looking at this this will not work with podman-remote at all, right? I think not supporting remote would be fine but we should disable this command on the remote cleint and document this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, no need for remote support.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luap99 Yes we cant have this on remote-client. Do you still think we should not use /dev/stdout ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/dev/stdout is fine if we only support linux.

Look here how to disable to command on the remote client:

Annotations: map[string]string{registry.EngineMode: registry.ABIMode},

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luap99 linux or *nix. I disabled it via !registry.IsRemote().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the Annotation and not use !registry.IsRemote() This should be consistent with the other commands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Luap99 Yes annotation seems cleaner switched to annotation, PTAL.

@flouthoc flouthoc requested a review from Luap99 August 20, 2021 11:21
@flouthoc flouthoc force-pushed the volume-export branch 3 times, most recently from be0c5a6 to a11850b Compare August 20, 2021 12:03
@flouthoc flouthoc force-pushed the volume-export branch 3 times, most recently from 375b7fa to 446c0bc Compare August 20, 2021 19:57
@flouthoc flouthoc force-pushed the volume-export branch 2 times, most recently from 7c34dd1 to fde9ce7 Compare August 22, 2021 07:48
@flouthoc
Copy link
Collaborator Author

@rhatdan @mheon added a check for unmounted volumes :). PTAL

@rhatdan
Copy link
Member

rhatdan commented Aug 23, 2021

LGTM
@Luap99 @mheon PTAL

pkg/domain/infra/abi/volumes.go Outdated Show resolved Hide resolved
pkg/domain/infra/tunnel/volumes.go Outdated Show resolved Hide resolved
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mheon
Copy link
Member

mheon commented Aug 23, 2021

One last thing otherwise LGTM

Adds support for transferring data between systems and backing up systems.
Use cases: recover from disasters or move data between machines.

Signed-off-by: flouthoc <[email protected]>
@mheon
Copy link
Member

mheon commented Aug 23, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit 90cf78b into containers:main Aug 23, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants