Add remote support for `podman volume import` and `podman volume export` #26434

mheon · 2025-06-14T13:24:43Z

These were coded up entirely in cmd/, which meant no API support. Refactor them into libpod/, add API endpoints, all that fun stuff!

Fixes #26409

Does this PR introduce a user-facing change?

The `podman volume import` and `podman volume export` commands are now available in the remote Podman client.

Previously, our approach was to inspect the volume, grab its mountpoint, and tar that up, all in the CLI code. There's no reason why that has to be in the CLI - if we move it into Libpod, and add a REST endpoint to stream the tar, we can enable it for the remote client as well. As a bonus, previously, we could not properly handle volumes that needed to be mounted. Now, we can mount the volume if necessary, and as such export works with more types of volumes, including volume drivers. Signed-off-by: Matt Heon <[email protected]>

packit-as-a-service · 2025-06-14T13:57:42Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

Luap99 · 2025-06-14T14:09:38Z

cmd/podman/volumes/export.go


 	outputFlagName := "output"
-	flags.StringVarP(&cliExportOpts.Output, outputFlagName, "o", "/dev/stdout", "Write to a specified file (default: stdout, which must be redirected)")
+	flags.StringVarP(&cliExportOpts.OutputPath, outputFlagName, "o", "/dev/stdout", "Write to a specified file (default: stdout, which must be redirected)")


/dev/stdout is not a working default for windows

Luap99 · 2025-06-14T14:10:50Z

cmd/podman/volumes/import.go

-	tarPath := args[1]
+	filePath := args[1]
+	if filePath == "-" {
+		filePath = "/dev/stdin"


again not a proper path for windows it would be best if you could just pass an io.Writer/io.Reader all the ways through the stack
then you can just use os.Stdin

Luap99 · 2025-06-14T14:12:30Z

libpod/volume.go

+	err := v.mount()
+	mountPoint := v.mountPoint()
+	v.lock.Unlock()
+	if err != nil {
+		return nil, err
+	}
+	defer func() {
+		v.lock.Lock()
+		defer v.lock.Unlock()
+
+		if err := v.unmount(false); err != nil {
+			logrus.Errorf("Error unmounting volume %s: %v", v.Name(), err)
+		}
+	}()


do we actually want to unlock, I realize this is what the code did before but like in what world is it sane to allow volume rm while export/import is running

We unlock for commands like podman cp so I see no reason not to here?

Mhh, I guess fair enough as we don't want things like a volume ls being blocked for a long time but when I make the listing parts not holding the lock I like to revalue.

Luap99 · 2025-06-14T14:12:43Z

libpod/volume.go

+	v.lock.Lock()
+	err := v.mount()
+	mountPoint := v.mountPoint()
+	v.lock.Unlock()
+	if err != nil {
+		return err
+	}
+	defer func() {
+		v.lock.Lock()
+		defer v.lock.Unlock()
+
+		if err := v.unmount(false); err != nil {
+			logrus.Errorf("Error unmounting volume %s: %v", v.Name(), err)
+		}
+	}()


same thing here

Luap99 · 2025-06-14T14:14:28Z

pkg/api/handlers/libpod/volumes.go

+	tmpfile, err := os.CreateTemp("", "podman-volume-import-")
+	if err != nil {
+		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("creating temporary file: %w", err))
+		return
+	}
+	defer func() {
+		tmpfile.Close()
+		os.Remove(tmpfile.Name())
+	}()
+
+	if _, err := io.Copy(tmpfile, r.Body); err != nil {
+		utils.Error(w, http.StatusInternalServerError, fmt.Errorf("copying archive to temporary file: %w", err))
+		return
+	}


why a temporary copy at all, just pipe the body to import to avoid any copies.

Other than that I see no seek(0) called on tmpfile so wouldn't it get EOF right away?

Luap99 · 2025-06-14T14:16:52Z

pkg/bindings/volumes/volumes.go

 }
+
+// Export exports a volume to the given path
+func Export(ctx context.Context, nameOrID string, options entities.VolumeExportOptions) error {


I would expect the bindings to just return the stream so the caller can write or do what they want, otherwise you force all bindings users to a temporary file if they need to inspect the stream for whatever reasons

Oh and also bindings options don't use entities structs, they should use specific bindings types

Luap99 · 2025-06-14T14:17:52Z

pkg/bindings/volumes/volumes.go

+func Import(ctx context.Context, nameOrID string, options entities.VolumeImportOptions) error {
+	if options.InputPath == "" {
+		return errors.New("must provide valid path for file to write to")
+	}
+
+	targetFile, err := os.Create(options.InputPath)
+	if err != nil {
+		return fmt.Errorf("unable to create target file path %q: %w", options.InputPath, err)
+	}
+	defer targetFile.Close()


Same here I would expect the caller to provider a io.Reader

Luap99 · 2025-06-14T14:20:55Z

utils/utils.go

 // UntarToFileSystem untars an os.file of a tarball to a destination in the filesystem
-func UntarToFileSystem(dest string, tarball *os.File, options *archive.TarOptions) error {
-	logrus.Debugf("untarring %s", tarball.Name())
+func UntarToFileSystem(dest string, tarball io.Reader, options *archive.TarOptions) error {


what do we gain from this function? the caller should just use archive.Untar directly then

Luap99 · 2025-06-17T13:02:21Z

Note "Address review comments" is general not a useful commit title for me, I suggest that gets squashed

mheon · 2025-06-17T13:11:34Z

Yep, once CI is passing I'll squash down

mheon · 2025-06-18T18:46:37Z

Squashed, CI going green

Luap99

just some minor nits, overall LGTM

utils/utils.go

cmd/podman/volumes/export.go

cmd/podman/volumes/import.go

As with `volume export`, this was coded up exclusively in cmd/ instead of in libpod. Move it into Libpod, add a REST endpoint, add bindings, and now everything talks using the ContainerEngine wiring. Also similar to `volume export` this also makes things work much better with volumes that require mounting - we can now guarantee they're actually mounted, instead of just hoping. Includes some refactoring of `volume export` as well, to simplify its implementation and ensure both Import and Export work with readers/writers, as opposed to just files. Fixes containers#26409 Signed-off-by: Matt Heon <[email protected]>

Luap99

LGTM, but FYI your branch is not rebased and thus a bit out of date. Not a blocker I as I don't expect other changes to cause a conflict but still try to remember to rebase on pushes

mheon · 2025-06-24T18:04:59Z

@containers/podman-maintainers This is ready to merge, PTAL

lsm5

LGTM

lsm5

/lgtm

openshift-ci · 2025-06-25T19:45:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lsm5, Luap99, mheon

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~~ [Luap99,lsm5,mheon]

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

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 14, 2025

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jun 14, 2025

Luap99 reviewed Jun 14, 2025

View reviewed changes

mheon force-pushed the import_export branch 4 times, most recently from e4a277b to 029c547 Compare June 17, 2025 12:49

mheon force-pushed the import_export branch 4 times, most recently from f6bbae0 to da6580b Compare June 18, 2025 18:46

Luap99 reviewed Jun 19, 2025

View reviewed changes

utils/utils.go Outdated Show resolved Hide resolved

cmd/podman/volumes/export.go Outdated Show resolved Hide resolved

cmd/podman/volumes/import.go Outdated Show resolved Hide resolved

mheon force-pushed the import_export branch from da6580b to 9887645 Compare June 23, 2025 17:42

Luap99 approved these changes Jun 24, 2025

View reviewed changes

lsm5 approved these changes Jun 25, 2025

View reviewed changes

openshift-ci bot assigned lsm5 Jun 25, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2025

openshift-merge-bot bot merged commit 683e9b2 into containers:main Jun 25, 2025
76 of 77 checks passed

stale-locking-app 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 24, 2025

stale-locking-app bot locked as resolved and limited conversation to collaborators Sep 24, 2025

Add remote support for podman volume import and podman volume export #26434

Add remote support for podman volume import and podman volume export #26434

Uh oh!

Conversation

mheon commented Jun 14, 2025

Does this PR introduce a user-facing change?

Uh oh!

packit-as-a-service bot commented Jun 14, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Luap99 commented Jun 17, 2025

Uh oh!

mheon commented Jun 17, 2025

Uh oh!

mheon commented Jun 18, 2025

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

mheon commented Jun 24, 2025

Uh oh!

lsm5 left a comment

Choose a reason for hiding this comment

Uh oh!

lsm5 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add remote support for `podman volume import` and `podman volume export` #26434

Add remote support for `podman volume import` and `podman volume export` #26434