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: #28152
Fixes: https://issues.redhat.com/browse/RUN-4357

Signed-off-by: Jan Rodák <hony.com@seznam.cz>

Author: Honny1

cmd/podman/containers/stop.go

@@ -73,9 +73,14 @@ func stopFlags(cmd *cobra.Command) {
 	flags.StringArrayVarP(&filters, filterFlagName, "f", []string{}, "Filter output based on conditions given")
 	_ = cmd.RegisterFlagCompletionFunc(filterFlagName, common.AutocompletePsFilters)
 
+	notStoppedByUserFlagName := "not-stopped-by-user"
+	flags.BoolVar(&stopOptions.NotStoppedByUser, notStoppedByUserFlagName, false, "Do not mark container as stopped by user")
+	_ = flags.MarkHidden(notStoppedByUserFlagName)
+
 	if registry.IsRemote() {
 		_ = flags.MarkHidden("cidfile")
 		_ = flags.MarkHidden("ignore")
+		_ = flags.MarkHidden(notStoppedByUserFlagName)
 	}
 
 	flags.SetNormalizeFunc(utils.TimeoutAliasFlags)

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  --not-stopped-by-user --all
 
 [Install]
 WantedBy=default.target

libpod/container_api.go

@@ -283,6 +283,13 @@ 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, 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.
+func (c *Container) StopWithArgs(timeout uint, notStoppedByUser 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 +327,7 @@ func (c *Container) StopWithTimeout(timeout uint) (finalErr error) {
 			return err
 		}
 	}
-
-	return c.stop(timeout)
+	return c.stopInternal(timeout, notStoppedByUser)
 }
 
 // Kill sends a signal to a container

libpod/container_internal.go

@@ -1373,6 +1373,12 @@ func (c *Container) stopWithAll() bool {
 
 // Internal, non-locking function to stop container
 func (c *Container) stop(timeout uint) error {
+	return c.stopInternal(timeout, false)
+}
+
+// Internal, non-locking function to stop container
+// notStoppedByUser controls whether to set the StoppedByUser state field.
+func (c *Container) stopInternal(timeout uint, notStoppedByUser bool) error {
 	logrus.Debugf("Stopping ctr %s (timeout %d)", c.ID(), timeout)
 
 	all := c.stopWithAll()
@@ -1390,7 +1396,10 @@ 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 !notStoppedByUser {
+		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

pkg/domain/entities/containers.go

@@ -101,6 +101,11 @@ type StopOptions struct {
 	Ignore  bool
 	Latest  bool
 	Timeout *uint
+	// NotStoppedByUser specifies that the container should not be marked as
+	// stopped by user. This is used internally by systemd units and should not
+	// be exposed via the REST API. When set to true, the StoppedByUser field will
+	// not be set when stopping the container.
+	NotStoppedByUser bool
 }
 
 type StopReport struct {

pkg/domain/infra/abi/containers.go

@@ -318,13 +318,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 := c.StopWithArgs(timeout, options.NotStoppedByUser); err != nil {
 			switch {
 			case errors.Is(err, define.ErrCtrStopped):
 				logrus.Debugf("Container %s is already stopped", c.ID())
@@ -359,7 +358,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) {

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 --not-stopped-by-user", func() {
+		SkipIfRemote("--not-stopped-by-user 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", "--not-stopped-by-user", "test-unless-stopped-not-user-stop"},
+			{"stop", "--not-stopped-by-user", "test-always-not-user-stop"},
+			{"stop", "--not-stopped-by-user", "test-no-restart-not-user-stop"},
+			{"stop", "--not-stopped-by-user", "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 --not-stopped-by-user sets StoppedByUser to false", func() {
+		SkipIfRemote("--not-stopped-by-user flag is not supported on remote")
+		containerName := "test-not-stopped-by-user"
+		podmanTest.PodmanExitCleanly("run", "-d", "--name", containerName, ALPINE, "top")
+
+		podmanTest.PodmanExitCleanly("stop", "--not-stopped-by-user", containerName)
+
+		data := podmanTest.InspectContainer(containerName)
+		Expect(data).To(HaveLen(1))
+		Expect(data[0].State.StoppedByUser).To(BeFalse())
+	})
+
+	It("podman stop without --not-stopped-by-user 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())
+	})
 })