Merge pull request #15060 from rhatdan/rm1

With --rm option remove container if podman run fails

3 files changed, 50 insertions, 12 deletions

diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
index ab742fb35..783224e9c 100644
--- a/pkg/domain/infra/abi/containers.go
+++ b/pkg/domain/infra/abi/containers.go
@@ -1061,6 +1061,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
@@ -1081,6 +1090,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
 		}
 	}
@@ -1098,6 +1109,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
 		}
 
@@ -1114,10 +1130,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)
@@ -1129,8 +1142,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
index 272c23268..98c73c51a 100644
--- a/pkg/domain/infra/tunnel/containers.go
+++ b/pkg/domain/infra/tunnel/containers.go
@@ -787,8 +787,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
 		}
 	}
@@ -800,6 +809,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
 	}
@@ -812,10 +826,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
 	}
@@ -831,8 +842,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
index d028a5ac7..908c169ee 100644
--- a/test/system/030-run.bats
+++ b/test/system/030-run.bats
@@ -871,4 +871,20 @@ EOF
     run_podman container rm -fa
 }
 
+@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