summaryrefslogtreecommitdiff
path: root/pkg
diff options
context:
space:
mode:
authorPaul Holzinger <paul.holzinger@web.de>2021-03-28 16:26:31 +0200
committerPaul Holzinger <paul.holzinger@web.de>2021-03-28 20:01:17 +0200
commitaabafc5b1b55b560c6444b15a7c18a3fa65993d9 (patch)
tree07a20c878d2f1cc3cc27cb0b268e5615ec070b8b /pkg
parent7e4b13b94699ee688c146afab727d2dec991376a (diff)
downloadpodman-aabafc5b1b55b560c6444b15a7c18a3fa65993d9.tar.gz
podman-aabafc5b1b55b560c6444b15a7c18a3fa65993d9.tar.bz2
podman-aabafc5b1b55b560c6444b15a7c18a3fa65993d9.zip
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 <paul.holzinger@web.de>
Diffstat (limited to 'pkg')
-rw-r--r--pkg/systemd/generate/common.go36
-rw-r--r--pkg/systemd/generate/common_test.go147
-rw-r--r--pkg/systemd/generate/containers.go21
-rw-r--r--pkg/systemd/generate/containers_test.go91
-rw-r--r--pkg/systemd/generate/pods.go8
-rw-r--r--pkg/systemd/generate/pods_test.go19
6 files changed, 290 insertions, 32 deletions
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
@@ -395,6 +395,56 @@ 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
+`
tests := []struct {
name string
info containerInfo
@@ -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 {