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 <mheon@redhat.com>

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