Fix unless-stopped containers not restarting after podman-restart.service stop them

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 <hony.com@seznam.cz>

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
 	}

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

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

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 {

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)

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 {

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 {

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"))
+	})
 })

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())
+	})
 })