From 458ba5a8afedae15acfec0a1f64195f41d80edcf Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Mon, 19 Aug 2024 11:20:05 -0400 Subject: [PATCH] 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 #22852 Fixes RHEL-39513 Signed-off-by: Matt Heon --- cmd/podman/containers/run.go | 5 ++++ libpod/container.go | 11 +++++++++ libpod/container_inspect.go | 3 +++ libpod/container_validate.go | 12 +++++++++ libpod/define/annotations.go | 6 +++++ libpod/define/container_inspect.go | 5 ++++ libpod/oci_conmon_common.go | 2 +- pkg/api/handlers/compat/exec.go | 2 +- pkg/domain/infra/abi/containers.go | 39 +++++++++++++++++++++++------- pkg/specgen/generate/oci_linux.go | 4 +++ pkg/specgen/specgen.go | 6 +++++ pkg/specgenutil/util.go | 6 ++++- test/system/030-run.bats | 21 ++++++++++++++++ 13 files changed, 110 insertions(+), 12 deletions(-) diff --git a/cmd/podman/containers/run.go b/cmd/podman/containers/run.go index be688c21f5..d50e787be6 100644 --- a/cmd/podman/containers/run.go +++ b/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 { diff --git a/libpod/container.go b/libpod/container.go index e424afa9f6..4234430726 100644 --- a/libpod/container.go +++ b/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 { diff --git a/libpod/container_inspect.go b/libpod/container_inspect.go index ef4bac14e4..aa561a5cdc 100644 --- a/libpod/container_inspect.go +++ b/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, ";") } diff --git a/libpod/container_validate.go b/libpod/container_validate.go index 0c2b604dd0..a98545c91f 100644 --- a/libpod/container_validate.go +++ b/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) diff --git a/libpod/define/annotations.go b/libpod/define/annotations.go index ac1956f56b..e1da8a157e 100644 --- a/libpod/define/annotations.go +++ b/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 diff --git a/libpod/define/container_inspect.go b/libpod/define/container_inspect.go index 240b09d21d..89b70e59e6 100644 --- a/libpod/define/container_inspect.go +++ b/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"` diff --git a/libpod/oci_conmon_common.go b/libpod/oci_conmon_common.go index 39a2a0d626..9a9607305c 100644 --- a/libpod/oci_conmon_common.go +++ b/libpod/oci_conmon_common.go @@ -1035,7 +1035,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 } diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index e7f29d727a..7c28458d57 100644 --- a/pkg/api/handlers/compat/exec.go +++ b/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 diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 47389e9fdc..1694d6ce79 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -36,6 +36,7 @@ import ( "github.com/containers/podman/v5/pkg/util" "github.com/containers/storage" "github.com/containers/storage/types" + "github.com/hashicorp/go-multierror" "github.com/sirupsen/logrus" ) @@ -291,15 +292,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 }) @@ -827,7 +848,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) } diff --git a/pkg/specgen/generate/oci_linux.go b/pkg/specgen/generate/oci_linux.go index d6247bbf67..d19cf9c03c 100644 --- a/pkg/specgen/generate/oci_linux.go +++ b/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, ";") } diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 00a208921a..1299daf3de 100644 --- a/pkg/specgen/specgen.go +++ b/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 diff --git a/pkg/specgenutil/util.go b/pkg/specgenutil/util.go index b5eb5d914c..71065997b0 100644 --- a/pkg/specgenutil/util.go +++ b/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 { diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 1aa079dab4..7dc0d9a1b0 100644 --- a/test/system/030-run.bats +++ b/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.