Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix podman stop and podman run --rmi #23670

Merged
merged 1 commit into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmd/podman/containers/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 11 additions & 0 deletions libpod/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions libpod/container_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, ";")
}
Expand Down
12 changes: 12 additions & 0 deletions libpod/container_validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 6 additions & 0 deletions libpod/define/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions libpod/define/container_inspect.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
2 changes: 1 addition & 1 deletion libpod/oci_conmon_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/handlers/compat/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 30 additions & 9 deletions pkg/domain/infra/abi/containers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like if you specify AutoRemoteImage, we are no longer cleaning up the containers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the image implies removing the container, and removing the container also unmounts, cleans up the network, etc, so we should be OK.

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
})
Expand Down Expand Up @@ -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)
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/specgen/generate/oci_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, ";")
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/specgen/specgen.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion pkg/specgenutil/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
21 changes: 21 additions & 0 deletions test/system/030-run.bats
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down