Handle --rm when starting a container

podman start should follow the same behaviour as podman run when removing a container. Signed-off-by: Daniel J Walsh <[email protected]>
containers · Dec 11, 2020 · 098e0a7 · 098e0a7
1 parent dd95478
commit 098e0a7
Show file tree

Hide file tree

Showing 6 changed files with 77 additions and 26 deletions.
diff --git a/docs/source/markdown/podman-create.1.md b/docs/source/markdown/podman-create.1.md
@@ -803,10 +803,6 @@ To generate systemd unit files, please see *podman generate systemd*
 
 Automatically remove the container when it exits. The default is *false*.
 
-Note that the container will not be removed when it could not be created or
-started successfully. This allows the user to inspect the container after
-failure.
-
 #### **--rootfs**
 
 If specified, the first argument refers to an exploded container on the file system.

diff --git a/docs/source/markdown/podman-run.1.md b/docs/source/markdown/podman-run.1.md
@@ -840,10 +840,6 @@ To generate systemd unit files, please see **podman generate systemd**.
 
 Automatically remove the container when it exits. The default is **false**.
 
-Note that the container will not be removed when it could not be created or
-started successfully. This allows the user to inspect the container after
-failure.
-
 #### **--rmi**=*true|false*
 
 After exit of the container, remove the image unless another

diff --git a/pkg/domain/entities/container_ps.go b/pkg/domain/entities/container_ps.go
@@ -12,6 +12,8 @@ import (
 
 // Listcontainer describes a container suitable for listing
 type ListContainer struct {
+	// AutoRemove
+	AutoRemove bool
 	// Container command
 	Command []string
 	// Container creation time

diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
@@ -515,6 +515,29 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 				reports = append(reports, &report)
 				return reports, errors.Wrapf(report.Err, "unable to start container %s", name)
 			}
+			if ctr.AutoRemove {
+				// Defer the removal, so we can return early if needed and
+				// de-spaghetti the code.
+				defer func() {
+					shouldRestart, err := containers.ShouldRestart(ic.ClientCxt, ctr.ID)
+					if err != nil {
+						logrus.Errorf("Failed to check if %s should restart: %v", ctr.ID, err)
+						return
+					}
+
+					if !shouldRestart {
+						if err := containers.Remove(ic.ClientCxt, ctr.ID, bindings.PFalse, bindings.PTrue); err != nil {
+							if errorhandling.Contains(err, define.ErrNoSuchCtr) ||
+								errorhandling.Contains(err, define.ErrCtrRemoved) {
+								logrus.Warnf("Container %s does not exist: %v", ctr.ID, err)
+							} else {
+								logrus.Errorf("Error removing container %s: %v", ctr.ID, err)
+							}
+						}
+					}
+				}()
+			}
+
 			exitCode, err := containers.Wait(ic.ClientCxt, name, nil)
 			if err == define.ErrNoSuchCtr {
 				// Check events
@@ -535,6 +558,16 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 		if !ctrRunning {
 			err = containers.Start(ic.ClientCxt, name, &options.DetachKeys)
 			if err != nil {
+				if ctr.AutoRemove {
+					if err := containers.Remove(ic.ClientCxt, ctr.ID, bindings.PFalse, bindings.PTrue); err != nil {
+						if errorhandling.Contains(err, define.ErrNoSuchCtr) ||
+							errorhandling.Contains(err, define.ErrCtrRemoved) {
+							logrus.Warnf("Container %s does not exist: %v", ctr.ID, err)
+						} else {
+							logrus.Errorf("Error removing container %s: %v", ctr.ID, err)
+						}
+					}
+				}
 				report.Err = errors.Wrapf(err, "unable to start container %q", name)
 				report.ExitCode = define.ExitCode(err)
 				reports = append(reports, &report)

diff --git a/pkg/ps/ps.go b/pkg/ps/ps.go
@@ -179,24 +179,25 @@ func ListContainerBatch(rt *libpod.Runtime, ctr *libpod.Container, opts entities
 	}
 
 	ps := entities.ListContainer{
-		Command:   conConfig.Command,
-		Created:   conConfig.CreatedTime,
-		Exited:    exited,
-		ExitCode:  exitCode,
-		ExitedAt:  exitedTime.Unix(),
-		ID:        conConfig.ID,
-		Image:     conConfig.RootfsImageName,
-		ImageID:   conConfig.RootfsImageID,
-		IsInfra:   conConfig.IsInfra,
-		Labels:    conConfig.Labels,
-		Mounts:    ctr.UserVolumes(),
-		Names:     []string{conConfig.Name},
-		Pid:       pid,
-		Pod:       conConfig.Pod,
-		Ports:     portMappings,
-		Size:      size,
-		StartedAt: startedTime.Unix(),
-		State:     conState.String(),
+		AutoRemove: ctr.AutoRemove(),
+		Command:    conConfig.Command,
+		Created:    conConfig.CreatedTime,
+		Exited:     exited,
+		ExitCode:   exitCode,
+		ExitedAt:   exitedTime.Unix(),
+		ID:         conConfig.ID,
+		Image:      conConfig.RootfsImageName,
+		ImageID:    conConfig.RootfsImageID,
+		IsInfra:    conConfig.IsInfra,
+		Labels:     conConfig.Labels,
+		Mounts:     ctr.UserVolumes(),
+		Names:      []string{conConfig.Name},
+		Pid:        pid,
+		Pod:        conConfig.Pod,
+		Ports:      portMappings,
+		Size:       size,
+		StartedAt:  startedTime.Unix(),
+		State:      conState.String(),
 	}
 	if opts.Pod && len(conConfig.Pod) > 0 {
 		podName, err := rt.GetName(conConfig.Pod)

diff --git a/test/e2e/start_test.go b/test/e2e/start_test.go
@@ -49,6 +49,29 @@ var _ = Describe("Podman start", func() {
 		Expect(session.ExitCode()).To(Equal(0))
 	})
 
+	It("podman start --rm removed on failure", func() {
+		session := podmanTest.Podman([]string{"create", "--name=test", "--rm", ALPINE, "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		session = podmanTest.Podman([]string{"start", "test"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(125))
+		session = podmanTest.Podman([]string{"container", "exists", "test"})
+		Expect(session.ExitCode()).To(Not(Equal(0)))
+	})
+
+	It("podman start --rm --attach removed on failure", func() {
+		session := podmanTest.Podman([]string{"create", "--rm", ALPINE, "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		cid := session.OutputToString()
+		session = podmanTest.Podman([]string{"start", "--attach", cid})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(125))
+		session = podmanTest.Podman([]string{"container", "exists", cid})
+		Expect(session.ExitCode()).To(Not(Equal(0)))
+	})
+
 	It("podman container start single container by id", func() {
 		session := podmanTest.Podman([]string{"container", "create", ALPINE, "ls"})
 		session.WaitWithDefaultTimeout()