From 0ea5080c9156853b7806879226e23ab0405815d6 Mon Sep 17 00:00:00 2001
From: Valentin Rothberg <vrothberg@redhat.com>
Date: Tue, 13 Sep 2022 09:38:42 +0200
Subject: [PATCH] generate systemd: warn on --restart without --new

Emit a warning to the user when generating a unit with --new on a
container that was created with a custom --restart policy. As shown
in #15284, a custom --restart policy in that case can lead to issues
on system shutdown where systemd attempts to nuke the unit but Podman
keeps on restarting the container.

Fixes: #15284
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
---
 docs/source/markdown/podman-generate-systemd.1.md |  4 +++-
 pkg/systemd/generate/containers.go                | 12 ++++++++++++
 test/system/250-systemd.bats                      |  7 +++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/docs/source/markdown/podman-generate-systemd.1.md b/docs/source/markdown/podman-generate-systemd.1.md
index ee649c95ba..b733cff8dd 100644
--- a/docs/source/markdown/podman-generate-systemd.1.md
+++ b/docs/source/markdown/podman-generate-systemd.1.md
@@ -85,7 +85,9 @@ Set the systemd unit requires (`Requires=`) option. Similar to wants, but declar
 #### **--restart-policy**=*policy*
 
 Set the systemd restart policy.  The restart-policy must be one of: "no", "on-success", "on-failure", "on-abnormal",
-"on-watchdog", "on-abort", or "always".  The default policy is *on-failure*.
+"on-watchdog", "on-abort", or "always".  The default policy is *on-failure* unless the container was created with a custom restart policy.
+
+Note that generating a unit without `--new` on a container with a custom restart policy can lead to issues on shutdown; systemd will attempt to stop the unit while Podman tries to restart it.  It is recommended to to create the container without `--restart` and use the `--restart-policy` option instead when generating the unit file.
 
 #### **--restart-sec**=*time*
 
diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go
index 0274dd7b72..8510cfd42e 100644
--- a/pkg/systemd/generate/containers.go
+++ b/pkg/systemd/generate/containers.go
@@ -148,6 +148,18 @@ func generateContainerInfo(ctr *libpod.Container, options entities.GenerateSyste
 		return nil, errors.New("conmon PID file path is empty, try to recreate the container with --conmon-pidfile flag")
 	}
 
+	// #15284: old units generated without --new can lead to issues on
+	// shutdown when the containers are created with a custom restart
+	// policy.
+	if !options.New {
+		switch config.RestartPolicy {
+		case libpodDefine.RestartPolicyNo, libpodDefine.RestartPolicyNone:
+			// All good
+		default:
+			logrus.Warnf("Container %s has restart policy %q which can lead to issues on shutdown: consider recreating the container without a restart policy and use systemd's restart mechanism instead", ctr.ID(), config.RestartPolicy)
+		}
+	}
+
 	createCommand := []string{}
 	if config.CreateCommand != nil {
 		createCommand = config.CreateCommand
diff --git a/test/system/250-systemd.bats b/test/system/250-systemd.bats
index 0e1dc356d4..ddec3a492e 100644
--- a/test/system/250-systemd.bats
+++ b/test/system/250-systemd.bats
@@ -81,6 +81,13 @@ function service_cleanup() {
         skip "FIXME: 2022-09-01: requires conmon-2.1.4, ubuntu has 2.1.3"
     fi
 
+    # Warn when a custom restart policy is used without --new (see #15284)
+    run_podman create --restart=always $IMAGE
+    cid="$output"
+    run_podman generate systemd $cid
+    is "$output" ".*Container $cid has restart policy .*always.* which can lead to issues on shutdown.*" "generate systemd emits warning"
+    run_podman rm -f $cid
+
     cname=$(random_string)
     # See #7407 for --pull=always.
     run_podman create --pull=always --name $cname --label "io.containers.autoupdate=registry" $IMAGE \