Merge pull request #5439 from ttys3/fixup-systemdgen-with-new-param

systemd generator: force run container detached if CreateCommand has no detach param

4 files changed, 139 insertions, 1 deletions

diff --git a/docs/source/markdown/podman-generate-systemd.1.md b/docs/source/markdown/podman-generate-systemd.1.md
index 2bcfdb954..27b40bbb6 100644
--- a/docs/source/markdown/podman-generate-systemd.1.md
+++ b/docs/source/markdown/podman-generate-systemd.1.md
@@ -25,6 +25,7 @@ Use the name of the container for the start, stop, and description in the unit f
 **--new**
 
 Create a new container via podman-run instead of starting an existing one.  This option relies on container configuration files, which may not map directly to podman CLI flags; please review the generated output carefully before placing in production.
+Since we use systemd `Type=forking` service, using this option will force the container run with the detached param `-d`
 
 **--timeout**, **-t**=*value*
 
diff --git a/pkg/systemd/generate/systemdgen.go b/pkg/systemd/generate/systemdgen.go
index 4410e1395..4cd7745c0 100644
--- a/pkg/systemd/generate/systemdgen.go
+++ b/pkg/systemd/generate/systemdgen.go
@@ -164,6 +164,26 @@ func CreateContainerSystemdUnit(info *ContainerInfo, opts Options) (string, erro
 			"--cidfile", "%t/%n-cid",
 			"--cgroups=no-conmon",
 		}
+
+		// Enforce detaching
+		//
+		// since we use systemd `Type=forking` service
+		// @see https://www.freedesktop.org/software/systemd/man/systemd.service.html#Type=
+		// when we generated systemd service file with the --new param,
+		// `ExecStart` will have `/usr/bin/podman run ...`
+		// if `info.CreateCommand` has no `-d` or `--detach` param,
+		// podman will run the container in default attached mode,
+		// as a result, `systemd start` will wait the `podman run` command exit until failed with timeout error.
+		hasDetachParam := false
+		for _, p := range info.CreateCommand[index:] {
+			if p == "--detach" || p == "-d" {
+				hasDetachParam = true
+			}
+		}
+		if !hasDetachParam {
+			command = append(command, "-d")
+		}
+
 		command = append(command, info.CreateCommand[index:]...)
 		info.RunCommand = strings.Join(command, " ")
 		info.New = true
diff --git a/pkg/systemd/generate/systemdgen_test.go b/pkg/systemd/generate/systemdgen_test.go
index 3749d89ce..9eb21e9ed 100644
--- a/pkg/systemd/generate/systemdgen_test.go
+++ b/pkg/systemd/generate/systemdgen_test.go
@@ -132,7 +132,7 @@ After=network-online.target
 [Service]
 Restart=always
 ExecStartPre=/usr/bin/rm -f %t/%n-pid %t/%n-cid
-ExecStart=/usr/bin/podman run --conmon-pidfile %t/%n-pid --cidfile %t/%n-cid --cgroups=no-conmon --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/%n-pid --cidfile %t/%n-cid --cgroups=no-conmon -d --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
 ExecStop=/usr/bin/podman stop --ignore --cidfile %t/%n-cid -t 42
 ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/%n-cid
 PIDFile=%t/%n-pid
@@ -142,6 +142,50 @@ Type=forking
 [Install]
 WantedBy=multi-user.target default.target`
 
+	goodNameNewDetach := `# jadda-jadda.service
+# autogenerated by Podman CI
+
+[Unit]
+Description=Podman jadda-jadda.service
+Documentation=man:podman-generate-systemd(1)
+Wants=network.target
+After=network-online.target
+
+[Service]
+Restart=always
+ExecStartPre=/usr/bin/rm -f %t/%n-pid %t/%n-cid
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/%n-pid --cidfile %t/%n-cid --cgroups=no-conmon --detach --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
+ExecStop=/usr/bin/podman stop --ignore --cidfile %t/%n-cid -t 42
+ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/%n-cid
+PIDFile=%t/%n-pid
+KillMode=none
+Type=forking
+
+[Install]
+WantedBy=multi-user.target`
+
+	goodIdNew := `# container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.service
+# autogenerated by Podman CI
+
+[Unit]
+Description=Podman container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.service
+Documentation=man:podman-generate-systemd(1)
+Wants=network.target
+After=network-online.target
+
+[Service]
+Restart=always
+ExecStartPre=/usr/bin/rm -f %t/%n-pid %t/%n-cid
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/%n-pid --cidfile %t/%n-cid --cgroups=no-conmon -d awesome-image:latest
+ExecStop=/usr/bin/podman stop --ignore --cidfile %t/%n-cid -t 10
+ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/%n-cid
+PIDFile=%t/%n-pid
+KillMode=none
+Type=forking
+
+[Install]
+WantedBy=multi-user.target`
+
 	tests := []struct {
 		name    string
 		info    ContainerInfo
@@ -230,6 +274,51 @@ WantedBy=multi-user.target default.target`
 			goodNameNew,
 			false,
 		},
+		{"good with explicit short detach param",
+			ContainerInfo{
+				Executable:    "/usr/bin/podman",
+				ServiceName:   "jadda-jadda",
+				ContainerName: "jadda-jadda",
+				RestartPolicy: "always",
+				PIDFile:       "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:   42,
+				PodmanVersion: "CI",
+				New:           true,
+				CreateCommand: []string{"I'll get stripped", "container", "run", "-d", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
+			},
+			goodNameNew,
+			false,
+		},
+		{"good with explicit full detach param",
+			ContainerInfo{
+				Executable:    "/usr/bin/podman",
+				ServiceName:   "jadda-jadda",
+				ContainerName: "jadda-jadda",
+				RestartPolicy: "always",
+				PIDFile:       "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:   42,
+				PodmanVersion: "CI",
+				New:           true,
+				CreateCommand: []string{"I'll get stripped", "container", "run", "--detach", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
+			},
+			goodNameNewDetach,
+			false,
+		},
+		{"good with id and no param",
+			ContainerInfo{
+				Executable:    "/usr/bin/podman",
+				ServiceName:   "container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
+				ContainerName: "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
+				RestartPolicy: "always",
+				PIDFile:       "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:   10,
+				PodmanVersion: "CI",
+				New:           true,
+				CreateCommand: []string{"I'll get stripped", "container", "run", "awesome-image:latest"},
+			},
+			goodIdNew,
+			false,
+		},
 	}
 	for _, tt := range tests {
 		test := tt
diff --git a/test/e2e/generate_systemd_test.go b/test/e2e/generate_systemd_test.go
index 31131a68b..e5ab0b854 100644
--- a/test/e2e/generate_systemd_test.go
+++ b/test/e2e/generate_systemd_test.go
@@ -195,6 +195,34 @@ var _ = Describe("Podman generate systemd", func() {
 		Expect(found).To(BeTrue())
 	})
 
+	It("podman generate systemd --new without explicit detaching param", func() {
+		n := podmanTest.Podman([]string{"create", "--name", "foo", "alpine", "top"})
+		n.WaitWithDefaultTimeout()
+		Expect(n.ExitCode()).To(Equal(0))
+
+		session := podmanTest.Podman([]string{"generate", "systemd", "--timeout", "42", "--name", "--new", "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		// Grepping the output (in addition to unit tests)
+		found, _ := session.GrepString("--cgroups=no-conmon -d")
+		Expect(found).To(BeTrue())
+	})
+
+	It("podman generate systemd --new with explicit detaching param in middle", func() {
+		n := podmanTest.Podman([]string{"create", "--name", "foo", "-d", "alpine", "top"})
+		n.WaitWithDefaultTimeout()
+		Expect(n.ExitCode()).To(Equal(0))
+
+		session := podmanTest.Podman([]string{"generate", "systemd", "--timeout", "42", "--name", "--new", "foo"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+
+		// Grepping the output (in addition to unit tests)
+		found, _ := session.GrepString("--name foo -d alpine top")
+		Expect(found).To(BeTrue())
+	})
+
 	It("podman generate systemd --new pod", func() {
 		n := podmanTest.Podman([]string{"pod", "create", "--name", "foo"})
 		n.WaitWithDefaultTimeout()