From 7c54d142326b1602e2e5c55bec1bee39e7064177 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 13 Feb 2023 11:54:41 +0100 Subject: [PATCH] quadlet: add ExecStop Remove the container in ExecStop to make sure that Quadlet's adheres to Podman's customizable stop signal/timeout. Certain programs ignore SIGTERM which renders the services generated by Quadlet less user friendly compared to the ones from podman-generate-systemd. Previously, `systemctl stop` would just hang until systemd's stop timeout is hit. Since `podman rm` also removes the CID file, the additional `rm` can be removed. Note that `podman rm` will return immediately if the specified CID file isn't present. I am working on a short tutorial on Quadlet and hit the issue with a simple container running `sleep`. `sleep` ignores SIGTERM and stopping the service would take forever even with `PodmanArgs=--stop-timeout=0`. Signed-off-by: Valentin Rothberg --- pkg/systemd/quadlet/quadlet.go | 12 +++++++----- test/e2e/quadlet/basic.container | 3 ++- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/systemd/quadlet/quadlet.go b/pkg/systemd/quadlet/quadlet.go index 237139e68d..5ad6c05fe8 100644 --- a/pkg/systemd/quadlet/quadlet.go +++ b/pkg/systemd/quadlet/quadlet.go @@ -287,13 +287,15 @@ func ConvertContainer(container *parser.UnitFile, isUser bool) (*parser.UnitFile // Need the containers filesystem mounted to start podman service.Add(UnitGroup, "RequiresMountsFor", "%t/containers") - // If the conman exited uncleanly it may not have removed the container, so force it, - // -i makes it ignore non-existing files. + // If conmon exited uncleanly it may not have removed the container, so + // force it, -i makes it ignore non-existing files. + service.Add(ServiceGroup, "ExecStop", podmanBinary()+" rm -f -i --cidfile=%t/%N.cid") + // The ExecStopPost is needed when the main PID (i.e., conmon) gets killed. + // In that case, ExecStop is not executed but *Post only. If both are + // fired in sequence, *Post will exit when detecting that the --cidfile + // has already been removed by the previous `rm`.. service.Add(ServiceGroup, "ExecStopPost", "-"+podmanBinary()+" rm -f -i --cidfile=%t/%N.cid") - // Remove the cid file, to avoid confusion as the container is no longer running. - service.Add(ServiceGroup, "ExecStopPost", "-rm -f %t/%N.cid") - podman := NewPodmanCmdline("run") podman.addf("--name=%s", containerName) diff --git a/test/e2e/quadlet/basic.container b/test/e2e/quadlet/basic.container index 794ded61e3..3896f28c93 100644 --- a/test/e2e/quadlet/basic.container +++ b/test/e2e/quadlet/basic.container @@ -14,7 +14,8 @@ ## assert-key-is "Service" "Type" "notify" ## assert-key-is "Service" "NotifyAccess" "all" ## assert-key-is "Service" "SyslogIdentifier" "%N" -## assert-key-is-regex "Service" "ExecStopPost" "-.*/podman rm -f -i --cidfile=%t/%N.cid" "-rm -f %t/%N.cid" +## assert-key-is-regex "Service" "ExecStopPost" "-.*/podman rm -f -i --cidfile=%t/%N.cid" +## assert-key-is-regex "Service" "ExecStop" ".*/podman rm -f -i --cidfile=%t/%N.cid" ## assert-key-is "Service" "Environment" "PODMAN_SYSTEMD_UNIT=%n" [Container]