From 7326b862e3041bf38fbb798b4ee0994bc99034ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Rod=C3=A1k?= Date: Thu, 5 Mar 2026 18:46:07 +0100 Subject: [PATCH] Fix `unless-stopped` containers not restarting after `podman-restart.service` stop them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `podman-restart.service` stops containers, it marks them as "stopped by user" which breaks the `unless-stopped` restart policy. Add hidden `--not-stopped-by-user` flag to prevent this, allowing `unless-stopped` containers to restart on next boot. Fixes: https://github.com/containers/podman/issues/28152 Fixes: https://issues.redhat.com/browse/RUN-4357 Signed-off-by: Jan Rodák --- cmd/podman/containers/stop.go | 23 +++++++++-- .../systemd/system/podman-restart.service.in | 2 +- libpod/container_api.go | 21 +++++++++- libpod/container_internal.go | 24 +++++++++++- pkg/domain/entities/engine_container.go | 1 + pkg/domain/infra/abi/containers.go | 24 ++++++++---- pkg/domain/infra/tunnel/containers.go | 4 ++ test/e2e/ps_test.go | 39 +++++++++++++++++++ test/e2e/stop_test.go | 23 +++++++++++ 9 files changed, 146 insertions(+), 15 deletions(-) diff --git a/cmd/podman/containers/stop.go b/cmd/podman/containers/stop.go index 957044a0d2..2c87eb5de3 100644 --- a/cmd/podman/containers/stop.go +++ b/cmd/podman/containers/stop.go @@ -51,8 +51,9 @@ var ( stopOptions = entities.StopOptions{ Filters: make(map[string][]string), } - stopCidFiles = []string{} - stopTimeout int + stopCidFiles = []string{} + stopTimeout int + stopAsService bool ) func stopFlags(cmd *cobra.Command) { @@ -73,6 +74,10 @@ func stopFlags(cmd *cobra.Command) { flags.StringArrayVarP(&filters, filterFlagName, "f", []string{}, "Filter output based on conditions given") _ = cmd.RegisterFlagCompletionFunc(filterFlagName, common.AutocompletePsFilters) + serviceFlagName := "service" + flags.BoolVar(&stopAsService, serviceFlagName, false, "Stop as service (do not mark as stopped by user)") + _ = flags.MarkHidden(serviceFlagName) + if registry.IsRemote() { _ = flags.MarkHidden("cidfile") _ = flags.MarkHidden("ignore") @@ -97,6 +102,10 @@ func init() { } func stop(cmd *cobra.Command, args []string) error { + if registry.IsRemote() && stopAsService { + return fmt.Errorf("--service is not supported on remote connections") + } + var errs utils.OutputErrors args = utils.RemoveSlash(args) @@ -124,7 +133,15 @@ func stop(cmd *cobra.Command, args []string) error { stopOptions.Filters[fname] = append(stopOptions.Filters[fname], filter) } - responses, err := registry.ContainerEngine().ContainerStop(context.Background(), args, stopOptions) + var ( + responses []*entities.StopReport + err error + ) + if stopAsService { + responses, err = registry.ContainerEngine().ContainerStopService(context.Background(), args, stopOptions) + } else { + responses, err = registry.ContainerEngine().ContainerStop(context.Background(), args, stopOptions) + } if err != nil { return err } diff --git a/contrib/systemd/system/podman-restart.service.in b/contrib/systemd/system/podman-restart.service.in index d347ac9f53..2b52b452f8 100644 --- a/contrib/systemd/system/podman-restart.service.in +++ b/contrib/systemd/system/podman-restart.service.in @@ -10,7 +10,7 @@ Type=oneshot RemainAfterExit=true Environment=LOGGING="--log-level=info" ExecStart=@@PODMAN@@ $LOGGING start --all --filter should-start-on-boot=true -ExecStop=@@PODMAN@@ $LOGGING stop --all --filter should-start-on-boot=true +ExecStop=@@PODMAN@@ $LOGGING stop --service --all [Install] WantedBy=default.target diff --git a/libpod/container_api.go b/libpod/container_api.go index 2ef7269d7e..b25a45d1b2 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -283,6 +283,24 @@ func (c *Container) Stop() error { // manually. If timeout is 0, SIGKILL will be used immediately to kill the // container. func (c *Container) StopWithTimeout(timeout uint) (finalErr error) { + return c.StopWithArgs(timeout, true) +} + +// StopService stops the container without marking it as stopped by user (e.g. for +// systemd ExecStop). Containers with restart policy unless-stopped will be +// eligible to start again on next boot. +func (c *Container) StopService(timeout uint) (finalErr error) { + return c.StopWithArgs(timeout, false) +} + +// StopWithArgs is a version of Stop that allows a timeout to be specified manually +// and controls whether to set the StoppedByUser state field. If timeout is 0, +// SIGKILL will be used immediately to kill the container. +// +// An explicit stop is treated as a user-driven lifecycle action. Because of +// that, this path may not trigger automatic restart-policy handling in cleanup, +// even when stoppedByUser is false. +func (c *Container) StopWithArgs(timeout uint, stoppedByUser bool) (finalErr error) { // Have to lock the pod the container is a part of. // This prevents running `podman stop` at the same time a // `podman pod start` is running, which could lead to weird races. @@ -320,8 +338,7 @@ func (c *Container) StopWithTimeout(timeout uint) (finalErr error) { return err } } - - return c.stop(timeout) + return c.stopInternal(timeout, stoppedByUser) } // Kill sends a signal to a container diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 3768550316..bb96e512f4 100644 --- a/libpod/container_internal.go +++ b/libpod/container_internal.go @@ -373,7 +373,10 @@ func (c *Container) syncContainer() error { // Only save back to DB if state changed if c.state.State != oldState { - // Check for a restart policy match + // Mark restart-policy match only for runtime-observed exits from + // Running/Paused into Stopped/Exited when the container was not + // explicitly stopped by the user. Explicit stopInternal() paths set + // state to Stopping first, so they typically do not satisfy this. if c.config.RestartPolicy != define.RestartPolicyNone && c.config.RestartPolicy != define.RestartPolicyNo && (oldState == define.ContainerStateRunning || oldState == define.ContainerStatePaused) && (c.state.State == define.ContainerStateStopped || c.state.State == define.ContainerStateExited) && @@ -1373,6 +1376,16 @@ func (c *Container) stopWithAll() bool { // Internal, non-locking function to stop container func (c *Container) stop(timeout uint) error { + return c.stopInternal(timeout, true) +} + +// Internal, non-locking function to stop container +// stoppedByUser controls whether to set the StoppedByUser state field. +func (c *Container) stopInternal(timeout uint, stoppedByUser bool) error { + // This is explicit container stop that flows pass through Running -> Stopping -> Stopped/Exited states. + // As a result, this does not satisfy the Running/Paused -> Stopped/Exited + // transition that is required to trigger restart policy during cleanup. + logrus.Debugf("Stopping ctr %s (timeout %d)", c.ID(), timeout) all := c.stopWithAll() @@ -1390,13 +1403,20 @@ func (c *Container) stop(timeout uint) error { cannotStopErr = fmt.Errorf("can only stop created or running containers. %s is in state %s: %w", c.ID(), c.state.State.String(), define.ErrCtrStateInvalid) } - c.state.StoppedByUser = true + if stoppedByUser { + c.state.StoppedByUser = true + } + if cannotStopErr == nil { // Set the container state to "stopping" and unlock the container // before handing it over to conmon to unblock other commands. #8501 // demonstrates nicely that a high stop timeout will block even simple // commands such as `podman ps` from progressing if the container lock // is held when busy-waiting for the container to be stopped. + // + // This intermediate Stopping state also ensures an explicit stop path is + // distinguished from a runtime-observed Running/Paused -> Stopped/Exited + // transition when syncContainer() computes RestartPolicyMatch. c.state.State = define.ContainerStateStopping } if err := c.save(); err != nil { diff --git a/pkg/domain/entities/engine_container.go b/pkg/domain/entities/engine_container.go index 6b9c53e20f..140211dd65 100644 --- a/pkg/domain/entities/engine_container.go +++ b/pkg/domain/entities/engine_container.go @@ -50,6 +50,7 @@ type ContainerEngine interface { //nolint:interfacebloat ContainerStat(ctx context.Context, nameOrDir string, path string) (*ContainerStatReport, error) ContainerStats(ctx context.Context, namesOrIds []string, options ContainerStatsOptions) (chan ContainerStatsReport, error) ContainerStop(ctx context.Context, namesOrIds []string, options StopOptions) ([]*StopReport, error) + ContainerStopService(ctx context.Context, namesOrIds []string, options StopOptions) ([]*StopReport, error) ContainerTop(ctx context.Context, options TopOptions) (*StringSliceReport, error) ContainerUnmount(ctx context.Context, nameOrIDs []string, options ContainerUnmountOptions) ([]*ContainerUnmountReport, error) ContainerUnpause(ctx context.Context, namesOrIds []string, options PauseUnPauseOptions) ([]*PauseUnpauseReport, error) diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index b5e3c98498..0d06e132d7 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -294,7 +294,10 @@ func (ic *ContainerEngine) ContainerUnpause(_ context.Context, namesOrIds []stri return reports, nil } -func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []string, options entities.StopOptions) ([]*entities.StopReport, error) { +// containerStopRunner is the signature for stopping a single container (used to share logic between ContainerStop and ContainerStopService). +type containerStopRunner func(*libpod.Container, uint) error + +func (ic *ContainerEngine) containerStopImpl(ctx context.Context, namesOrIds []string, options entities.StopOptions, runStop containerStopRunner) ([]*entities.StopReport, error) { containers, err := getContainers(ic.Libpod, getContainersOptions{ all: options.All, @@ -318,13 +321,12 @@ func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []strin } errMap, err := parallelctr.ContainerOp(ctx, libpodContainers, func(c *libpod.Container) error { - var err error + timeout := c.StopTimeout() if options.Timeout != nil { - err = c.StopWithTimeout(*options.Timeout) - } else { - err = c.Stop() + timeout = *options.Timeout } - if err != nil { + + if err := runStop(c, timeout); err != nil { switch { case errors.Is(err, define.ErrCtrStopped): logrus.Debugf("Container %s is already stopped", c.ID()) @@ -359,7 +361,7 @@ func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []strin } } } else { - if err = c.Cleanup(ctx, false); err != nil { + if err := c.Cleanup(ctx, false); 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) { @@ -384,6 +386,14 @@ func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []strin return reports, nil } +func (ic *ContainerEngine) ContainerStop(ctx context.Context, namesOrIds []string, options entities.StopOptions) ([]*entities.StopReport, error) { + return ic.containerStopImpl(ctx, namesOrIds, options, func(c *libpod.Container, t uint) error { return c.StopWithTimeout(t) }) +} + +func (ic *ContainerEngine) ContainerStopService(ctx context.Context, namesOrIds []string, options entities.StopOptions) ([]*entities.StopReport, error) { + return ic.containerStopImpl(ctx, namesOrIds, options, func(c *libpod.Container, t uint) error { return c.StopService(t) }) +} + func (ic *ContainerEngine) ContainerPrune(_ context.Context, options entities.ContainerPruneOptions) ([]*reports.PruneReport, error) { filterFuncs := make([]libpod.ContainerFilter, 0, len(options.Filters)) for k, v := range options.Filters { diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index 6947189536..69e89c36f3 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -183,6 +183,10 @@ func (ic *ContainerEngine) ContainerStop(_ context.Context, namesOrIds []string, return reports, nil } +func (ic *ContainerEngine) ContainerStopService(_ context.Context, _ []string, _ entities.StopOptions) ([]*entities.StopReport, error) { + return nil, fmt.Errorf("container stop as service is not supported when using remote connections") +} + func (ic *ContainerEngine) ContainerKill(_ context.Context, namesOrIds []string, opts entities.KillOptions) ([]*entities.KillReport, error) { ctrs, rawInputs, err := getContainersAndInputByContext(ic.ClientCtx, opts.All, false, namesOrIds, nil) if err != nil { diff --git a/test/e2e/ps_test.go b/test/e2e/ps_test.go index 5a5128a542..f90251161d 100644 --- a/test/e2e/ps_test.go +++ b/test/e2e/ps_test.go @@ -1117,4 +1117,43 @@ var _ = Describe("Podman ps", func() { Expect(output).ToNot(ContainSubstring("test-unless-stopped-exit-bad")) Expect(output).ToNot(ContainSubstring("test-always-exit-bad")) }) + + It("podman ps filter should-start-on-boot with --service", func() { + SkipIfRemote("--service flag is not supported on remote") + + commands := [][]string{ + {"create", "--restart", "unless-stopped", "--name", "test-unless-stopped-not-user-stop", ALPINE, "top"}, + {"create", "--restart", "always", "--name", "test-always-not-user-stop", ALPINE, "top"}, + {"create", "--restart", "no", "--name", "test-no-restart-not-user-stop", ALPINE, "top"}, + {"create", "--restart", "on-failure", "--name", "test-onfailure-not-user-stop", ALPINE, "top"}, + {"start", "test-unless-stopped-not-user-stop"}, + {"start", "test-always-not-user-stop"}, + {"start", "test-no-restart-not-user-stop"}, + {"start", "test-onfailure-not-user-stop"}, + {"stop", "--service", "test-unless-stopped-not-user-stop"}, + {"stop", "--service", "test-always-not-user-stop"}, + {"stop", "--service", "test-no-restart-not-user-stop"}, + {"stop", "--service", "test-onfailure-not-user-stop"}, + } + + for _, cmd := range commands { + podmanTest.PodmanExitCleanly(cmd...) + } + + session := podmanTest.PodmanExitCleanly("ps", "-a", "--filter", "should-start-on-boot=true", "--format", "{{.Names}}") + output := session.OutputToString() + + Expect(output).To(ContainSubstring("test-unless-stopped-not-user-stop")) + Expect(output).To(ContainSubstring("test-always-not-user-stop")) + Expect(output).ToNot(ContainSubstring("test-no-restart-not-user-stop")) + Expect(output).ToNot(ContainSubstring("test-onfailure-not-user-stop")) + + session = podmanTest.PodmanExitCleanly("ps", "-a", "--filter", "should-start-on-boot=false", "--format", "{{.Names}}") + output = session.OutputToString() + + Expect(output).To(ContainSubstring("test-no-restart-not-user-stop")) + Expect(output).To(ContainSubstring("test-onfailure-not-user-stop")) + Expect(output).ToNot(ContainSubstring("test-unless-stopped-not-user-stop")) + Expect(output).ToNot(ContainSubstring("test-always-not-user-stop")) + }) }) diff --git a/test/e2e/stop_test.go b/test/e2e/stop_test.go index a597b6150b..1c77c3d16b 100644 --- a/test/e2e/stop_test.go +++ b/test/e2e/stop_test.go @@ -407,4 +407,27 @@ var _ = Describe("Podman stop", func() { Expect(session1).Should(ExitCleanly()) Expect(session1.OutputToString()).To(BeEquivalentTo(cid2)) }) + + It("podman stop --service sets StoppedByUser to false", func() { + SkipIfRemote("--service flag is not supported on remote") + containerName := "test-not-stopped-by-user" + podmanTest.PodmanExitCleanly("run", "-d", "--name", containerName, ALPINE, "top") + + podmanTest.PodmanExitCleanly("stop", "--service", containerName) + + data := podmanTest.InspectContainer(containerName) + Expect(data).To(HaveLen(1)) + Expect(data[0].State.StoppedByUser).To(BeFalse()) + }) + + It("podman stop without --service flag sets StoppedByUser to true", func() { + containerName := "test-default-stop" + podmanTest.PodmanExitCleanly("run", "-d", "--name", containerName, ALPINE, "top") + + podmanTest.PodmanExitCleanly("stop", containerName) + + data := podmanTest.InspectContainer(containerName) + Expect(data).To(HaveLen(1)) + Expect(data[0].State.StoppedByUser).To(BeTrue()) + }) })