From 505f43c084a710c36195b8fe979c2ccb673c6e0c Mon Sep 17 00:00:00 2001 From: Robb Manes Date: Tue, 16 Mar 2021 12:42:02 -0400 Subject: Add RequiresMountsFor= to systemd generate It is rare but possible that storage locations for the graphroot and the runroot are not mounted at boot time, and therefore might race when doing container operations. An example we've seen in the wild is that a slow tmpfs mount for the runroot would suddenly mount over /run, causing the container to lose all currently-running data, requiring a system refresh to get it back. This patch adds RequiresMountsFor= to the systemd.unit header to ensure the paths for both the graphroot and runroot are mounted prior to starting any generated unit files. Signed-off-by: Robb Manes --- pkg/systemd/generate/common.go | 1 + pkg/systemd/generate/containers.go | 23 ++++++++++++++ pkg/systemd/generate/containers_test.go | 54 +++++++++++++++++++++++++++++++++ pkg/systemd/generate/pods.go | 6 ++++ pkg/systemd/generate/pods_test.go | 19 ++++++++++++ 5 files changed, 103 insertions(+) (limited to 'pkg/systemd/generate') diff --git a/pkg/systemd/generate/common.go b/pkg/systemd/generate/common.go index 94a6f4cb5..19d468403 100644 --- a/pkg/systemd/generate/common.go +++ b/pkg/systemd/generate/common.go @@ -36,6 +36,7 @@ Description=Podman {{{{.ServiceName}}}}.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor={{{{.GraphRoot}}}} {{{{.RunRoot}}}} ` // filterPodFlags removes --pod and --pod-id-file from the specified command. diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go index 9343a5067..bc13a6116 100644 --- a/pkg/systemd/generate/containers.go +++ b/pkg/systemd/generate/containers.go @@ -71,6 +71,12 @@ type containerInfo struct { // If not nil, the container is part of the pod. We can use the // podInfo to extract the relevant data. Pod *podInfo + // Location of the GraphRoot for the container. Required for ensuring the + // volume has finished mounting when coming online at boot. + GraphRoot string + // Location of the RunRoot for the container. Required for ensuring the tmpfs + // or volume exists and is mounted when coming online at boot. + RunRoot string } const containerTemplate = headerTemplate + ` @@ -132,6 +138,21 @@ func generateContainerInfo(ctr *libpod.Container, options entities.GenerateSyste nameOrID, serviceName := containerServiceName(ctr, options) + store := ctr.Runtime().GetStore() + if store == nil { + return nil, errors.Errorf("could not determine storage store for container") + } + + graphRoot := store.GraphRoot() + if graphRoot == "" { + return nil, errors.Errorf("could not lookup container's graphroot: got empty string") + } + + runRoot := store.RunRoot() + if runRoot == "" { + return nil, errors.Errorf("could not lookup container's runroot: got empty string") + } + info := containerInfo{ ServiceName: serviceName, ContainerNameOrID: nameOrID, @@ -140,6 +161,8 @@ func generateContainerInfo(ctr *libpod.Container, options entities.GenerateSyste StopTimeout: timeout, GenerateTimestamp: true, CreateCommand: createCommand, + GraphRoot: graphRoot, + RunRoot: runRoot, } return &info, nil diff --git a/pkg/systemd/generate/containers_test.go b/pkg/systemd/generate/containers_test.go index ebbbdb786..1359c1a37 100644 --- a/pkg/systemd/generate/containers_test.go +++ b/pkg/systemd/generate/containers_test.go @@ -48,6 +48,7 @@ Description=Podman container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -73,6 +74,7 @@ Description=Podman container-foobar.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -96,6 +98,7 @@ Description=Podman container-foobar.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage BindsTo=a.service b.service c.service pod.service After=a.service b.service c.service pod.service @@ -121,6 +124,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -145,6 +149,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -169,6 +174,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -193,6 +199,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -217,6 +224,7 @@ Description=Podman container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -242,6 +250,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -270,6 +279,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -294,6 +304,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -318,6 +329,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -342,6 +354,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -366,6 +379,7 @@ Description=Podman jadda-jadda.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage [Service] Environment=PODMAN_SYSTEMD_UNIT=%n @@ -400,6 +414,8 @@ WantedBy=multi-user.target default.target StopTimeout: 22, PodmanVersion: "CI", EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodID, false, @@ -416,6 +432,8 @@ WantedBy=multi-user.target default.target StopTimeout: 22, PodmanVersion: "CI", EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodIDNoHeaderInfo, false, @@ -432,6 +450,8 @@ WantedBy=multi-user.target default.target StopTimeout: 10, PodmanVersion: "CI", EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodName, false, @@ -449,6 +469,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", BoundToServices: []string{"pod", "a", "b", "c"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNameBoundTo, false, @@ -464,6 +486,8 @@ WantedBy=multi-user.target default.target StopTimeout: 10, PodmanVersion: "CI", EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, "", false, @@ -481,6 +505,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "container", "run", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN", "foo=arg \"with \" space"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodWithNameAndGeneric, true, @@ -498,6 +524,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "-d", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodWithExplicitShortDetachParam, true, @@ -515,6 +543,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "-d", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", Pod: &podInfo{ PodIDFile: "%t/pod-foobar.pod-id-file", }, @@ -535,6 +565,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "--detach", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNameNewDetach, true, @@ -552,6 +584,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodIDNew, true, @@ -569,6 +603,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "--detach=true", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, genGoodNewDetach("--detach=true"), true, @@ -586,6 +622,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "--detach=false", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, genGoodNewDetach("-d"), true, @@ -603,6 +641,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "--name", "test", "-p", "80:80", "--detach=false", "awesome-image:latest", "somecmd", "--detach=false"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNameNewDetachFalseWithCmd, true, @@ -620,6 +660,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "--name", "test", "-p", "80:80", "--detach=false", "--detach=false", "awesome-image:latest", "somecmd", "--detach=false"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNameNewDetachFalseWithCmd, true, @@ -637,6 +679,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "-dti", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, genGoodNewDetach("-dti"), true, @@ -654,6 +698,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "run", "-tid", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, genGoodNewDetach("-tid"), true, @@ -671,6 +717,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "--events-backend", "none", "--runroot", "/root", "run", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNewRootFlags, true, @@ -688,6 +736,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "container", "create", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodContainerCreate, true, @@ -705,6 +755,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "create", "--name", "test", "--log-driver=journald", "--log-opt=tag={{.Name}}", "awesome-image:latest"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNewWithJournaldTag, true, @@ -722,6 +774,8 @@ WantedBy=multi-user.target default.target PodmanVersion: "CI", CreateCommand: []string{"I'll get stripped", "create", "--name", "test", "awesome-image:latest", "sh", "-c", "kill $$ && echo %\\"}, EnvVariable: define.EnvVariable, + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", }, goodNewWithSpecialChars, true, diff --git a/pkg/systemd/generate/pods.go b/pkg/systemd/generate/pods.go index f96058d36..a76979ecf 100644 --- a/pkg/systemd/generate/pods.go +++ b/pkg/systemd/generate/pods.go @@ -73,6 +73,12 @@ type podInfo struct { ExecStopPost string // Removes autogenerated by Podman and timestamp if set to true GenerateNoHeader bool + // Location of the GraphRoot for the pod. Required for ensuring the + // volume has finished mounting when coming online at boot. + GraphRoot string + // Location of the RunRoot for the pod. Required for ensuring the tmpfs + // or volume exists and is mounted when coming online at boot. + RunRoot string } const podTemplate = headerTemplate + `Requires={{{{- range $index, $value := .RequiredServices -}}}}{{{{if $index}}}} {{{{end}}}}{{{{ $value }}}}.service{{{{end}}}} diff --git a/pkg/systemd/generate/pods_test.go b/pkg/systemd/generate/pods_test.go index 50c8d4556..559f7365f 100644 --- a/pkg/systemd/generate/pods_test.go +++ b/pkg/systemd/generate/pods_test.go @@ -47,6 +47,7 @@ Description=Podman pod-123abc.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage Requires=container-1.service container-2.service Before=container-1.service container-2.service @@ -74,6 +75,7 @@ Description=Podman pod-123abc.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage Requires=container-1.service container-2.service Before=container-1.service container-2.service @@ -101,6 +103,7 @@ Description=Podman pod-123abc.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage Requires=container-1.service container-2.service Before=container-1.service container-2.service @@ -128,6 +131,7 @@ Description=Podman pod-123abc.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage Requires=container-1.service container-2.service Before=container-1.service container-2.service @@ -155,6 +159,7 @@ Description=Podman pod-123abc.service Documentation=man:podman-generate-systemd(1) Wants=network.target After=network-online.target +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage Requires=container-1.service container-2.service Before=container-1.service container-2.service @@ -191,6 +196,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 42, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "pod", "create", "--name", "foo", "bar=arg with space"}, }, @@ -208,6 +215,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 42, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "pod", "create", "--name", "foo", "bar=arg with space"}, }, @@ -225,6 +234,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 42, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "--events-backend", "none", "--runroot", "/root", "pod", "create", "--name", "foo", "bar=arg with space"}, }, @@ -242,6 +253,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 10, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "pod", "create", "--name", "foo", "bar=arg with space"}, }, @@ -259,6 +272,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 10, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "--events-backend", "none", "--runroot", "/root", "pod", "create", "--name", "foo", "bar=arg with space"}, }, @@ -276,6 +291,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 10, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "pod", "create", "--name", "foo", "--replace=false"}, }, @@ -293,6 +310,8 @@ WantedBy=multi-user.target default.target PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", StopTimeout: 10, PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", RequiredServices: []string{"container-1", "container-2"}, CreateCommand: []string{"podman", "pod", "create", "--name", "foo", "--label", "key={{someval}}"}, }, -- cgit v1.2.3-54-g00ecf From 1b349d79adc322b9df4a9c2cac49a4e42b301ef4 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Sun, 28 Mar 2021 16:26:31 +0200 Subject: podman generate systemd --new do not duplicate params podman generate systemd --new inserts extra idfile arguments. The generated unit can break when the user did provide their own idfile arguments as they overwrite the arguments added by generate systemd. This also happens when a user tries to generate the systemd unit on a container already create with a --new unit. This should now create a identical unit. The solution is to remove all user provided idfile arguments. This commit also ensures that we do not remove arguments that are part off the containers entrypoint. Fixes #9776 Signed-off-by: Paul Holzinger --- pkg/systemd/generate/common.go | 36 ++++++-- pkg/systemd/generate/common_test.go | 147 +++++++++++++++++++++++++++++--- pkg/systemd/generate/containers.go | 21 ++--- pkg/systemd/generate/containers_test.go | 91 ++++++++++++++++++++ pkg/systemd/generate/pods.go | 8 +- pkg/systemd/generate/pods_test.go | 19 +++++ 6 files changed, 290 insertions(+), 32 deletions(-) (limited to 'pkg/systemd/generate') diff --git a/pkg/systemd/generate/common.go b/pkg/systemd/generate/common.go index 19d468403..eafd45528 100644 --- a/pkg/systemd/generate/common.go +++ b/pkg/systemd/generate/common.go @@ -39,20 +39,46 @@ After=network-online.target RequiresMountsFor={{{{.GraphRoot}}}} {{{{.RunRoot}}}} ` -// filterPodFlags removes --pod and --pod-id-file from the specified command. -func filterPodFlags(command []string) []string { +// filterPodFlags removes --pod, --pod-id-file and --infra-conmon-pidfile from the specified command. +// argCount is the number of last arguments which should not be filtered, e.g. the container entrypoint. +func filterPodFlags(command []string, argCount int) []string { processed := []string{} - for i := 0; i < len(command); i++ { + for i := 0; i < len(command)-argCount; i++ { s := command[i] - if s == "--pod" || s == "--pod-id-file" { + if s == "--pod" || s == "--pod-id-file" || s == "--infra-conmon-pidfile" { i++ continue } - if strings.HasPrefix(s, "--pod=") || strings.HasPrefix(s, "--pod-id-file=") { + if strings.HasPrefix(s, "--pod=") || + strings.HasPrefix(s, "--pod-id-file=") || + strings.HasPrefix(s, "--infra-conmon-pidfile=") { continue } processed = append(processed, s) } + processed = append(processed, command[len(command)-argCount:]...) + return processed +} + +// filterCommonContainerFlags removes --conmon-pidfile, --cidfile and --cgroups from the specified command. +// argCount is the number of last arguments which should not be filtered, e.g. the container entrypoint. +func filterCommonContainerFlags(command []string, argCount int) []string { + processed := []string{} + for i := 0; i < len(command)-argCount; i++ { + s := command[i] + + switch { + case s == "--conmon-pidfile", s == "--cidfile", s == "--cgroups": + i++ + continue + case strings.HasPrefix(s, "--conmon-pidfile="), + strings.HasPrefix(s, "--cidfile="), + strings.HasPrefix(s, "--cgroups="): + continue + } + processed = append(processed, s) + } + processed = append(processed, command[len(command)-argCount:]...) return processed } diff --git a/pkg/systemd/generate/common_test.go b/pkg/systemd/generate/common_test.go index 3787e461e..30e758127 100644 --- a/pkg/systemd/generate/common_test.go +++ b/pkg/systemd/generate/common_test.go @@ -1,7 +1,6 @@ package generate import ( - "strings" "testing" "github.com/stretchr/testify/assert" @@ -9,22 +8,144 @@ import ( func TestFilterPodFlags(t *testing.T) { tests := []struct { - input []string + input []string + output []string + argCount int }{ - {[]string{"podman", "pod", "create"}}, - {[]string{"podman", "pod", "create", "--name", "foo"}}, - {[]string{"podman", "pod", "create", "--pod-id-file", "foo"}}, - {[]string{"podman", "pod", "create", "--pod-id-file=foo"}}, - {[]string{"podman", "run", "--pod", "foo"}}, - {[]string{"podman", "run", "--pod=foo"}}, + { + []string{"podman", "pod", "create"}, + []string{"podman", "pod", "create"}, + 0, + }, + { + []string{"podman", "pod", "create", "--name", "foo"}, + []string{"podman", "pod", "create", "--name", "foo"}, + 0, + }, + { + []string{"podman", "pod", "create", "--pod-id-file", "foo"}, + []string{"podman", "pod", "create"}, + 0, + }, + { + []string{"podman", "pod", "create", "--pod-id-file=foo"}, + []string{"podman", "pod", "create"}, + 0, + }, + { + []string{"podman", "pod", "create", "--pod-id-file", "foo", "--infra-conmon-pidfile", "foo"}, + []string{"podman", "pod", "create"}, + 0, + }, + { + []string{"podman", "pod", "create", "--pod-id-file", "foo", "--infra-conmon-pidfile=foo"}, + []string{"podman", "pod", "create"}, + 0, + }, + { + []string{"podman", "run", "--pod", "foo"}, + []string{"podman", "run"}, + 0, + }, + { + []string{"podman", "run", "--pod=foo"}, + []string{"podman", "run"}, + 0, + }, + { + []string{"podman", "run", "--pod=foo", "fedora", "podman", "run", "--pod=test", "alpine"}, + []string{"podman", "run", "fedora", "podman", "run", "--pod=test", "alpine"}, + 5, + }, + { + []string{"podman", "run", "--pod", "foo", "fedora", "podman", "run", "--pod", "test", "alpine"}, + []string{"podman", "run", "fedora", "podman", "run", "--pod", "test", "alpine"}, + 6, + }, + { + []string{"podman", "run", "--pod-id-file=foo", "fedora", "podman", "run", "--pod-id-file=test", "alpine"}, + []string{"podman", "run", "fedora", "podman", "run", "--pod-id-file=test", "alpine"}, + 5, + }, + { + []string{"podman", "run", "--pod-id-file", "foo", "fedora", "podman", "run", "--pod-id-file", "test", "alpine"}, + []string{"podman", "run", "fedora", "podman", "run", "--pod-id-file", "test", "alpine"}, + 6, + }, + } + + for _, test := range tests { + processed := filterPodFlags(test.input, test.argCount) + assert.Equal(t, test.output, processed) + } +} + +func TestFilterCommonContainerFlags(t *testing.T) { + tests := []struct { + input []string + output []string + argCount int + }{ + { + []string{"podman", "run", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--conmon-pidfile", "foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--conmon-pidfile=foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cidfile", "foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cidfile=foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cgroups", "foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cgroups=foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "alpine"}, + []string{"podman", "run", "alpine"}, + 1, + }, + { + []string{"podman", "run", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo", "alpine", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo"}, + []string{"podman", "run", "alpine", "--cgroups", "foo", "--conmon-pidfile", "foo", "--cidfile", "foo"}, + 7, + }, + { + []string{"podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "alpine", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo"}, + []string{"podman", "run", "alpine", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo"}, + 4, + }, } for _, test := range tests { - processed := filterPodFlags(test.input) - for _, s := range processed { - assert.False(t, strings.HasPrefix(s, "--pod-id-file")) - assert.False(t, strings.HasPrefix(s, "--pod")) - } + processed := filterCommonContainerFlags(test.input, test.argCount) + assert.Equal(t, test.output, processed) } } diff --git a/pkg/systemd/generate/containers.go b/pkg/systemd/generate/containers.go index bc13a6116..e06655a8d 100644 --- a/pkg/systemd/generate/containers.go +++ b/pkg/systemd/generate/containers.go @@ -238,13 +238,7 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst "--cidfile", "{{{{.ContainerIDFile}}}}", "--cgroups=no-conmon", ) - // If the container is in a pod, make sure that the - // --pod-id-file is set correctly. - if info.Pod != nil { - podFlags := []string{"--pod-id-file", "{{{{.Pod.PodIDFile}}}}"} - startCommand = append(startCommand, podFlags...) - info.CreateCommand = filterPodFlags(info.CreateCommand) - } + remainingCmd := info.CreateCommand[index:] // Presence check for certain flags/options. fs := pflag.NewFlagSet("args", pflag.ContinueOnError) @@ -254,7 +248,16 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst fs.BoolP("detach", "d", false, "") fs.String("name", "", "") fs.Bool("replace", false, "") - fs.Parse(info.CreateCommand[index:]) + fs.Parse(remainingCmd) + + remainingCmd = filterCommonContainerFlags(remainingCmd, fs.NArg()) + // If the container is in a pod, make sure that the + // --pod-id-file is set correctly. + if info.Pod != nil { + podFlags := []string{"--pod-id-file", "{{{{.Pod.PodIDFile}}}}"} + startCommand = append(startCommand, podFlags...) + remainingCmd = filterPodFlags(remainingCmd, fs.NArg()) + } hasDetachParam, err := fs.GetBool("detach") if err != nil { @@ -266,8 +269,6 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst return "", err } - remainingCmd := info.CreateCommand[index:] - if !hasDetachParam { // Enforce detaching // diff --git a/pkg/systemd/generate/containers_test.go b/pkg/systemd/generate/containers_test.go index 1359c1a37..899ba6bfa 100644 --- a/pkg/systemd/generate/containers_test.go +++ b/pkg/systemd/generate/containers_test.go @@ -392,6 +392,56 @@ ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id PIDFile=%t/jadda-jadda.pid Type=forking +[Install] +WantedBy=multi-user.target default.target +` + + goodNewWithIDFiles := `# 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 +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage + +[Service] +Environment=PODMAN_SYSTEMD_UNIT=%n +Restart=always +TimeoutStopSec=70 +ExecStartPre=/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 awesome-image:latest podman run --cgroups=foo --conmon-pidfile=foo --cidfile=foo alpine +ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10 +ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id +PIDFile=%t/jadda-jadda.pid +Type=forking + +[Install] +WantedBy=multi-user.target default.target +` + + goodNewWithPodIDFiles := `# 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 +RequiresMountsFor=/var/lib/containers/storage /var/run/containers/storage + +[Service] +Environment=PODMAN_SYSTEMD_UNIT=%n +Restart=always +TimeoutStopSec=70 +ExecStartPre=/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 %t/pod-foobar.pod-id-file -d awesome-image:latest podman run --cgroups=foo --conmon-pidfile=foo --cidfile=foo --pod-id-file /tmp/pod-foobar.pod-id-file alpine +ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10 +ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id +PIDFile=%t/jadda-jadda.pid +Type=forking + [Install] WantedBy=multi-user.target default.target ` @@ -782,6 +832,47 @@ WantedBy=multi-user.target default.target false, false, }, + {"good with ID files", + containerInfo{ + Executable: "/usr/bin/podman", + ServiceName: "jadda-jadda", + ContainerNameOrID: "jadda-jadda", + RestartPolicy: "always", + PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", + StopTimeout: 10, + PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", + CreateCommand: []string{"I'll get stripped", "create", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "awesome-image:latest", "podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "alpine"}, + EnvVariable: define.EnvVariable, + }, + goodNewWithIDFiles, + true, + false, + false, + }, + {"good with pod ID files", + containerInfo{ + Executable: "/usr/bin/podman", + ServiceName: "jadda-jadda", + ContainerNameOrID: "jadda-jadda", + RestartPolicy: "always", + PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", + StopTimeout: 10, + PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", + CreateCommand: []string{"I'll get stripped", "create", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "--pod", "test", "awesome-image:latest", "podman", "run", "--cgroups=foo", "--conmon-pidfile=foo", "--cidfile=foo", "--pod-id-file", "/tmp/pod-foobar.pod-id-file", "alpine"}, + EnvVariable: define.EnvVariable, + Pod: &podInfo{ + PodIDFile: "%t/pod-foobar.pod-id-file", + }, + }, + goodNewWithPodIDFiles, + true, + false, + false, + }, } for _, tt := range tests { test := tt diff --git a/pkg/systemd/generate/pods.go b/pkg/systemd/generate/pods.go index a76979ecf..1b92649e8 100644 --- a/pkg/systemd/generate/pods.go +++ b/pkg/systemd/generate/pods.go @@ -279,16 +279,16 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions) } podRootArgs = info.CreateCommand[1 : podCreateIndex-1] info.RootFlags = strings.Join(escapeSystemdArguments(podRootArgs), " ") - podCreateArgs = filterPodFlags(info.CreateCommand[podCreateIndex+1:]) + podCreateArgs = filterPodFlags(info.CreateCommand[podCreateIndex+1:], 0) } // We're hard-coding the first five arguments and append the // CreateCommand with a stripped command and subcommand. startCommand := []string{info.Executable} startCommand = append(startCommand, podRootArgs...) startCommand = append(startCommand, - []string{"pod", "create", - "--infra-conmon-pidfile", "{{{{.PIDFile}}}}", - "--pod-id-file", "{{{{.PodIDFile}}}}"}...) + "pod", "create", + "--infra-conmon-pidfile", "{{{{.PIDFile}}}}", + "--pod-id-file", "{{{{.PodIDFile}}}}") // Presence check for certain flags/options. fs := pflag.NewFlagSet("args", pflag.ContinueOnError) diff --git a/pkg/systemd/generate/pods_test.go b/pkg/systemd/generate/pods_test.go index 559f7365f..0e4d92c50 100644 --- a/pkg/systemd/generate/pods_test.go +++ b/pkg/systemd/generate/pods_test.go @@ -320,6 +320,25 @@ WantedBy=multi-user.target default.target false, false, }, + {"pod --new with ID files", + podInfo{ + Executable: "/usr/bin/podman", + ServiceName: "pod-123abc", + InfraNameOrID: "jadda-jadda-infra", + RestartPolicy: "on-failure", + PIDFile: "/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid", + StopTimeout: 10, + PodmanVersion: "CI", + GraphRoot: "/var/lib/containers/storage", + RunRoot: "/var/run/containers/storage", + RequiredServices: []string{"container-1", "container-2"}, + CreateCommand: []string{"podman", "pod", "create", "--infra-conmon-pidfile", "/tmp/pod-123abc.pid", "--pod-id-file", "/tmp/pod-123abc.pod-id", "--name", "foo", "bar=arg with space"}, + }, + podGoodNamedNew, + true, + false, + false, + }, } for _, tt := range tests { -- cgit v1.2.3-54-g00ecf