cleanup: add new --stopped-only option

The podman container cleanup process runs asynchronous and by the time
it gets the lock it is possible another podman process already did the
cleanup and then did a new init() to start it again. If the cleanup
process gets the lock there it will cause very weird things.

This can be observed in the remote start API as CI flakes.

Fixes #23754

Signed-off-by: Paul Holzinger <[email protected]>

cmd/podman/containers/cleanup.go

@@ -53,6 +53,11 @@ func init() {
 
 	flags.BoolVar(&cleanupOptions.Remove, "rm", false, "After cleanup, remove the container entirely")
 	flags.BoolVar(&cleanupOptions.RemoveImage, "rmi", false, "After cleanup, remove the image entirely")
+
+	stoppedOnlyFlag := "stopped-only"
+	flags.BoolVar(&cleanupOptions.StoppedOnly, stoppedOnlyFlag, false, "Only cleanup when the container is in the stopped state")
+	_ = flags.MarkHidden(stoppedOnlyFlag)
+
 	validate.AddLatestFlag(cleanupCommand, &cleanupOptions.Latest)
 }
 

libpod/container_api.go

@@ -785,8 +785,10 @@ func (c *Container) WaitForConditionWithInterval(ctx context.Context, waitTimeou
 }
 
 // Cleanup unmounts all mount points in container and cleans up container storage
-// It also cleans up the network stack
-func (c *Container) Cleanup(ctx context.Context) error {
+// It also cleans up the network stack.
+// onlyStopped is set by the podman container cleanup to ensure we only cleanup a stopped container,
+// all other states mean another process already called cleanup before us which is fine in such cases.
+func (c *Container) Cleanup(ctx context.Context, onlyStopped bool) error {
 	if !c.batched {
 		c.lock.Lock()
 		defer c.lock.Unlock()
@@ -808,6 +810,9 @@ func (c *Container) Cleanup(ctx context.Context) error {
 	if !c.ensureState(define.ContainerStateConfigured, define.ContainerStateCreated, define.ContainerStateStopped, define.ContainerStateStopping, define.ContainerStateExited) {
 		return fmt.Errorf("container %s is running or paused, refusing to clean up: %w", c.ID(), define.ErrCtrStateInvalid)
 	}
+	if onlyStopped && !c.ensureState(define.ContainerStateStopped) {
+		return fmt.Errorf("container %s is not stopped and only cleanup for a stopped container was requested: %w", c.ID(), define.ErrCtrStateInvalid)
+	}
 
 	// if the container was not created in the oci runtime or was already cleaned up, then do nothing
 	if c.ensureState(define.ContainerStateConfigured, define.ContainerStateExited) {

libpod/pod_api.go

@@ -180,7 +180,7 @@ func (p *Pod) stopWithTimeout(ctx context.Context, cleanup bool, timeout int) (m
 			}
 
 			if cleanup {
-				err := c.Cleanup(ctx)
+				err := c.Cleanup(ctx, false)
 				if err != nil && !errors.Is(err, define.ErrCtrStateInvalid) && !errors.Is(err, define.ErrCtrStopped) {
 					return err
 				}
@@ -299,7 +299,7 @@ func (p *Pod) Cleanup(ctx context.Context) (map[string]error, error) {
 		c := ctr
 		logrus.Debugf("Adding parallel job to clean up container %s", c.ID())
 		retChan := parallel.Enqueue(ctx, func() error {
-			return c.Cleanup(ctx)
+			return c.Cleanup(ctx, false)
 		})
 
 		ctrErrChan[c.ID()] = retChan

pkg/domain/entities/containers.go

@@ -371,6 +371,7 @@ type ContainerCleanupOptions struct {
 	Latest      bool
 	Remove      bool
 	RemoveImage bool
+	StoppedOnly bool // Only cleanup if the container is stopped, i.e. was running before
 }
 
 // ContainerCleanupReport describes the response from a

pkg/domain/infra/abi/containers.go

@@ -313,7 +313,7 @@ func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []strin
 				}
 			}
 		} else {
-			if err = c.Cleanup(ctx); err != nil {
+			if err = c.Cleanup(ctx, false); err != nil {
 				// The container could still have been removed, as we unlocked
 				// after we stopped it.
 				if errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved) {
@@ -1320,7 +1320,7 @@ func (ic *ContainerEngine) ContainerCleanup(ctx context.Context, namesOrIds []st
 				report.RmErr = fmt.Errorf("failed to clean up and remove container %v: %w", ctr.ID(), err)
 			}
 		} else {
-			err := ctr.Cleanup(ctx)
+			err := ctr.Cleanup(ctx, options.StoppedOnly)
 			// ignore error if ctr is removed or cannot be cleaned up, likely the ctr was already restarted by another process
 			if err != nil && !errors.Is(err, define.ErrNoSuchCtr) && !errors.Is(err, define.ErrCtrStateInvalid) {
 				report.CleanErr = fmt.Errorf("failed to clean up container %v: %w", ctr.ID(), err)

pkg/specgenutil/util.go

@@ -311,7 +311,9 @@ func CreateExitCommandArgs(storageConfig storageTypes.StoreOptions, config *conf
 		command = append(command, "--module", module)
 	}
 
-	command = append(command, []string{"container", "cleanup"}...)
+	// --stopped-only is used to ensure we only cleanup stopped containers and do not race
+	// against other processes that did a cleanup() + init() again before we had the chance to run
+	command = append(command, []string{"container", "cleanup", "--stopped-only"}...)
 
 	if rm {
 		command = append(command, "--rm")