[v5.2-rhel] Fix podman stop and podman run --rmi

This started off as an attempt to make `podman stop` on a
container started with `--rm` actually remove the container,
instead of just cleaning it up and waiting for the cleanup
process to finish the removal.

In the process, I realized that `podman run --rmi` was rather
broken. It was only done as part of the Podman CLI, not the
cleanup process (meaning it only worked with attached containers)
and the way it was wired meant that I was fairly confident that
it wouldn't work if I did a `podman stop` on an attached
container run with `--rmi`. I rewired it to use the same
mechanism that `podman run --rm` uses, so it should be a lot more
durable now, and I also wired it into `podman inspect` so you can
tell that a container will remove its image.

Tests have been added for the changes to `podman run --rmi`. No
tests for `stop` on a `run --rm` container as that would be racy.

Fixes containers#22852
Fixes RHEL-39513

For this branch
Backport of: [458ba5a](containers@458ba5a)
Fixes: https://issues.redhat.com/browse/RHEL-61667

Signed-off-by: Matt Heon <[email protected]>
Signed-off-by: tomsweeneyredhat <[email protected]>

cmd/podman/containers/run.go

@@ -211,6 +211,11 @@ func run(cmd *cobra.Command, args []string) error {
 	s.ImageArch = cliVals.Arch
 	s.ImageVariant = cliVals.Variant
 	s.Passwd = &runOpts.Passwd
+
+	if runRmi {
+		s.RemoveImage = &runRmi
+	}
+
 	runOpts.Spec = s
 
 	if err := createPodIfNecessary(cmd, s, cliVals.Net); err != nil {

libpod/container.go

@@ -1261,6 +1261,17 @@ func (c *Container) AutoRemove() bool {
 	return spec.Annotations[define.InspectAnnotationAutoremove] == define.InspectResponseTrue
 }
 
+// AutoRemoveImage indicates that the container will automatically remove the
+// image it is using after it exits and is removed.
+// Only allowed if AutoRemove is true.
+func (c *Container) AutoRemoveImage() bool {
+	spec := c.config.Spec
+	if spec.Annotations == nil {
+		return false
+	}
+	return spec.Annotations[define.InspectAnnotationAutoremoveImage] == define.InspectResponseTrue
+}
+
 // Timezone returns the timezone configured inside the container.
 // Local means it has the same timezone as the host machine
 func (c *Container) Timezone() string {

libpod/container_inspect.go

@@ -517,6 +517,9 @@ func (c *Container) generateInspectContainerHostConfig(ctrSpec *spec.Spec, named
 		if ctrSpec.Annotations[define.InspectAnnotationAutoremove] == define.InspectResponseTrue {
 			hostConfig.AutoRemove = true
 		}
+		if ctrSpec.Annotations[define.InspectAnnotationAutoremoveImage] == define.InspectResponseTrue {
+			hostConfig.AutoRemoveImage = true
+		}
 		if ctrs, ok := ctrSpec.Annotations[define.VolumesFromAnnotation]; ok {
 			hostConfig.VolumesFrom = strings.Split(ctrs, ";")
 		}

libpod/container_validate.go

@@ -158,6 +158,18 @@ func (c *Container) validate() error {
 		}
 	}
 
+	// Autoremoving image requires autoremoving the associated container
+	if c.config.Spec.Annotations != nil {
+		if c.config.Spec.Annotations[define.InspectAnnotationAutoremoveImage] == define.InspectResponseTrue {
+			if c.config.Spec.Annotations[define.InspectAnnotationAutoremove] != define.InspectResponseTrue {
+				return fmt.Errorf("autoremoving image requires autoremoving the container: %w", define.ErrInvalidArg)
+			}
+			if c.config.Rootfs != "" {
+				return fmt.Errorf("autoremoving image is not possible when a rootfs is in use: %w", define.ErrInvalidArg)
+			}
+		}
+	}
+
 	// Cannot set startup HC without a healthcheck
 	if c.config.HealthCheckConfig == nil && c.config.StartupHealthCheckConfig != nil {
 		return fmt.Errorf("cannot set a startup healthcheck when there is no regular healthcheck: %w", define.ErrInvalidArg)

libpod/define/annotations.go

@@ -18,6 +18,12 @@ const (
 	// the two supported boolean values (InspectResponseTrue and
 	// InspectResponseFalse) it will be used in the output of Inspect().
 	InspectAnnotationAutoremove = "io.podman.annotations.autoremove"
+	// InspectAnnotationAutoremoveImage is used by Inspect to identify
+	// containers which will automatically remove the image used by the
+	// container. If an annotation with this key is found in the OCI spec and
+	// is one of the two supported boolean values (InspectResponseTrue and
+	// InspectResponseFalse) it will be used in the output of Inspect().
+	InspectAnnotationAutoremoveImage = "io.podman.annotations.autoremove-image"
 	// InspectAnnotationPrivileged is used by Inspect to identify containers
 	// which are privileged (IE, running with elevated privileges).
 	// It is expected to be a boolean, populated by one of

libpod/define/container_inspect.go

@@ -390,6 +390,11 @@ type InspectContainerHostConfig struct {
 	// It is not handled directly within libpod and is stored in an
 	// annotation.
 	AutoRemove bool `json:"AutoRemove"`
+	// AutoRemoveImage is whether the container's image will be
+	// automatically removed on exiting.
+	// It is not handled directly within libpod and is stored in an
+	// annotation.
+	AutoRemoveImage bool `json:"AutoRemoveImage"`
 	// Annotations are provided to the runtime when the container is
 	// started.
 	Annotations map[string]string `json:"Annotations"`

libpod/oci_conmon_common.go

@@ -1116,7 +1116,7 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
 		args = append(args, "--no-pivot")
 	}
 
-	exitCommand, err := specgenutil.CreateExitCommandArgs(ctr.runtime.storageConfig, ctr.runtime.config, ctr.runtime.syslog || logrus.IsLevelEnabled(logrus.DebugLevel), ctr.AutoRemove(), false)
+	exitCommand, err := specgenutil.CreateExitCommandArgs(ctr.runtime.storageConfig, ctr.runtime.config, ctr.runtime.syslog || logrus.IsLevelEnabled(logrus.DebugLevel), ctr.AutoRemove(), ctr.AutoRemoveImage(), false)
 	if err != nil {
 		return 0, err
 	}

pkg/api/handlers/compat/exec.go

@@ -74,7 +74,7 @@ func ExecCreateHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	// Automatically log to syslog if the server has log-level=debug set
-	exitCommandArgs, err := specgenutil.CreateExitCommandArgs(storageConfig, runtimeConfig, logrus.IsLevelEnabled(logrus.DebugLevel), true, true)
+	exitCommandArgs, err := specgenutil.CreateExitCommandArgs(storageConfig, runtimeConfig, logrus.IsLevelEnabled(logrus.DebugLevel), true, false, true)
 	if err != nil {
 		utils.InternalServerError(w, err)
 		return

pkg/domain/infra/abi/containers.go

@@ -35,6 +35,7 @@ import (
 	"github.com/containers/podman/v5/pkg/specgenutil"
 	"github.com/containers/podman/v5/pkg/util"
 	"github.com/containers/storage"
+	"github.com/hashicorp/go-multierror"
 	"github.com/sirupsen/logrus"
 )
 
@@ -290,15 +291,35 @@ func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []strin
 				return err
 			}
 		}
-		err = c.Cleanup(ctx)
-		if err != nil {
-			// Issue #7384 and #11384: If the container is configured for
-			// auto-removal, it might already have been removed at this point.
-			// We still need to clean up since we do not know if the other cleanup process is successful
-			if c.AutoRemove() && (errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved)) {
-				return nil
+		if c.AutoRemove() {
+			_, imageName := c.Image()
+
+			if err := ic.Libpod.RemoveContainer(ctx, c, false, true, nil); err != nil {
+				// Issue #7384 and #11384: If the container is configured for
+				// auto-removal, it might already have been removed at this point.
+				// We still need to clean up since we do not know if the other cleanup process is successful
+				if !(errors.Is(err, define.ErrNoSuchCtr) || errors.Is(err, define.ErrCtrRemoved)) {
+					return err
+				}
+			}
+
+			if c.AutoRemoveImage() {
+				imageEngine := ImageEngine{Libpod: ic.Libpod}
+				_, rmErrors := imageEngine.Remove(ctx, []string{imageName}, entities.ImageRemoveOptions{Ignore: true})
+				if len(rmErrors) > 0 {
+					mErr := multierror.Append(nil, rmErrors...)
+					return fmt.Errorf("removing container %s image %s: %w", c.ID(), imageName, mErr)
+				}
+			}
+		} else {
+			if err = c.Cleanup(ctx); 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) {
+					return nil
+				}
+				return err
 			}
-			return err
 		}
 		return nil
 	})
@@ -826,7 +847,7 @@ func makeExecConfig(options entities.ExecOptions, rt *libpod.Runtime) (*libpod.E
 		return nil, fmt.Errorf("retrieving Libpod configuration to build exec exit command: %w", err)
 	}
 	// TODO: Add some ability to toggle syslog
-	exitCommandArgs, err := specgenutil.CreateExitCommandArgs(storageConfig, runtimeConfig, logrus.IsLevelEnabled(logrus.DebugLevel), false, true)
+	exitCommandArgs, err := specgenutil.CreateExitCommandArgs(storageConfig, runtimeConfig, logrus.IsLevelEnabled(logrus.DebugLevel), false, false, true)
 	if err != nil {
 		return nil, fmt.Errorf("constructing exit command for exec session: %w", err)
 	}

pkg/specgen/generate/oci_linux.go

@@ -318,6 +318,10 @@ func SpecGenToOCI(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runt
 		configSpec.Annotations[define.InspectAnnotationAutoremove] = define.InspectResponseTrue
 	}
 
+	if s.RemoveImage != nil && *s.RemoveImage {
+		configSpec.Annotations[define.InspectAnnotationAutoremoveImage] = define.InspectResponseTrue
+	}
+
 	if len(s.VolumesFrom) > 0 {
 		configSpec.Annotations[define.VolumesFromAnnotation] = strings.Join(s.VolumesFrom, ";")
 	}

pkg/specgen/specgen.go

@@ -159,6 +159,12 @@ type ContainerBasicConfig struct {
 	// and exits.
 	// Optional.
 	Remove *bool `json:"remove,omitempty"`
+	// RemoveImage indicates that the container should remove the image it
+	// was created from after it exits.
+	// Only allowed if Remove is set to true and Image, not Rootfs, is in
+	// use.
+	// Optional.
+	RemoveImage *bool `json:"removeImage,omitempty"`
 	// ContainerCreateCommand is the command that was used to create this
 	// container.
 	// This will be shown in the output of Inspect() on the container, and

pkg/specgenutil/util.go

@@ -261,7 +261,7 @@ func parseAndValidatePort(port string) (uint16, error) {
 	return uint16(num), nil
 }
 
-func CreateExitCommandArgs(storageConfig storageTypes.StoreOptions, config *config.Config, syslog, rm, exec bool) ([]string, error) {
+func CreateExitCommandArgs(storageConfig storageTypes.StoreOptions, config *config.Config, syslog, rm, rmi, exec bool) ([]string, error) {
 	// We need a cleanup process for containers in the current model.
 	// But we can't assume that the caller is Podman - it could be another
 	// user of the API.
@@ -317,6 +317,10 @@ func CreateExitCommandArgs(storageConfig storageTypes.StoreOptions, config *conf
 		command = append(command, "--rm")
 	}
 
+	if rmi {
+		command = append(command, "--rmi")
+	}
+
 	// This has to be absolutely last, to ensure that the exec session ID
 	// will be added after it by Libpod.
 	if exec {

test/system/030-run.bats

@@ -209,6 +209,27 @@ echo $rand        |   0 | $rand
 
     run_podman 125 run --rmi --rm=false $NONLOCAL_IMAGE /bin/true
     is "$output" "Error: the --rmi option does not work without --rm" "--rmi should refuse to remove images when --rm=false set by user"
+
+    # Try again with a detached container and verify it works
+    cname=c_$(safename)
+    run_podman run -d --name $cname --rmi $NONLOCAL_IMAGE /bin/true
+    # 10 chances for the image to disappear
+    for i in `seq 1 10`; do
+        sleep 0.5
+        run_podman '?' image exists $NONLOCAL_IMAGE
+        if [[ $status == 1 ]]; then
+           break
+        fi
+    done
+    # Final check will error if image still exists
+    run_podman 1 image exists $NONLOCAL_IMAGE
+
+    # Verify that the inspect annotation is set correctly
+    run_podman run -d --name $cname --rmi $NONLOCAL_IMAGE sleep 10
+    run_podman inspect --format '{{ .HostConfig.AutoRemoveImage }} {{ .HostConfig.AutoRemove }}' $cname
+    is "$output" "true true" "Inspect correctly shows image autoremove and normal autoremove"
+    run_podman stop -t0 $cname
+    run_podman 1 image exists $NONLOCAL_IMAGE
 }
 
 # 'run --conmon-pidfile --cid-file' makes sure we don't regress on these flags.