From 6118ab494884eea62c388fa1536349f05cdac9d3 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 15 Jun 2020 15:32:43 +0200 Subject: [PATCH] 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 --- pkg/systemd/generate/containers.go | 38 ++++++++++++++----- pkg/systemd/generate/containers_test.go | 36 +++++++++++++++--- pkg/systemd/generate/pods.go | 17 ++++++++- pkg/systemd/generate/pods_test.go | 49 ++++++++++++++++++++++++- 4 files changed, 121 insertions(+), 19 deletions(-) diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go index dced1a3da5..4180022cb1 100644 --- 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 index 8365ecd7a5..8d3ea1ca00 100644 --- 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 index 5cfd5ab0a1..367b8381fd 100644 --- 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 index f6e225c355..f7ce33a304 100644 --- 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 {