Skip to content

Commit

Permalink
Merge pull request #9848 from Luap99/fix-9776
Browse files Browse the repository at this point in the history 
podman generate systemd --new do not duplicate params
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Mar 29, 2021
2 parents 8e01f48 + aabafc5 commit 236943d
Show file tree
Hide file tree
Showing 6 changed files with 290 additions and 32 deletions.
36 changes: 31 additions & 5 deletions pkg/systemd/generate/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
147 changes: 134 additions & 13 deletions pkg/systemd/generate/common_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,30 +1,151 @@
package generate

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
)

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)
}
}

Expand Down
21 changes: 11 additions & 10 deletions pkg/systemd/generate/containers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -266,8 +269,6 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
return "", err
}

remainingCmd := info.CreateCommand[index:]

if !hasDetachParam {
// Enforce detaching
//
Expand Down
91 changes: 91 additions & 0 deletions pkg/systemd/generate/containers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
`
Expand Down Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions pkg/systemd/generate/pods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 236943d

Please sign in to comment.