Merge pull request containers#10942 from vrothberg/fix-10935

podman start: remove containers configured for auto removal
vrothberg · Jul 16, 2021 · 3ba9f2a · 3ba9f2a
2 parents f0cd16c + 9924c57
commit 3ba9f2a
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 28 deletions.
diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go
@@ -261,6 +261,24 @@ func (ic *ContainerEngine) ContainerRestart(ctx context.Context, namesOrIds []st
 	return reports, nil
 }
 
+func (ic *ContainerEngine) removeContainer(ctx context.Context, ctr *libpod.Container, options entities.RmOptions) error {
+	err := ic.Libpod.RemoveContainer(ctx, ctr, options.Force, options.Volumes)
+	if err == nil {
+		return nil
+	}
+	logrus.Debugf("Failed to remove container %s: %s", ctr.ID(), err.Error())
+	switch errors.Cause(err) {
+	case define.ErrNoSuchCtr:
+		if options.Ignore {
+			logrus.Debugf("Ignoring error (--allow-missing): %v", err)
+			return nil
+		}
+	case define.ErrCtrRemoved:
+		return nil
+	}
+	return err
+}
+
 func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string, options entities.RmOptions) ([]*entities.RmReport, error) {
 	reports := []*entities.RmReport{}
 
@@ -318,21 +336,7 @@ func (ic *ContainerEngine) ContainerRm(ctx context.Context, namesOrIds []string,
 	}
 
 	errMap, err := parallelctr.ContainerOp(ctx, ctrs, func(c *libpod.Container) error {
-		err := ic.Libpod.RemoveContainer(ctx, c, options.Force, options.Volumes)
-		if err == nil {
-			return nil
-		}
-		logrus.Debugf("Failed to remove container %s: %s", c.ID(), err.Error())
-		switch errors.Cause(err) {
-		case define.ErrNoSuchCtr:
-			if options.Ignore {
-				logrus.Debugf("Ignoring error (--allow-missing): %v", err)
-				return nil
-			}
-		case define.ErrCtrRemoved:
-			return nil
-		}
-		return err
+		return ic.removeContainer(ctx, c, options)
 	})
 	if err != nil {
 		return nil, err
@@ -791,6 +795,11 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 					Err:      err,
 					ExitCode: exitCode,
 				})
+				if ctr.AutoRemove() {
+					if err := ic.removeContainer(ctx, ctr, entities.RmOptions{}); err != nil {
+						logrus.Errorf("Error removing container %s: %v", ctr.ID(), err)
+					}
+				}
 				return reports, errors.Wrapf(err, "unable to start container %s", ctr.ID())
 			}
 
@@ -827,9 +836,6 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 				ExitCode: 125,
 			}
 			if err := ctr.Start(ctx, true); err != nil {
-				// if lastError != nil {
-				//	fmt.Fprintln(os.Stderr, lastError)
-				// }
 				report.Err = err
 				if errors.Cause(err) == define.ErrWillDeadlock {
 					report.Err = errors.Wrapf(err, "please run 'podman system renumber' to resolve deadlocks")
@@ -838,6 +844,11 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 				}
 				report.Err = errors.Wrapf(err, "unable to start container %q", ctr.ID())
 				reports = append(reports, report)
+				if ctr.AutoRemove() {
+					if err := ic.removeContainer(ctx, ctr, entities.RmOptions{}); err != nil {
+						logrus.Errorf("Error removing container %s: %v", ctr.ID(), err)
+					}
+				}
 				continue
 			}
 			report.ExitCode = 0

diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go
@@ -541,6 +541,17 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 		return nil, err
 	}
 	removeOptions := new(containers.RemoveOptions).WithVolumes(true).WithForce(false)
+	removeContainer := func(id string) {
+		if err := containers.Remove(ic.ClientCtx, id, removeOptions); err != nil {
+			if errorhandling.Contains(err, define.ErrNoSuchCtr) ||
+				errorhandling.Contains(err, define.ErrCtrRemoved) {
+				logrus.Debugf("Container %s does not exist: %v", id, err)
+			} else {
+				logrus.Errorf("Error removing container %s: %v", id, err)
+			}
+		}
+	}
+
 	// There can only be one container if attach was used
 	for i, ctr := range ctrs {
 		name := ctr.ID
@@ -568,6 +579,9 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 			}
 
 			if err != nil {
+				if ctr.AutoRemove {
+					removeContainer(ctr.ID)
+				}
 				report.ExitCode = define.ExitCode(report.Err)
 				report.Err = err
 				reports = append(reports, &report)
@@ -582,16 +596,10 @@ func (ic *ContainerEngine) ContainerStart(ctx context.Context, namesOrIds []stri
 						logrus.Errorf("Failed to check if %s should restart: %v", ctr.ID, err)
 						return
 					}
+					logrus.Errorf("Should restart: %v", shouldRestart)
 
-					if !shouldRestart {
-						if err := containers.Remove(ic.ClientCtx, ctr.ID, removeOptions); err != nil {
-							if errorhandling.Contains(err, define.ErrNoSuchCtr) ||
-								errorhandling.Contains(err, define.ErrCtrRemoved) {
-								logrus.Debugf("Container %s does not exist: %v", ctr.ID, err)
-							} else {
-								logrus.Errorf("Error removing container %s: %v", ctr.ID, err)
-							}
-						}
+					if !shouldRestart && ctr.AutoRemove {
+						removeContainer(ctr.ID)
 					}
 				}()
 			}

diff --git a/test/e2e/start_test.go b/test/e2e/start_test.go
@@ -65,7 +65,6 @@ var _ = Describe("Podman start", func() {
 	})
 
 	It("podman start --rm --attach removed on failure", func() {
-		Skip("FIXME: #10935, race condition removing container")
 		session := podmanTest.Podman([]string{"create", "--rm", ALPINE, "foo"})
 		session.WaitWithDefaultTimeout()
 		Expect(session).Should(Exit(0))