Merge pull request #24197 from TomSweeneyRedHat/dev/tsweeney/clean_9.5_0day

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

cmd/podman/containers/run.go

@@ -16,7 +16,6 @@ import (
 	"github.com/containers/podman/v5/pkg/rootless"
 	"github.com/containers/podman/v5/pkg/specgen"
 	"github.com/containers/podman/v5/pkg/specgenutil"
-	"github.com/containers/storage/types"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"golang.org/x/term"
@@ -211,6 +210,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 {
@@ -238,13 +242,9 @@ func run(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 	if runRmi {
-		_, rmErrors := registry.ImageEngine().Remove(registry.GetContext(), []string{imageName}, entities.ImageRemoveOptions{})
+		_, rmErrors := registry.ImageEngine().Remove(registry.GetContext(), []string{imageName}, entities.ImageRemoveOptions{Ignore: true})
 		for _, err := range rmErrors {
-			// ImageUnknown would be a super-unlikely race
-			if !errors.Is(err, types.ErrImageUnknown) {
-				// Typical case: ErrImageUsedByContainer
-				logrus.Warn(err)
-			}
+			logrus.Warnf("Failed to remove image: %v", err)
 		}
 	}
 	return 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.