With --rm option remove container if podman run fails

Fixes containers#15049 Signed-off-by: Daniel J Walsh <[email protected]> <MH: Fixed cherry-pick conflicts> Signed-off-by: Matthew Heon <[email protected]>
mheon · Aug 10, 2022 · 7efd81c · 7efd81c
1 parent 346b22f
commit 7efd81c
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 12 deletions.
diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
@@ -1035,6 +1035,15 @@ func (ic *ContainerEngine) Diff(ctx context.Context, namesOrIDs []string, opts e
 }
 
 func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.ContainerRunOptions) (*entities.ContainerRunReport, error) {
+	removeContainer := func(ctr *libpod.Container, force bool) error {
+		var timeout *uint
+		if err := ic.Libpod.RemoveContainer(ctx, ctr, force, true, timeout); err != nil {
+			logrus.Debugf("unable to remove container %s after failing to start and attach to it: %v", ctr.ID(), err)
+			return err
+		}
+		return nil
+	}
+
 	warn, err := generate.CompleteSpec(ctx, ic.Libpod, opts.Spec)
 	if err != nil {
 		return nil, err
@@ -1055,6 +1064,8 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 
 	if opts.CIDFile != "" {
 		if err := util.CreateCidFile(opts.CIDFile, ctr.ID()); err != nil {
+			// If you fail to create CIDFile then remove the container
+			_ = removeContainer(ctr, true)
 			return nil, err
 		}
 	}
@@ -1072,6 +1083,11 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 		if err := ctr.Start(ctx, true); err != nil {
 			// This means the command did not exist
 			report.ExitCode = define.ExitCode(err)
+			if opts.Rm {
+				if rmErr := removeContainer(ctr, true); rmErr != nil && !errors.Is(rmErr, define.ErrNoSuchCtr) {
+					logrus.Errorf("Container %s failed to be removed", ctr.ID())
+				}
+			}
 			return &report, err
 		}
 
@@ -1088,10 +1104,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 			return &report, nil
 		}
 		if opts.Rm {
-			var timeout *uint
-			if deleteError := ic.Libpod.RemoveContainer(ctx, ctr, true, false, timeout); deleteError != nil {
-				logrus.Debugf("unable to remove container %s after failing to start and attach to it", ctr.ID())
-			}
+			_ = removeContainer(ctr, true)
 		}
 		if errors.Is(err, define.ErrWillDeadlock) {
 			logrus.Debugf("Deadlock error on %q: %v", ctr.ID(), err)
@@ -1103,8 +1116,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 	}
 	report.ExitCode = ic.GetContainerExitCode(ctx, ctr)
 	if opts.Rm && !ctr.ShouldRestart(ctx) {
-		var timeout *uint
-		if err := ic.Libpod.RemoveContainer(ctx, ctr, false, true, timeout); err != nil {
+		if err := removeContainer(ctr, false); err != nil {
 			if errors.Is(err, define.ErrNoSuchCtr) ||
 				errors.Is(err, define.ErrCtrRemoved) {
 				logrus.Infof("Container %s was already removed, skipping --rm", ctr.ID())

diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
@@ -771,8 +771,17 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 	for _, w := range con.Warnings {
 		fmt.Fprintf(os.Stderr, "%s\n", w)
 	}
+	removeContainer := func(id string, force bool) error {
+		removeOptions := new(containers.RemoveOptions).WithVolumes(true).WithForce(force)
+		reports, err := containers.Remove(ic.ClientCtx, id, removeOptions)
+		logIfRmError(id, err, reports)
+		return err
+	}
+
 	if opts.CIDFile != "" {
 		if err := util.CreateCidFile(opts.CIDFile, con.ID); err != nil {
+			// If you fail to create CIDFile then remove the container
+			_ = removeContainer(con.ID, true)
 			return nil, err
 		}
 	}
@@ -784,6 +793,11 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 		err := containers.Start(ic.ClientCtx, con.ID, new(containers.StartOptions).WithRecursive(true))
 		if err != nil {
 			report.ExitCode = define.ExitCode(err)
+			if opts.Rm {
+				if rmErr := removeContainer(con.ID, true); rmErr != nil && !errors.Is(rmErr, define.ErrNoSuchCtr) {
+					logrus.Errorf("Container %s failed to be removed", con.ID)
+				}
+			}
 		}
 		return &report, err
 	}
@@ -796,10 +810,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 
 		report.ExitCode = define.ExitCode(err)
 		if opts.Rm {
-			reports, rmErr := containers.Remove(ic.ClientCtx, con.ID, new(containers.RemoveOptions).WithForce(false).WithVolumes(true))
-			if rmErr != nil || reports[0].Err != nil {
-				logrus.Debugf("unable to remove container %s after failing to start and attach to it", con.ID)
-			}
+			_ = removeContainer(con.ID, false)
 		}
 		return &report, err
 	}
@@ -815,8 +826,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 			}
 
 			if !shouldRestart {
-				reports, err := containers.Remove(ic.ClientCtx, con.ID, new(containers.RemoveOptions).WithForce(false).WithVolumes(true))
-				logIfRmError(con.ID, err, reports)
+				_ = removeContainer(con.ID, false)
 			}
 		}()
 	}

diff --git a/test/system/030-run.bats b/test/system/030-run.bats
@@ -853,4 +853,20 @@ EOF
     run_podman rm $output
 }
 
+@test "podman run failed --rm " {
+    port=$(random_free_port)
+
+    # Run two containers with the same port bindings. The second must fail
+    run_podman     run -p $port:80 --rm -d --name c_ok           $IMAGE top
+    run_podman 126 run -p $port:80      -d --name c_fail_no_rm   $IMAGE top
+    run_podman 126 run -p $port:80 --rm -d --name c_fail_with_rm $IMAGE top
+    # Prior to #15060, the third container would still show up in ps -a
+    run_podman ps -a --sort names --format '{{.Image}}--{{.Names}}'
+    is "$output" "$IMAGE--c_fail_no_rm
+$IMAGE--c_ok" \
+       "podman ps -a shows running & failed containers, but not failed-with-rm"
+
+    run_podman container rm -f -t 0 c_ok c_fail_no_rm
+}
+
 # vim: filetype=sh