Merge pull request #3497 from QazerLab/bugfix/systemd-generate-pidfile

Use conmon pidfile in generated systemd unit as PIDFile.

7 files changed, 101 insertions, 17 deletions

diff --git a/docs/podman-generate-systemd.1.md b/docs/podman-generate-systemd.1.md
index 09752480d..64e68a69a 100644
--- a/docs/podman-generate-systemd.1.md
+++ b/docs/podman-generate-systemd.1.md
@@ -39,7 +39,7 @@ ExecStart=/usr/bin/podman start c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f14
 ExecStop=/usr/bin/podman stop -t 10 c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc
 KillMode=none
 Type=forking
-PIDFile=/var/lib/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc.pid
+PIDFile=/var/run/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/conmon.pid
 [Install]
 WantedBy=multi-user.target
 ```
@@ -55,7 +55,7 @@ ExecStart=/usr/bin/podman start c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f14
 ExecStop=/usr/bin/podman stop -t 1 c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc
 KillMode=none
 Type=forking
-PIDFile=/var/lib/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc.pid
+PIDFile=/var/run/containers/storage/overlay-containers/c21da63c4783be2ac2cd3487ef8d2ec15ee2a28f63dd8f145e3b05607f31cffc/userdata/conmon.pid
 [Install]
 WantedBy=multi-user.target
 ```
diff --git a/libpod/runtime_ctr.go b/libpod/runtime_ctr.go
index 633d26fe1..ae9b3e5bc 100644
--- a/libpod/runtime_ctr.go
+++ b/libpod/runtime_ctr.go
@@ -237,7 +237,7 @@ func (r *Runtime) setupContainer(ctx context.Context, ctr *Container) (c *Contai
 		}
 	}()
 
-	if rootless.IsRootless() && ctr.config.ConmonPidFile == "" {
+	if ctr.config.ConmonPidFile == "" {
 		ctr.config.ConmonPidFile = filepath.Join(ctr.state.RunDir, "conmon.pid")
 	}
 
diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go
index 10720886b..0ea89a72c 100644
--- a/pkg/adapter/containers.go
+++ b/pkg/adapter/containers.go
@@ -1058,7 +1058,14 @@ func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (stri
 	if c.Name {
 		name = ctr.Name()
 	}
-	return systemdgen.CreateSystemdUnitAsString(name, ctr.ID(), c.RestartPolicy, ctr.Config().StaticDir, timeout)
+
+	config := ctr.Config()
+	conmonPidFile := config.ConmonPidFile
+	if conmonPidFile == "" {
+		return "", errors.Errorf("conmon PID file path is empty, try to recreate the container with --conmon-pidfile flag")
+	}
+
+	return systemdgen.CreateSystemdUnitAsString(name, ctr.ID(), c.RestartPolicy, conmonPidFile, timeout)
 }
 
 // GetNamespaces returns namespace information about a container for PS
diff --git a/pkg/systemdgen/systemdgen.go b/pkg/systemdgen/systemdgen.go
index 3d1c31b5d..06c5ebde5 100644
--- a/pkg/systemdgen/systemdgen.go
+++ b/pkg/systemdgen/systemdgen.go
@@ -2,17 +2,18 @@ package systemdgen
 
 import (
 	"fmt"
-	"path/filepath"
+	"os"
 
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 )
 
 var template = `[Unit]
 Description=%s Podman Container
 [Service]
 Restart=%s
-ExecStart=/usr/bin/podman start %s
-ExecStop=/usr/bin/podman stop -t %d %s
+ExecStart=%s start %s
+ExecStop=%s stop -t %d %s
 KillMode=none
 Type=forking
 PIDFile=%s
@@ -33,11 +34,26 @@ func ValidateRestartPolicy(restart string) error {
 
 // CreateSystemdUnitAsString takes variables to create a systemd unit file used to control
 // a libpod container
-func CreateSystemdUnitAsString(name, cid, restart, pidPath string, stopTimeout int) (string, error) {
+func CreateSystemdUnitAsString(name, cid, restart, pidFile string, stopTimeout int) (string, error) {
+	podmanExe := getPodmanExecutable()
+	return createSystemdUnitAsString(podmanExe, name, cid, restart, pidFile, stopTimeout)
+}
+
+func createSystemdUnitAsString(exe, name, cid, restart, pidFile string, stopTimeout int) (string, error) {
 	if err := ValidateRestartPolicy(restart); err != nil {
 		return "", err
 	}
-	pidFile := filepath.Join(pidPath, fmt.Sprintf("%s.pid", cid))
-	unit := fmt.Sprintf(template, name, restart, name, stopTimeout, name, pidFile)
+
+	unit := fmt.Sprintf(template, name, restart, exe, name, exe, stopTimeout, name, pidFile)
 	return unit, nil
 }
+
+func getPodmanExecutable() string {
+	podmanExe, err := os.Executable()
+	if err != nil {
+		podmanExe = "/usr/bin/podman"
+		logrus.Warnf("Could not obtain podman executable location, using default %s", podmanExe)
+	}
+
+	return podmanExe
+}
diff --git a/pkg/systemdgen/systemdgen_test.go b/pkg/systemdgen/systemdgen_test.go
index f2f49e750..e413b24ce 100644
--- a/pkg/systemdgen/systemdgen_test.go
+++ b/pkg/systemdgen/systemdgen_test.go
@@ -41,7 +41,7 @@ ExecStart=/usr/bin/podman start 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4
 ExecStop=/usr/bin/podman stop -t 10 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401
 KillMode=none
 Type=forking
-PIDFile=/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid
+PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid
 [Install]
 WantedBy=multi-user.target`
 
@@ -53,15 +53,16 @@ ExecStart=/usr/bin/podman start foobar
 ExecStop=/usr/bin/podman stop -t 10 foobar
 KillMode=none
 Type=forking
-PIDFile=/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid
+PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid
 [Install]
 WantedBy=multi-user.target`
 
 	type args struct {
+		exe         string
 		name        string
 		cid         string
 		restart     string
-		pidPath     string
+		pidFile     string
 		stopTimeout int
 	}
 	tests := []struct {
@@ -73,10 +74,11 @@ WantedBy=multi-user.target`
 
 		{"good with id",
 			args{
+				"/usr/bin/podman",
 				"639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
 				"639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
 				"always",
-				"/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/",
+				"/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
 				10,
 			},
 			goodID,
@@ -84,10 +86,11 @@ WantedBy=multi-user.target`
 		},
 		{"good with name",
 			args{
+				"/usr/bin/podman",
 				"foobar",
 				"639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
 				"always",
-				"/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/",
+				"/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
 				10,
 			},
 			goodName,
@@ -95,10 +98,11 @@ WantedBy=multi-user.target`
 		},
 		{"bad restart policy",
 			args{
+				"/usr/bin/podman",
 				"639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
 				"639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
 				"never",
-				"/var/lib/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/",
+				"/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
 				10,
 			},
 			"",
@@ -107,7 +111,7 @@ WantedBy=multi-user.target`
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := CreateSystemdUnitAsString(tt.args.name, tt.args.cid, tt.args.restart, tt.args.pidPath, tt.args.stopTimeout)
+			got, err := createSystemdUnitAsString(tt.args.exe, tt.args.name, tt.args.cid, tt.args.restart, tt.args.pidFile, tt.args.stopTimeout)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("CreateSystemdUnitAsString() error = %v, wantErr %v", err, tt.wantErr)
 				return
diff --git a/test/system/250-generate-systemd.bats b/test/system/250-generate-systemd.bats
new file mode 100644
index 000000000..80199af5f
--- /dev/null
+++ b/test/system/250-generate-systemd.bats
@@ -0,0 +1,46 @@
+#!/usr/bin/env bats   -*- bats -*-
+#
+# Tests generated configurations for systemd.
+#
+
+load helpers
+
+# Be extra paranoid in naming to avoid collisions.
+SERVICE_NAME="podman_test_$(random_string)"
+UNIT_DIR="$HOME/.config/systemd/user"
+UNIT_FILE="$UNIT_DIR/$SERVICE_NAME.service"
+
+function setup() {
+    skip_if_not_systemd
+    skip_if_remote
+
+    basic_setup
+
+    if [ ! -d "$UNIT_DIR" ]; then
+        mkdir -p "$UNIT_DIR"
+        systemctl --user daemon-reload
+    fi
+}
+
+function teardown() {
+    rm -f "$UNIT_FILE"
+    systemctl --user stop "$SERVICE_NAME"
+    basic_teardown
+}
+
+@test "podman generate - systemd - basic" {
+    run_podman create $IMAGE echo "I'm alive!"
+    cid="$output"
+
+    run_podman generate systemd $cid > "$UNIT_FILE"
+
+    run systemctl --user start "$SERVICE_NAME"
+    if [ $status -ne 0 ]; then
+        die "The systemd service $SERVICE_NAME did not start correctly, output: $output"
+    fi
+
+    run_podman logs $cid
+    is "$output" "I'm alive!" "Container output"
+}
+
+# vim: filetype=sh
diff --git a/test/system/helpers.bash b/test/system/helpers.bash
index 29ef19ecc..1db80f111 100644
--- a/test/system/helpers.bash
+++ b/test/system/helpers.bash
@@ -236,6 +236,17 @@ function skip_if_remote() {
     skip "${1:-test does not work with podman-remote}"
 }
 
+#########################
+#  skip_if_not_systemd  #  ...with an optional message
+#########################
+function skip_if_not_systemd() {
+    if systemctl --user >/dev/null 2>&1; then
+        return
+    fi
+
+    skip "${1:-no systemd or daemon does not respond}"
+}
+
 #########
 #  die  #  Abort with helpful message
 #########