From b6775d5d22d463e4d92d6358ccd48dab6f8a1862 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Mon, 11 Feb 2019 12:57:08 -0500 Subject: Fix manual detach from containers to not wait for exit At present, when manually detaching from an attached container (using the detach hotkeys, default C-p C-q), Podman will still wait for the container to exit to obtain its exit code (so we can set Podman's exit code to match). This is correct in the case where attach finished because the container exited, but very wrong for the manual detach case. As a result of this, we can no longer guarantee that the cleanup and --rm functions will fire at the end of 'podman run' - we may be exiting before we get that far. Cleanup is easy enough - we swap to unconditionally using the cleanup processes we've used for detached and rootless containers all along. To duplicate --rm we need to also teach 'podman cleanup' to optionally remove containers instead of cleaning them up. (There is an argument for just using 'podman rm' instead of 'podman cleanup --rm', but cleanup does have different semantics given that we only ever expect it to run when the container has just exited. I think it might be useful to keep the two separate for things like 'podman events'...) Signed-off-by: Matthew Heon --- cmd/podman/attach.go | 2 +- cmd/podman/cleanup.go | 22 ++++++++++++++++++---- cmd/podman/cliconfig/config.go | 1 + cmd/podman/run.go | 24 ++++++++---------------- cmd/podman/start.go | 7 +++++++ cmd/podman/utils.go | 2 +- libpod/container_attach_linux.go | 4 ++-- libpod/errors.go | 5 +++++ pkg/spec/createconfig.go | 17 ++++++++++------- utils/utils.go | 12 +++++------- 10 files changed, 58 insertions(+), 38 deletions(-) diff --git a/cmd/podman/attach.go b/cmd/podman/attach.go index c29886825..8e760582d 100644 --- a/cmd/podman/attach.go +++ b/cmd/podman/attach.go @@ -74,7 +74,7 @@ func attachCmd(c *cliconfig.AttachValues) error { inputStream = nil } - if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false); err != nil { + if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false); err != nil && err != libpod.ErrDetach { return errors.Wrapf(err, "error attaching to container %s", ctr.ID()) } diff --git a/cmd/podman/cleanup.go b/cmd/podman/cleanup.go index b1f727d33..064551189 100644 --- a/cmd/podman/cleanup.go +++ b/cmd/podman/cleanup.go @@ -37,6 +37,7 @@ func init() { flags.BoolVarP(&cleanupCommand.All, "all", "a", false, "Cleans up all containers") flags.BoolVarP(&cleanupCommand.Latest, "latest", "l", false, "Act on the latest container podman is aware of") + flags.BoolVar(&cleanupCommand.Rm, "rm", false, "After cleanup, remove the container entirely") } func cleanupCmd(c *cliconfig.CleanupValues) error { @@ -55,12 +56,25 @@ func cleanupCmd(c *cliconfig.CleanupValues) error { ctx := getContext() for _, ctr := range cleanupContainers { - if err = ctr.Cleanup(ctx); err != nil { - if lastError != nil { - fmt.Fprintln(os.Stderr, lastError) + hadError := false + if c.Rm { + if err := runtime.RemoveContainer(ctx, ctr, false); err != nil { + if lastError != nil { + fmt.Fprintln(os.Stderr, lastError) + } + lastError = errors.Wrapf(err, "failed to cleanup and remove container %v", ctr.ID()) + hadError = true } - lastError = errors.Wrapf(err, "failed to cleanup container %v", ctr.ID()) } else { + if err := ctr.Cleanup(ctx); err != nil { + if lastError != nil { + fmt.Fprintln(os.Stderr, lastError) + } + lastError = errors.Wrapf(err, "failed to cleanup container %v", ctr.ID()) + hadError = true + } + } + if !hadError { fmt.Println(ctr.ID()) } } diff --git a/cmd/podman/cliconfig/config.go b/cmd/podman/cliconfig/config.go index b925d29ff..e895b4a49 100644 --- a/cmd/podman/cliconfig/config.go +++ b/cmd/podman/cliconfig/config.go @@ -531,6 +531,7 @@ type CleanupValues struct { PodmanCommand All bool Latest bool + Rm bool } type SystemPruneValues struct { diff --git a/cmd/podman/run.go b/cmd/podman/run.go index 8649dc190..6002578ff 100644 --- a/cmd/podman/run.go +++ b/cmd/podman/run.go @@ -118,6 +118,14 @@ func runCmd(c *cliconfig.RunValues) error { } } if err := startAttachCtr(ctr, outputStream, errorStream, inputStream, c.String("detach-keys"), c.Bool("sig-proxy"), true); err != nil { + // We've manually detached from the container + // Do not perform cleanup, or wait for container exit code + // Just exit immediately + if err == libpod.ErrDetach { + exitCode = 0 + return nil + } + // This means the command did not exist exitCode = 127 if strings.Index(err.Error(), "permission denied") > -1 { @@ -147,22 +155,6 @@ func runCmd(c *cliconfig.RunValues) error { exitCode = int(ecode) } - if createConfig.Rm { - return runtime.RemoveContainer(ctx, ctr, true) - } - - if err := ctr.Cleanup(ctx); err != nil { - // If the container has been removed already, no need to error on cleanup - // Also, if it was restarted, don't error either - if errors.Cause(err) == libpod.ErrNoSuchCtr || - errors.Cause(err) == libpod.ErrCtrRemoved || - errors.Cause(err) == libpod.ErrCtrStateInvalid { - return nil - } - - return err - } - return nil } diff --git a/cmd/podman/start.go b/cmd/podman/start.go index 344719fca..1de258aa4 100644 --- a/cmd/podman/start.go +++ b/cmd/podman/start.go @@ -108,6 +108,13 @@ func startCmd(c *cliconfig.StartValues) error { // attach to the container and also start it not already running err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning) + if err == libpod.ErrDetach { + // User manually detached + // Exit cleanly immediately + exitCode = 0 + return nil + } + if ctrRunning { return err } diff --git a/cmd/podman/utils.go b/cmd/podman/utils.go index 744d010d5..c62015344 100644 --- a/cmd/podman/utils.go +++ b/cmd/podman/utils.go @@ -91,7 +91,7 @@ func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detac err = <-attachChan if err != nil { - return errors.Wrapf(err, "error attaching to container %s", ctr.ID()) + return err } return nil diff --git a/libpod/container_attach_linux.go b/libpod/container_attach_linux.go index 1d6f0bd96..3ff6ddc76 100644 --- a/libpod/container_attach_linux.go +++ b/libpod/container_attach_linux.go @@ -109,8 +109,8 @@ func (c *Container) attachContainerSocket(resize <-chan remotecommand.TerminalSi case err := <-receiveStdoutError: return err case err := <-stdinDone: - if _, ok := err.(utils.DetachError); ok { - return nil + if err == ErrDetach { + return err } if streams.AttachOutput || streams.AttachError { return <-receiveStdoutError diff --git a/libpod/errors.go b/libpod/errors.go index 30a19d30f..dd82d0796 100644 --- a/libpod/errors.go +++ b/libpod/errors.go @@ -4,6 +4,7 @@ import ( "errors" "github.com/containers/libpod/libpod/image" + "github.com/containers/libpod/utils" ) var ( @@ -56,6 +57,10 @@ var ( // ErrInternal indicates an internal library error ErrInternal = errors.New("internal libpod error") + // ErrDetach indicates that an attach session was manually detached by + // the user. + ErrDetach = utils.ErrDetach + // ErrRuntimeStopped indicates that the runtime has already been shut // down and no further operations can be performed on it ErrRuntimeStopped = errors.New("runtime has already been stopped") diff --git a/pkg/spec/createconfig.go b/pkg/spec/createconfig.go index f6c77d61c..8edab831f 100644 --- a/pkg/spec/createconfig.go +++ b/pkg/spec/createconfig.go @@ -11,7 +11,6 @@ import ( "github.com/containers/libpod/libpod" "github.com/containers/libpod/pkg/namespaces" - "github.com/containers/libpod/pkg/rootless" "github.com/containers/storage" "github.com/cri-o/ocicni/pkg/ocicni" "github.com/docker/go-connections/nat" @@ -340,7 +339,13 @@ func (c *CreateConfig) createExitCommand() []string { if c.Syslog { command = append(command, "--syslog") } - return append(command, []string{"container", "cleanup"}...) + command = append(command, []string{"container", "cleanup"}...) + + if c.Rm { + command = append(command, "--rm") + } + + return command } // GetContainerCreateOptions takes a CreateConfig and returns a slice of CtrCreateOptions @@ -518,11 +523,9 @@ func (c *CreateConfig) GetContainerCreateOptions(runtime *libpod.Runtime, pod *l if c.CgroupParent != "" { options = append(options, libpod.WithCgroupParent(c.CgroupParent)) } - // For a rootless container always cleanup the storage/network as they - // run in a different namespace thus not reusable when we restart. - if c.Detach || rootless.IsRootless() { - options = append(options, libpod.WithExitCommand(c.createExitCommand())) - } + + // Always use a cleanup process to clean up Podman after termination + options = append(options, libpod.WithExitCommand(c.createExitCommand())) return options, nil } diff --git a/utils/utils.go b/utils/utils.go index c7c5ab5cf..4a91b304f 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -9,6 +9,7 @@ import ( systemdDbus "github.com/coreos/go-systemd/dbus" "github.com/godbus/dbus" + "github.com/pkg/errors" ) // ExecCmd executes a command with args and returns its output as a string along @@ -82,12 +83,9 @@ func newProp(name string, units interface{}) systemdDbus.Property { } } -// DetachError is special error which returned in case of container detach. -type DetachError struct{} - -func (DetachError) Error() string { - return "detached from container" -} +// ErrDetach is an error indicating that the user manually detached from the +// container. +var ErrDetach = errors.New("detached from container") // CopyDetachable is similar to io.Copy but support a detach key sequence to break out. func CopyDetachable(dst io.Writer, src io.Reader, keys []byte) (written int64, err error) { @@ -108,7 +106,7 @@ func CopyDetachable(dst io.Writer, src io.Reader, keys []byte) (written int64, e } if i == len(keys)-1 { // src.Close() - return 0, DetachError{} + return 0, ErrDetach } nr, er = src.Read(buf) } -- cgit v1.2.3-54-g00ecf From 19a03976f73d21a821a07ae5f24250a4ceaee33a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 12 Feb 2019 12:57:11 -0500 Subject: Retain a copy of container exit file on cleanup When cleaning up containers, we presently remove the exit file created by Conmon, to ensure that if we restart the container, we won't have conflicts when Conmon tries writing a new exit file. Unfortunately, we need to retain that exit file (at least until we get a workable events system), so we can read it in cases where the container has been removed before 'podman run' can read its exit code. So instead of removing it, rename it, so there's no conflict with Conmon, and we can still read it later. Fixes: #1640 Signed-off-by: Matthew Heon --- cmd/podman/run.go | 2 +- libpod/container_internal.go | 15 +++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/cmd/podman/run.go b/cmd/podman/run.go index 6002578ff..86dc02dea 100644 --- a/cmd/podman/run.go +++ b/cmd/podman/run.go @@ -160,7 +160,7 @@ func runCmd(c *cliconfig.RunValues) error { // Read a container's exit file func readExitFile(runtimeTmp, ctrID string) (int, error) { - exitFile := filepath.Join(runtimeTmp, "exits", ctrID) + exitFile := filepath.Join(runtimeTmp, "exits", fmt.Sprintf("%s-old", ctrID)) logrus.Debugf("Attempting to read container %s exit code from file %s", ctrID, exitFile) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index b0dcc853e..f82cbd674 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -489,9 +489,20 @@ func (c *Container) removeConmonFiles() error { return errors.Wrapf(err, "error removing container %s OOM file", c.ID()) } + // Instead of outright deleting the exit file, rename it (if it exists). + // We want to retain it so we can get the exit code of containers which + // are removed (at least until we have a workable events system) exitFile := filepath.Join(c.runtime.ociRuntime.exitsDir, c.ID()) - if err := os.Remove(exitFile); err != nil && !os.IsNotExist(err) { - return errors.Wrapf(err, "error removing container %s exit file", c.ID()) + oldExitFile := filepath.Join(c.runtime.ociRuntime.exitsDir, fmt.Sprintf("%s-old", c.ID())) + if _, err := os.Stat(exitFile); err != nil { + if !os.IsNotExist(err) { + return errors.Wrapf(err, "error running stat on container %s exit file", c.ID()) + } + } else if err == nil { + // Rename should replace the old exit file (if it exists) + if err := os.Rename(exitFile, oldExitFile); err != nil { + return errors.Wrapf(err, "error renaming container %s exit file", c.ID()) + } } return nil -- cgit v1.2.3-54-g00ecf From 28ee842b769b5ac4ff9a99a6c21fc66a4d011b9b Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 12 Feb 2019 13:11:07 -0500 Subject: Address review comments on #2319 Signed-off-by: Matthew Heon --- cmd/podman/attach.go | 2 +- cmd/podman/cleanup.go | 4 ++-- cmd/podman/cliconfig/config.go | 2 +- cmd/podman/run.go | 2 +- cmd/podman/start.go | 2 +- cmd/podman/utils.go | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/cmd/podman/attach.go b/cmd/podman/attach.go index 8e760582d..ed175bdf4 100644 --- a/cmd/podman/attach.go +++ b/cmd/podman/attach.go @@ -74,7 +74,7 @@ func attachCmd(c *cliconfig.AttachValues) error { inputStream = nil } - if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false); err != nil && err != libpod.ErrDetach { + if err := startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, c.SigProxy, false); err != nil && errors.Cause(err) != libpod.ErrDetach { return errors.Wrapf(err, "error attaching to container %s", ctr.ID()) } diff --git a/cmd/podman/cleanup.go b/cmd/podman/cleanup.go index 064551189..537679d75 100644 --- a/cmd/podman/cleanup.go +++ b/cmd/podman/cleanup.go @@ -37,7 +37,7 @@ func init() { flags.BoolVarP(&cleanupCommand.All, "all", "a", false, "Cleans up all containers") flags.BoolVarP(&cleanupCommand.Latest, "latest", "l", false, "Act on the latest container podman is aware of") - flags.BoolVar(&cleanupCommand.Rm, "rm", false, "After cleanup, remove the container entirely") + flags.BoolVar(&cleanupCommand.Remove, "rm", false, "After cleanup, remove the container entirely") } func cleanupCmd(c *cliconfig.CleanupValues) error { @@ -57,7 +57,7 @@ func cleanupCmd(c *cliconfig.CleanupValues) error { for _, ctr := range cleanupContainers { hadError := false - if c.Rm { + if c.Remove { if err := runtime.RemoveContainer(ctx, ctr, false); err != nil { if lastError != nil { fmt.Fprintln(os.Stderr, lastError) diff --git a/cmd/podman/cliconfig/config.go b/cmd/podman/cliconfig/config.go index e895b4a49..85ded6da0 100644 --- a/cmd/podman/cliconfig/config.go +++ b/cmd/podman/cliconfig/config.go @@ -531,7 +531,7 @@ type CleanupValues struct { PodmanCommand All bool Latest bool - Rm bool + Remove bool } type SystemPruneValues struct { diff --git a/cmd/podman/run.go b/cmd/podman/run.go index 86dc02dea..16ec7c3c0 100644 --- a/cmd/podman/run.go +++ b/cmd/podman/run.go @@ -121,7 +121,7 @@ func runCmd(c *cliconfig.RunValues) error { // We've manually detached from the container // Do not perform cleanup, or wait for container exit code // Just exit immediately - if err == libpod.ErrDetach { + if errors.Cause(err) == libpod.ErrDetach { exitCode = 0 return nil } diff --git a/cmd/podman/start.go b/cmd/podman/start.go index 1de258aa4..d1434508d 100644 --- a/cmd/podman/start.go +++ b/cmd/podman/start.go @@ -108,7 +108,7 @@ func startCmd(c *cliconfig.StartValues) error { // attach to the container and also start it not already running err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning) - if err == libpod.ErrDetach { + if errors.Cause(err) == libpod.ErrDetach { // User manually detached // Exit cleanly immediately exitCode = 0 diff --git a/cmd/podman/utils.go b/cmd/podman/utils.go index c62015344..744d010d5 100644 --- a/cmd/podman/utils.go +++ b/cmd/podman/utils.go @@ -91,7 +91,7 @@ func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detac err = <-attachChan if err != nil { - return err + return errors.Wrapf(err, "error attaching to container %s", ctr.ID()) } return nil -- cgit v1.2.3-54-g00ecf From b1770ecc5b2eb2af900f5c57381165e5d1180e0a Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Tue, 12 Feb 2019 14:18:23 -0500 Subject: Remove a lot of '--rm' options from unit tests Previously, 'podman create --rm' did not work - it wouldn't error but it did nothing. It is now fixed, but unfortunately the unit tests used it a lot, in ways that just do not work when it actually functions. Begin the process of fixing now-failing tests. Signed-off-by: Matthew Heon --- test/e2e/create_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/create_test.go b/test/e2e/create_test.go index b28a0c428..9a526b778 100644 --- a/test/e2e/create_test.go +++ b/test/e2e/create_test.go @@ -142,7 +142,7 @@ var _ = Describe("Podman create", func() { } mountPath := filepath.Join(podmanTest.TempDir, "secrets") os.Mkdir(mountPath, 0755) - session := podmanTest.Podman([]string{"create", "--name", "test", "--rm", "--mount", fmt.Sprintf("type=bind,src=%s,target=/create/test", mountPath), ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) + session := podmanTest.Podman([]string{"create", "--name", "test", "--mount", fmt.Sprintf("type=bind,src=%s,target=/create/test", mountPath), ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) session = podmanTest.Podman([]string{"start", "test"}) @@ -153,7 +153,7 @@ var _ = Describe("Podman create", func() { Expect(session.ExitCode()).To(Equal(0)) Expect(session.OutputToString()).To(ContainSubstring("/create/test rw")) - session = podmanTest.Podman([]string{"create", "--name", "test_ro", "--rm", "--mount", fmt.Sprintf("type=bind,src=%s,target=/create/test,ro", mountPath), ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) + session = podmanTest.Podman([]string{"create", "--name", "test_ro", "--mount", fmt.Sprintf("type=bind,src=%s,target=/create/test,ro", mountPath), ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) session = podmanTest.Podman([]string{"start", "test_ro"}) @@ -164,7 +164,7 @@ var _ = Describe("Podman create", func() { Expect(session.ExitCode()).To(Equal(0)) Expect(session.OutputToString()).To(ContainSubstring("/create/test ro")) - session = podmanTest.Podman([]string{"create", "--name", "test_shared", "--rm", "--mount", fmt.Sprintf("type=bind,src=%s,target=/create/test,shared", mountPath), ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) + session = podmanTest.Podman([]string{"create", "--name", "test_shared", "--mount", fmt.Sprintf("type=bind,src=%s,target=/create/test,shared", mountPath), ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) session = podmanTest.Podman([]string{"start", "test_shared"}) @@ -180,7 +180,7 @@ var _ = Describe("Podman create", func() { mountPath = filepath.Join(podmanTest.TempDir, "scratchpad") os.Mkdir(mountPath, 0755) - session = podmanTest.Podman([]string{"create", "--name", "test_tmpfs", "--rm", "--mount", "type=tmpfs,target=/create/test", ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) + session = podmanTest.Podman([]string{"create", "--name", "test_tmpfs", "--mount", "type=tmpfs,target=/create/test", ALPINE, "grep", "/create/test", "/proc/self/mountinfo"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) session = podmanTest.Podman([]string{"start", "test_tmpfs"}) -- cgit v1.2.3-54-g00ecf From 9d4e7fe58b10c4130340a151a377cf7be8aaa810 Mon Sep 17 00:00:00 2001 From: Matthew Heon Date: Wed, 13 Feb 2019 11:41:20 -0500 Subject: Try disabling --rm on notify_socket test We have a consistent CI failure with the notify_socket test that I can't reproduce locally. There's no reason for the test to have --rm, so try removing it. Signed-off-by: Matthew Heon --- test/e2e/run_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go index 22a36bb6c..6bd49de33 100644 --- a/test/e2e/run_test.go +++ b/test/e2e/run_test.go @@ -322,7 +322,7 @@ var _ = Describe("Podman run", func() { os.Setenv("NOTIFY_SOCKET", sock) defer os.Unsetenv("NOTIFY_SOCKET") - session := podmanTest.Podman([]string{"run", "--rm", ALPINE, "printenv", "NOTIFY_SOCKET"}) + session := podmanTest.Podman([]string{"run", ALPINE, "printenv", "NOTIFY_SOCKET"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) Expect(len(session.OutputToStringArray())).To(BeNumerically(">", 0)) -- cgit v1.2.3-54-g00ecf