generate systemd: --replace on named containers/pods

Use `--replace` for named containers and pods. This will clean up previous containers and podsthat may not have been removed after a system crash. Fixes: #5485 Signed-off-by: Valentin Rothberg <[email protected]>
containers · Jun 15, 2020 · 6118ab4 · 6118ab4
1 parent fe488b5
commit 6118ab4
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 19 deletions.
diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go
@@ -207,24 +207,42 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
 			info.CreateCommand = filterPodFlags(info.CreateCommand)
 		}
 
-		// 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.
+		// Presence check for certain flags/options.
 		hasDetachParam := false
+		hasNameParam := false
+		hasReplaceParam := false
 		for _, p := range info.CreateCommand[index:] {
-			if p == "--detach" || p == "-d" {
+			switch p {
+			case "--detach", "-d":
 				hasDetachParam = true
+			case "--name":
+				hasNameParam = true
+			case "--replace":
+				hasReplaceParam = true
 			}
 		}
+
 		if !hasDetachParam {
+			// 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.
 			startCommand = append(startCommand, "-d")
 		}
+		if hasNameParam && !hasReplaceParam {
+			// Enforce --replace for named containers.  This will
+			// make systemd units more robuts as it allows them to
+			// start after system crashes (see
+			// github.com/containers/libpod/issues/5485).
+			startCommand = append(startCommand, "--replace")
+		}
 		startCommand = append(startCommand, info.CreateCommand[index:]...)
 
 		info.ExecStartPre = "/usr/bin/rm -f {{.PIDFile}} {{.ContainerIDFile}}"

diff --git a/pkg/systemd/generate/containers_test.go b/pkg/systemd/generate/containers_test.go
@@ -103,7 +103,7 @@ Type=forking
 [Install]
 WantedBy=multi-user.target default.target`
 
-	goodNameNew := `# jadda-jadda.service
+	goodWithNameAndGeneric := `# jadda-jadda.service
 # autogenerated by Podman CI
 
 [Unit]
@@ -116,7 +116,30 @@ After=network-online.target
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=always
 ExecStartPre=/usr/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
-ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon -d --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon -d --replace --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
+ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 42
+ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
+PIDFile=%t/jadda-jadda.pid
+KillMode=none
+Type=forking
+
+[Install]
+WantedBy=multi-user.target default.target`
+
+	goodWithExplicitShortDetachParam := `# 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]
+Environment=PODMAN_SYSTEMD_UNIT=%n
+Restart=always
+ExecStartPre=/usr/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon --replace -d --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
 ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 42
 ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
 PIDFile=%t/jadda-jadda.pid
@@ -139,7 +162,7 @@ After=network-online.target
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=always
 ExecStartPre=/usr/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
-ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon --pod-id-file /tmp/pod-foobar.pod-id-file -d --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon --pod-id-file /tmp/pod-foobar.pod-id-file --replace -d --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
 ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 42
 ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
 PIDFile=%t/jadda-jadda.pid
@@ -148,6 +171,7 @@ Type=forking
 
 [Install]
 WantedBy=multi-user.target default.target`
+
 	goodNameNewDetach := `# jadda-jadda.service
 # autogenerated by Podman CI
 
@@ -161,7 +185,7 @@ After=network-online.target
 Environment=PODMAN_SYSTEMD_UNIT=%n
 Restart=always
 ExecStartPre=/usr/bin/rm -f %t/jadda-jadda.pid %t/jadda-jadda.ctr-id
-ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon --detach --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
+ExecStart=/usr/bin/podman run --conmon-pidfile %t/jadda-jadda.pid --cidfile %t/jadda-jadda.ctr-id --cgroups=no-conmon --replace --detach --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN
 ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 42
 ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
 PIDFile=%t/jadda-jadda.pid
@@ -274,7 +298,7 @@ WantedBy=multi-user.target default.target`
 				CreateCommand:     []string{"I'll get stripped", "container", "run", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
 				EnvVariable:       EnvVariable,
 			},
-			goodNameNew,
+			goodWithNameAndGeneric,
 			true,
 			false,
 		},
@@ -290,7 +314,7 @@ WantedBy=multi-user.target default.target`
 				CreateCommand:     []string{"I'll get stripped", "container", "run", "-d", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
 				EnvVariable:       EnvVariable,
 			},
-			goodNameNew,
+			goodWithExplicitShortDetachParam,
 			true,
 			false,
 		},

diff --git a/pkg/systemd/generate/pods.go b/pkg/systemd/generate/pods.go
@@ -265,7 +265,7 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions)
 			if podCreateIndex == 0 {
 				return "", errors.Errorf("pod does not appear to be created via `podman pod create`: %v", info.CreateCommand)
 			}
-			podRootArgs = info.CreateCommand[1 : podCreateIndex-2]
+			podRootArgs = info.CreateCommand[0 : podCreateIndex-2]
 			podCreateArgs = filterPodFlags(info.CreateCommand[podCreateIndex+1:])
 		}
 		// We're hard-coding the first five arguments and append the
@@ -277,6 +277,21 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions)
 				"--infra-conmon-pidfile", "{{.PIDFile}}",
 				"--pod-id-file", "{{.PodIDFile}}"}...)
 
+		// Presence check for certain flags/options.
+		hasNameParam := false
+		hasReplaceParam := false
+		for _, p := range podCreateArgs {
+			switch p {
+			case "--name":
+				hasNameParam = true
+			case "--replace":
+				hasReplaceParam = true
+			}
+		}
+		if hasNameParam && !hasReplaceParam {
+			podCreateArgs = append(podCreateArgs, "--replace")
+		}
+
 		startCommand = append(startCommand, podCreateArgs...)
 
 		info.ExecStartPre1 = "/usr/bin/rm -f {{.PIDFile}} {{.PodIDFile}}"

diff --git a/pkg/systemd/generate/pods_test.go b/pkg/systemd/generate/pods_test.go
@@ -36,7 +36,7 @@ func TestValidateRestartPolicyPod(t *testing.T) {
 }
 
 func TestCreatePodSystemdUnit(t *testing.T) {
-	podGoodName := `# pod-123abc.service
+	podGood := `# pod-123abc.service
 # autogenerated by Podman CI
 
 [Unit]
@@ -56,13 +56,40 @@ PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635
 KillMode=none
 Type=forking
 
+[Install]
+WantedBy=multi-user.target default.target`
+
+	podGoodNamedNew := `# pod-123abc.service
+# autogenerated by Podman CI
+
+[Unit]
+Description=Podman pod-123abc.service
+Documentation=man:podman-generate-systemd(1)
+Wants=network.target
+After=network-online.target
+Requires=container-1.service container-2.service
+Before=container-1.service container-2.service
+
+[Service]
+Environment=PODMAN_SYSTEMD_UNIT=%n
+Restart=on-failure
+ExecStartPre=/usr/bin/rm -f %t/pod-123abc.pid %t/pod-123abc.pod-id
+ExecStartPre=/usr/bin/podman pod create --infra-conmon-pidfile %t/pod-123abc.pid --pod-id-file %t/pod-123abc.pod-id --name foo --replace
+ExecStart=/usr/bin/podman pod start --pod-id-file %t/pod-123abc.pod-id
+ExecStop=/usr/bin/podman pod stop --ignore --pod-id-file %t/pod-123abc.pod-id -t 10
+ExecStopPost=/usr/bin/podman pod rm --ignore -f --pod-id-file %t/pod-123abc.pod-id
+PIDFile=%t/pod-123abc.pid
+KillMode=none
+Type=forking
+
 [Install]
 WantedBy=multi-user.target default.target`
 
 	tests := []struct {
 		name    string
 		info    podInfo
 		want    string
+		new     bool
 		wantErr bool
 	}{
 		{"pod",
@@ -76,7 +103,24 @@ WantedBy=multi-user.target default.target`
 				PodmanVersion:    "CI",
 				RequiredServices: []string{"container-1", "container-2"},
 			},
-			podGoodName,
+			podGood,
+			false,
+			false,
+		},
+		{"pod --new",
+			podInfo{
+				Executable:       "/usr/bin/podman",
+				ServiceName:      "pod-123abc",
+				InfraNameOrID:    "jadda-jadda-infra",
+				RestartPolicy:    "on-failure",
+				PIDFile:          "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
+				StopTimeout:      10,
+				PodmanVersion:    "CI",
+				RequiredServices: []string{"container-1", "container-2"},
+				CreateCommand:    []string{"podman", "pod", "create", "--name", "foo"},
+			},
+			podGoodNamedNew,
+			true,
 			false,
 		},
 	}
@@ -86,6 +130,7 @@ WantedBy=multi-user.target default.target`
 		t.Run(tt.name, func(t *testing.T) {
 			opts := entities.GenerateSystemdOptions{
 				Files: false,
+				New:   test.new,
 			}
 			got, err := executePodTemplate(&test.info, opts)
 			if (err != nil) != test.wantErr {