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 <rothberg@redhat.com>
2020-06-15 15:32:43 +02:00 · 2020-06-15 15:32:43 +02:00 · 6118ab4948
parent fe488b5f11
commit 6118ab4948
4 changed files with 121 additions and 19 deletions
--- 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)
 		}

+		// Presence check for certain flags/options.
+		hasDetachParam := false
+		hasNameParam := false
+		hasReplaceParam := false
+		for _, p := range info.CreateCommand[index:] {
+			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.
-		hasDetachParam := false
-		for _, p := range info.CreateCommand[index:] {
-			if p == "--detach" || p == "-d" {
-				hasDetachParam = true
-			}
-		}
-		if !hasDetachParam {
+			// 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}}"
--- 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,
 		},
--- 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}}"
--- 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,6 +56,32 @@ 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`

@ -63,6 +89,7 @@ WantedBy=multi-user.target default.target`
 		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 {