podman save use named pipe #7073

QiWang19 · 2020-07-23T19:57:34Z

podman save uses named pipe as output path, not directly using /dev/stdout.
fix #7017

Signed-off-by: Qi Wang [email protected]

QiWang19 · 2020-07-23T19:58:12Z

@mtrmac I'm not sure if this works. Could you PTAL?

mtrmac

I don’t quite think this works.

It’s probably worth splitting this into a separate function to keep the main flow fairly simple.

cmd/podman/images/save.go

mtrmac

Things that should work:

The base case of podman save | cat > file or something similar.
podman save > /dev/full should reliably fail; so should podman save | read-one-byte-and-exit.
podman save | some-very-slow-consumer should reliably receive all of the file (i.e. the save should not terminate until all of the data has been written).

See #7017 (comment) for a broad outline of what I think is necessary to make that work. (But, to be fair, I haven’t written and tested the code.)

cmd/podman/images/save.go

QiWang19 · 2020-08-04T20:27:00Z

The documentation example, are you suggesting this should fail?

$ podman save > alpine-all.tar alpine

mtrmac · 2020-08-04T22:28:06Z

The documentation exampl, are you suggesting this should fail?
$ podman save > alpine-all.tar alpine

I’m not quite sure what this refers to, but no, not in general. I was some ways to trigger failures that should be detected (note that /dev/full is a specially behaving device, not a random meaningless name).

QiWang19 · 2020-08-07T12:24:35Z

@mtrmac PTAL

cmd/podman/images/save.go

cmd/podman/images/utils_linux.go

cmd/podman/images/save.go

mtrmac · 2020-08-10T10:02:15Z

cmd/podman/images/save.go

+			return err
+		}
+		defer func() {
+			if err = cleanup(); err != nil {


AFAICS cleanup() blocks until the pipe is opened and closed, which may never happen on some of the error paths, and this would just hang.

Maybe use context.Context to terminate that wait, or just expose errc and have this function inject a fallback “wait canceled” error that is then ignored in favor of the real cause.

@mtrmac PTAL

how to use context.Context to terminate?

The way the cleanup function now returns a channel, a separate termination context might not be necessary:

func save(…) (finalErr error) { succeeded := false … if /* use pipe */ { pipePath, cleanup, err := setupPipe() … if cleanup != nil { defer func() { channel := cleanup() if succeeded { writeErr := <- channel if writeErr != nil && finalErr == nil { finalErr = writeErr } } } } } … err := registry.ImageEngine().Save(context.Background(), args[0], tags, saveOpts) if err == nil { succeeded = true } return err }

rhatdan · 2020-08-11T09:59:05Z

/approve

rhatdan · 2020-08-11T09:59:39Z

cmd/podman/images/save.go

@@ -6,6 +6,8 @@ import (
 	"strings"

 	"github.com/containers/podman/v2/libpod/define"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/crypto/ssh/terminal"



Could you eliminate this empty line.

rhatdan · 2020-08-11T10:07:43Z

cmd/podman/images/save.go

+		}
+		defer func() {
+			errc := cleanup()
+			if err := <-errc; err != nil && err.Error() != "wait canceled" {


Can you make "wait canceled" into a constant error, rather then a string. Call it Success.

rhatdan · 2020-08-11T10:09:53Z

cmd/podman/images/utils_linux.go

+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"syscall"


Please use golang.org/x/sys/unix instead of syscall

openshift-ci-robot · 2020-08-11T10:10:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, 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:

~~OWNERS~~ [rhatdan]

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

rhatdan · 2020-08-11T10:11:08Z

Can you add some tests?

QiWang19 · 2020-08-11T14:51:50Z

@mtrmac PTAL, will this avoid the hangs?

QiWang19 · 2020-08-11T19:22:46Z

Can you add some tests?

The command needs test is podman save --format oci-archive alpine | cat> test.tar
I haven't foundd ways to test the pipe using ginkgo or in system tests

mtrmac

I don’t think this works. Test by replacing the parse.ValidateFileName call with a forced err = "this failed" — the command should not hang, it should remove the temporary directory, and it should return the “this failed” error string.

mtrmac · 2020-08-11T20:05:54Z

cmd/podman/images/utils_linux.go

+			return
+		}
+		fpipe.Close()
+		errc <- errWaitCancel


Why is this reporting any error at all when the goroutine has succeeded?

cmd/podman/images/utils_linux.go

cmd/podman/images/save.go

mtrmac · 2020-08-11T20:22:13Z

Can you add some tests?

The command needs test is podman save --format oci-archive alpine | cat> test.tar
I haven't foundd ways to test the pipe using ginkgo or in system tests

podman/test/system/015-help.bats

Line 19 in 43f2771

run_podman help "$@" |\

seems to suggest it should be possible.

In Go, It might be possible to use os.Pipe + cmd.Exec to run the second command; the PodmanAsUserBase call stack would have to be extended with a stdout parameter. That might not be worth the effort.

edsantiago · 2020-08-11T20:40:14Z

seems to suggest it should be possible

Without going into too much gory detail... it's not.The BATS run function redirects stdout and stderr, no choice about it. run_podman, as a courtesy for debugging test failures, echos the captured result to stdout, and that can (with caveats) be used in some cases.

That cannot work for this PR because the critical point is that the podman command itself have the stdout filehandle as a pipe. I have given @QiWang19 a one-time permission to run $PODMAN directly in a new test, without going through run_podman, as long as there is a big scary DO NOT EVER DO THIS warning.

mtrmac · 2020-08-11T20:41:34Z

seems to suggest it should be possible

Without going into too much gory detail... it's not.

Thanks!

mtrmac

LGTM.

(I’ll leave the aesthetic nits and tests, if any, up to Podman experts.)

cmd/podman/images/save.go

mtrmac · 2020-08-12T13:43:36Z

cmd/podman/images/utils_linux.go

+
+// setupPipe() for fixing https://github.com/containers/podman/issues/7017
+// uses named pipe since containers/image EvalSymlinks fails with /dev/stdout
+func setupPipe() (string, func() <-chan error, error) {


Non-blocking: The semantics of the cleanup callback is now complex enough that it might be worth documenting. (OTOH it’s not that much code and it has a single user.)

podman save uses named pipe as output path, not directly using /dev/stdout. fix containers#7017 Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2020-08-12T18:51:40Z

@edsantiago @mtrmac @rhatdan @TomSweeneyRedHat PTAL

TomSweeneyRedHat · 2020-08-12T19:59:32Z

LGTM
thanks for the work on this @QiWang19 and @mtrmac !

rhatdan · 2020-08-12T21:22:05Z

/lgtm

openshift-ci-robot requested review from giuseppe and vrothberg July 23, 2020 19:57

mtrmac reviewed Jul 23, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2020

QiWang19 force-pushed the save-stdout branch from f03ec15 to 3974e9d Compare July 31, 2020 13:01

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 31, 2020

QiWang19 force-pushed the save-stdout branch from 3974e9d to 84812ad Compare July 31, 2020 21:50

mtrmac reviewed Aug 1, 2020

View reviewed changes

QiWang19 force-pushed the save-stdout branch 3 times, most recently from 9549ffd to cb9dbb5 Compare August 4, 2020 20:24

QiWang19 force-pushed the save-stdout branch from cb9dbb5 to 0795828 Compare August 6, 2020 21:55

QiWang19 force-pushed the save-stdout branch 2 times, most recently from 6890d2c to f626be2 Compare August 7, 2020 16:01

mtrmac reviewed Aug 7, 2020

View reviewed changes

cmd/podman/images/save.go Outdated Show resolved Hide resolved

cmd/podman/images/save.go Outdated Show resolved Hide resolved

cmd/podman/images/utils_linux.go Outdated Show resolved Hide resolved

cmd/podman/images/utils_linux.go Show resolved Hide resolved

QiWang19 force-pushed the save-stdout branch from f626be2 to a650faf Compare August 7, 2020 20:43

mtrmac reviewed Aug 7, 2020

View reviewed changes

cmd/podman/images/save.go Outdated Show resolved Hide resolved

QiWang19 force-pushed the save-stdout branch 2 times, most recently from 78eaf92 to d2578ea Compare August 8, 2020 01:01

mtrmac reviewed Aug 10, 2020

View reviewed changes

QiWang19 force-pushed the save-stdout branch 3 times, most recently from 17c383f to 0e50185 Compare August 10, 2020 20:56

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2020

rhatdan approved these changes Aug 11, 2020

View reviewed changes

QiWang19 force-pushed the save-stdout branch from 0e50185 to 661a9a9 Compare August 11, 2020 19:20

mtrmac reviewed Aug 11, 2020

View reviewed changes

mtrmac mentioned this pull request Aug 12, 2020

podman load/save: support multi-image docker archive #6811

Merged

QiWang19 force-pushed the save-stdout branch 2 times, most recently from a8e9594 to 2937453 Compare August 12, 2020 13:42

mtrmac reviewed Aug 12, 2020

View reviewed changes

QiWang19 force-pushed the save-stdout branch from 2937453 to 268e7e8 Compare August 12, 2020 13:54

podman save use named pipe

49dea06

podman save uses named pipe as output path, not directly using /dev/stdout. fix containers#7017 Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the save-stdout branch from 268e7e8 to 49dea06 Compare August 12, 2020 14:03

openshift-ci-robot assigned rhatdan Aug 12, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2020

openshift-merge-robot merged commit d777a7b into containers:master Aug 12, 2020

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 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podman save use named pipe #7073

podman save use named pipe #7073

QiWang19 commented Jul 23, 2020

QiWang19 commented Jul 23, 2020

mtrmac left a comment

mtrmac left a comment

QiWang19 commented Aug 4, 2020 •

edited

Loading

mtrmac commented Aug 4, 2020

QiWang19 commented Aug 7, 2020

mtrmac Aug 10, 2020

QiWang19 Aug 10, 2020

QiWang19 Aug 11, 2020

mtrmac Aug 11, 2020

rhatdan commented Aug 11, 2020

rhatdan Aug 11, 2020

rhatdan Aug 11, 2020

rhatdan Aug 11, 2020

openshift-ci-robot commented Aug 11, 2020

rhatdan commented Aug 11, 2020

QiWang19 commented Aug 11, 2020

QiWang19 commented Aug 11, 2020

mtrmac left a comment

mtrmac Aug 11, 2020

mtrmac commented Aug 11, 2020

edsantiago commented Aug 11, 2020

mtrmac commented Aug 11, 2020

mtrmac left a comment

mtrmac Aug 12, 2020

QiWang19 commented Aug 12, 2020

TomSweeneyRedHat commented Aug 12, 2020

rhatdan commented Aug 12, 2020

podman save use named pipe #7073

podman save use named pipe #7073

Conversation

QiWang19 commented Jul 23, 2020

QiWang19 commented Jul 23, 2020

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

QiWang19 commented Aug 4, 2020 • edited Loading

mtrmac commented Aug 4, 2020

QiWang19 commented Aug 7, 2020

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

rhatdan commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 11, 2020

rhatdan commented Aug 11, 2020

QiWang19 commented Aug 11, 2020

QiWang19 commented Aug 11, 2020

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Aug 11, 2020

edsantiago commented Aug 11, 2020

mtrmac commented Aug 11, 2020

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QiWang19 commented Aug 12, 2020

TomSweeneyRedHat commented Aug 12, 2020

rhatdan commented Aug 12, 2020

QiWang19 commented Aug 4, 2020 •

edited

Loading