Merge pull request #2319 from mheon/unconditional_cleanup

Fix manual detach from containers to not wait for exit

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 && errors.Cause(err) != libpod.ErrDetach {
 		return errors.Wrapf(err, "error attaching to container %s", ctr.ID())
 	}
 

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.Remove, "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.Remove {
+			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())
 		}
 	}

cmd/podman/cliconfig/config.go

@@ -531,6 +531,7 @@ type CleanupValues struct {
 	PodmanCommand
 	All    bool
 	Latest bool
+	Remove bool
 }
 
 type SystemPruneValues struct {

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 errors.Cause(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,28 +155,12 @@ 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
 }
 
 // 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)
 

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 errors.Cause(err) == libpod.ErrDetach {
+				// User manually detached
+				// Exit cleanly immediately
+				exitCode = 0
+				return nil
+			}
+
 			if ctrRunning {
 				return err
 			}

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

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

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")

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
 }

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"})

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

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)
 			}