Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generate systemd: do not set KillMode #8889

Merged
merged 1 commit into from
Jan 5, 2021
Merged

generate systemd: do not set KillMode #8889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/systemd/generate/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import (
// is set to the unit's (unique) name.
const EnvVariable = "PODMAN_SYSTEMD_UNIT"

// minTimeoutStopSec is the minimal stop timeout for generated systemd units.
// Once exceeded, processes of the services are killed and the cgroup(s) are
// cleaned up.
const minTimeoutStopSec = 60

// RestartPolicies includes all valid restart policies to be used in a unit
// file.
var RestartPolicies = []string{"no", "on-success", "on-failure", "on-abnormal", "on-watchdog", "on-abort", "always"}
Expand Down
6 changes: 5 additions & 1 deletion pkg/systemd/generate/containers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ type containerInfo struct {
ExecStartPre string
// ExecStart of the unit.
ExecStart string
// TimeoutStopSec of the unit.
TimeoutStopSec uint
// ExecStop of the unit.
ExecStop string
// ExecStopPost of the unit.
Expand All @@ -74,14 +76,14 @@ After={{- range $index, $value := .BoundToServices -}}{{if $index}} {{end}}{{ $v
[Service]
Environment={{.EnvVariable}}=%n
Restart={{.RestartPolicy}}
TimeoutStopSec={{.TimeoutStopSec}}
{{- if .ExecStartPre}}
ExecStartPre={{.ExecStartPre}}
{{- end}}
ExecStart={{.ExecStart}}
ExecStop={{.ExecStop}}
ExecStopPost={{.ExecStopPost}}
PIDFile={{.PIDFile}}
KillMode=none
Type=forking

[Install]
Expand Down Expand Up @@ -255,6 +257,8 @@ func executeContainerTemplate(info *containerInfo, options entities.GenerateSyst
info.ExecStopPost = "{{.Executable}} rm --ignore -f --cidfile {{.ContainerIDFile}}"
}

info.TimeoutStopSec = minTimeoutStopSec + info.StopTimeout

if info.PodmanVersion == "" {
info.PodmanVersion = version.Version.String()
}
Expand Down
43 changes: 21 additions & 22 deletions pkg/systemd/generate/containers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/stretchr/testify/assert"
)

func TestValidateRestartPolicyContainer(t *testing.T) {
Expand Down Expand Up @@ -48,11 +49,11 @@ After=network-online.target
[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=always
TimeoutStopSec=82
ExecStart=/usr/bin/podman start 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401
ExecStop=/usr/bin/podman stop -t 10 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401
ExecStopPost=/usr/bin/podman stop -t 10 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401
ExecStop=/usr/bin/podman stop -t 22 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401
ExecStopPost=/usr/bin/podman stop -t 22 639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401
PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid
KillMode=none
Type=forking

[Install]
Expand All @@ -71,11 +72,11 @@ After=network-online.target
[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=always
TimeoutStopSec=70
ExecStart=/usr/bin/podman start foobar
ExecStop=/usr/bin/podman stop -t 10 foobar
ExecStopPost=/usr/bin/podman stop -t 10 foobar
PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid
KillMode=none
Type=forking

[Install]
Expand All @@ -96,11 +97,11 @@ After=a.service b.service c.service pod.service
[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=always
TimeoutStopSec=70
ExecStart=/usr/bin/podman start foobar
ExecStop=/usr/bin/podman stop -t 10 foobar
ExecStopPost=/usr/bin/podman stop -t 10 foobar
PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid
KillMode=none
Type=forking

[Install]
Expand All @@ -119,12 +120,12 @@ After=network-online.target
[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 --replace --name jadda-jadda --hostname hello-world awesome-image:latest command arg1 ... argN "foo=arg \"with \" space"
ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 42
ExecStop=/usr/bin/podman stop --ignore --cidfile %t/jadda-jadda.ctr-id -t 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have TimeoutStopSec now, what's the point of leaving ExecStop then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want Podman to take care of stopping and to clean up properly. TimeoutStopSec is the big hammer of systemd to nuke everything in the cgroup and may leave resources behind.

ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/jadda-jadda.ctr-id
PIDFile=%t/jadda-jadda.pid
KillMode=none
Type=forking

[Install]
Expand All @@ -143,12 +144,12 @@ After=network-online.target
[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 --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
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
KillMode=none
Type=forking

[Install]
Expand All @@ -167,12 +168,12 @@ After=network-online.target
[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 /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
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
KillMode=none
Type=forking

[Install]
Expand All @@ -191,12 +192,12 @@ After=network-online.target
[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 --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
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
KillMode=none
Type=forking

[Install]
Expand All @@ -215,12 +216,12 @@ After=network-online.target
[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=always
TimeoutStopSec=70
ExecStartPre=/bin/rm -f %t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid %t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.ctr-id
ExecStart=/usr/bin/podman run --conmon-pidfile %t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid --cidfile %t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.ctr-id --cgroups=no-conmon -d awesome-image:latest
ExecStop=/usr/bin/podman stop --ignore --cidfile %t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.ctr-id -t 10
ExecStopPost=/usr/bin/podman rm --ignore -f --cidfile %t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.ctr-id
PIDFile=%t/container-639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401.pid
KillMode=none
Type=forking

[Install]
Expand All @@ -242,7 +243,7 @@ WantedBy=multi-user.target default.target
ContainerNameOrID: "639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401",
RestartPolicy: "always",
PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
StopTimeout: 10,
StopTimeout: 22,
PodmanVersion: "CI",
EnvVariable: EnvVariable,
},
Expand Down Expand Up @@ -302,7 +303,7 @@ WantedBy=multi-user.target default.target
ContainerNameOrID: "jadda-jadda",
RestartPolicy: "always",
PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
StopTimeout: 42,
StopTimeout: 10,
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: EnvVariable,
Expand All @@ -318,7 +319,7 @@ WantedBy=multi-user.target default.target
ContainerNameOrID: "jadda-jadda",
RestartPolicy: "always",
PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
StopTimeout: 42,
StopTimeout: 10,
PodmanVersion: "CI",
CreateCommand: []string{"I'll get stripped", "container", "run", "-d", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
EnvVariable: EnvVariable,
Expand All @@ -334,7 +335,7 @@ WantedBy=multi-user.target default.target
ContainerNameOrID: "jadda-jadda",
RestartPolicy: "always",
PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
StopTimeout: 42,
StopTimeout: 10,
PodmanVersion: "CI",
CreateCommand: []string{"I'll get stripped", "container", "run", "-d", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
EnvVariable: EnvVariable,
Expand All @@ -353,7 +354,7 @@ WantedBy=multi-user.target default.target
ContainerNameOrID: "jadda-jadda",
RestartPolicy: "always",
PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
StopTimeout: 42,
StopTimeout: 10,
PodmanVersion: "CI",
CreateCommand: []string{"I'll get stripped", "container", "run", "--detach", "--name", "jadda-jadda", "--hostname", "hello-world", "awesome-image:latest", "command", "arg1", "...", "argN"},
EnvVariable: EnvVariable,
Expand Down Expand Up @@ -390,9 +391,7 @@ WantedBy=multi-user.target default.target
t.Errorf("CreateContainerSystemdUnit() error = \n%v, wantErr \n%v", err, test.wantErr)
return
}
if got != test.want {
t.Errorf("CreateContainerSystemdUnit() = \n%v\n---------> want\n%v", got, test.want)
}
assert.Equal(t, test.want, got)
})
}
}
6 changes: 5 additions & 1 deletion pkg/systemd/generate/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ type podInfo struct {
ExecStartPre2 string
// ExecStart of the unit.
ExecStart string
// TimeoutStopSec of the unit.
TimeoutStopSec uint
// ExecStop of the unit.
ExecStop string
// ExecStopPost of the unit.
Expand All @@ -72,6 +74,7 @@ Before={{- range $index, $value := .RequiredServices -}}{{if $index}} {{end}}{{
[Service]
Environment={{.EnvVariable}}=%n
Restart={{.RestartPolicy}}
TimeoutStopSec={{.TimeoutStopSec}}
{{- if .ExecStartPre1}}
ExecStartPre={{.ExecStartPre1}}
{{- end}}
Expand All @@ -82,7 +85,6 @@ ExecStart={{.ExecStart}}
ExecStop={{.ExecStop}}
ExecStopPost={{.ExecStopPost}}
PIDFile={{.PIDFile}}
KillMode=none
Type=forking

[Install]
Expand Down Expand Up @@ -298,6 +300,8 @@ func executePodTemplate(info *podInfo, options entities.GenerateSystemdOptions)
info.ExecStop = "{{.Executable}} pod stop --ignore --pod-id-file {{.PodIDFile}} {{if (ge .StopTimeout 0)}}-t {{.StopTimeout}}{{end}}"
info.ExecStopPost = "{{.Executable}} pod rm --ignore -f --pod-id-file {{.PodIDFile}}"
}
info.TimeoutStopSec = minTimeoutStopSec + info.StopTimeout

if info.PodmanVersion == "" {
info.PodmanVersion = version.Version.String()
}
Expand Down
15 changes: 7 additions & 8 deletions pkg/systemd/generate/pods_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/containers/podman/v2/pkg/domain/entities"
"github.com/stretchr/testify/assert"
)

func TestValidateRestartPolicyPod(t *testing.T) {
Expand Down Expand Up @@ -50,11 +51,11 @@ Before=container-1.service container-2.service
[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=always
TimeoutStopSec=102
ExecStart=/usr/bin/podman start jadda-jadda-infra
ExecStop=/usr/bin/podman stop -t 10 jadda-jadda-infra
ExecStopPost=/usr/bin/podman stop -t 10 jadda-jadda-infra
ExecStop=/usr/bin/podman stop -t 42 jadda-jadda-infra
ExecStopPost=/usr/bin/podman stop -t 42 jadda-jadda-infra
PIDFile=/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid
KillMode=none
Type=forking

[Install]
Expand All @@ -75,13 +76,13 @@ Before=container-1.service container-2.service
[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=on-failure
TimeoutStopSec=70
ExecStartPre=/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 "bar=arg with space" --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]
Expand All @@ -102,7 +103,7 @@ WantedBy=multi-user.target default.target
InfraNameOrID: "jadda-jadda-infra",
RestartPolicy: "always",
PIDFile: "/var/run/containers/storage/overlay-containers/639c53578af4d84b8800b4635fa4e680ee80fd67e0e6a2d4eea48d1e3230f401/userdata/conmon.pid",
StopTimeout: 10,
StopTimeout: 42,
PodmanVersion: "CI",
RequiredServices: []string{"container-1", "container-2"},
},
Expand Down Expand Up @@ -139,9 +140,7 @@ WantedBy=multi-user.target default.target
t.Errorf("CreatePodSystemdUnit() error = \n%v, wantErr \n%v", err, test.wantErr)
return
}
if got != test.want {
t.Errorf("CreatePodSystemdUnit() = \n%v\n---------> want\n%v", got, test.want)
}
assert.Equal(t, test.want, got)
})
}
}
3 changes: 3 additions & 0 deletions test/e2e/generate_systemd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ var _ = Describe("Podman generate systemd", func() {

found, _ := session.GrepString(" stop -t 1234 ")
Expect(found).To(BeTrue())

found, _ = session.GrepString("TimeoutStopSec=1294")
Expect(found).To(BeTrue())
})

It("podman generate systemd", func() {
Expand Down