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]>

openshift-ci · 2025-06-14T13:24:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [mheon]

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

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

@@ -46,54 +38,20 @@ func init() {
 	flags := exportCommand.Flags()

 	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

@@ -139,3 +144,59 @@ func Exists(ctx context.Context, nameOrID string, options *ExistsOptions) (bool,

 	return response.IsSuccess(), nil
 }
+
+// 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

@@ -51,8 +51,7 @@ func ExecCmdWithStdStreams(stdin io.Reader, stdout, stderr io.Writer, env []stri
 }

 // 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

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]>

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

Squashed, CI going green

Luap99

just some minor nits, overall LGTM

Luap99 · 2025-06-19T09:54:31Z

utils/utils.go

-	logrus.Debugf("untarring %s", tarball.Name())
-	return archive.Untar(tarball, dest, options)
-}
-
 // Creates a new tar file and writes bytes from io.ReadCloser
 func CreateTarFromSrc(source string, dest string) error {


this is unused now so it should be removed

Luap99 · 2025-06-19T09:57:40Z

cmd/podman/volumes/export.go

+	if err := containerEngine.VolumeExport(ctx, args[0], exportOpts); err != nil {
+		return err


nit might as well just return directly here

Luap99 · 2025-06-19T09:58:04Z

cmd/podman/volumes/import.go

+	containerEngine := registry.ContainerEngine()
+	ctx := context.Background()
+
+	if err := containerEngine.VolumeImport(ctx, args[0], opts); err != nil {


nit: same thing could return directly with the err != nil

openshift-ci bot added the release-note label Jun 14, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label 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 3 times, most recently from 2a2f435 to f6bbae0 Compare June 18, 2025 14:14

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

Luap99 reviewed Jun 19, 2025

View reviewed changes

		if err := containerEngine.VolumeExport(ctx, args[0], exportOpts); err != nil {
		return err

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

Are you sure you want to change the base?

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!

openshift-ci bot commented Jun 14, 2025

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!

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!

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

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