From a5766aa377bb237bad1b3d4cb7bba14aaa744b07 Mon Sep 17 00:00:00 2001 From: Matt Heon Date: Mon, 19 Aug 2024 11:20:05 -0400 Subject: [PATCH 1/2] [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 #22852 Fixes RHEL-39513 For this branch Backport of: [458ba5a](https://github.com/containers/podman/pull/23670/commits/458ba5a8afedae15acfec0a1f64195f41d80edcf) Fixes: https://issues.redhat.com/browse/RHEL-61667 Signed-off-by: Matt Heon Signed-off-by: tomsweeneyredhat --- 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 be688c21f50b..d50e787be6fd 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 e424afa9f6e1..423443072623 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 ef4bac14e4d4..aa561a5cdc9d 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 0c2b604dd0ad..a98545c91f62 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 ac1956f56b7a..e1da8a157eac 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 240b09d21d39..89b70e59e659 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 a3e5a24b0d57..e7a5a4049d56 100644 --- a/libpod/oci_conmon_common.go +++ b/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 } diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index e7f29d727a7c..7c28458d5713 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 3cbeb2cd6583..9006dc9e0bff 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/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) } diff --git a/pkg/specgen/generate/oci_linux.go b/pkg/specgen/generate/oci_linux.go index d6247bbf6771..d19cf9c03c84 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 00a208921a0f..1299daf3de7f 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 b5eb5d914c3c..71065997b09c 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 4080a1579231..f5eb351e8a9f 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. From 694f2a492eb14eeb54efb5da430d74f832aed699 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 27 Aug 2024 12:02:33 +0200 Subject: [PATCH 2/2] [v5.2-rhel] podman run: ignore image rm error Since commit 458ba5a8af the cleanup process now removes the image as well, thus the removal is racy and it will cause an error here. The code tried to ignore the error with errors.Is() but this never works across the remote API. However the API already has a ignore option so juts use that and fix the error message so that we can easily find the root cause and I do not have to guess where the log was written. Fixes #23719 Signed-off-by: Paul Holzinger (cherry picked from commit 8e78028e2c8d67963dadc5def5619ca69f858034) Signed-off-by: tomsweeneyredhat --- cmd/podman/containers/run.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/cmd/podman/containers/run.go b/cmd/podman/containers/run.go index d50e787be6fd..8abd08837e55 100644 --- a/cmd/podman/containers/run.go +++ b/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" @@ -243,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