simplify podman systemd generate - remove cidfile

[grooverdan]

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind feature

Description

podman systemd generate creates an ExecStartPre to remove a cidfile, and ExecStart line that include a cid file, and an ExecStop that uses it, and an ExecStopPost that removes it.

For a Type=notify with the MAINPID of the conman pushed (per comment #12778 (comment) / #9642) all of these usages of cidfile are not needed.

The Type=notify with an accurately communicated pid, and conman acting on all signals to shutdown the container means all the cidfile related directives can be removed.

Steps to reproduce the issue:

1. bin/podman generate systemd --name --new --template {container}

Describe the results you received:

$ bin/podman generate systemd --name --new --template quizzical_satoshi
# container-quizzical_satoshi@.service
# autogenerated by Podman 4.0.0-dev
# Tue Feb 15 10:04:40 AEDT 2022

[Unit]
Description=Podman container-quizzical_satoshi.service for %I
Documentation=man:podman-generate-systemd(1)
Wants=network-online.target
After=network-online.target
RequiresMountsFor=%t/containers

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n-%i
Restart=on-failure
TimeoutStopSec=70
ExecStartPre=/bin/rm -f %t/%n.ctr-id
ExecStart=/home/dan/repos/podman/bin/podman run --name=container-quizzical_satoshi-%i --cidfile=%t/%n.ctr-id --cgroups=no-conmon --rm --sdnotify=conmon -d -ti fedora:35
ExecStop=/home/dan/repos/podman/bin/podman stop --ignore --cidfile=%t/%n.ctr-id
ExecStopPost=/home/dan/repos/podman/bin/podman rm -f --ignore --cidfile=%t/%n.ctr-id
Type=notify
NotifyAccess=all

[Install]
WantedBy=default.target

Describe the results you expected:

$ bin/podman generate systemd --name --new --template quizzical_satoshi
# container-quizzical_satoshi@.service
# autogenerated by Podman 4.0.0-dev
# Tue Feb 15 10:04:40 AEDT 2022

[Unit]
Description=Podman container-quizzical_satoshi.service for %I
Documentation=man:podman-generate-systemd(1)
Wants=network-online.target
After=network-online.target
RequiresMountsFor=%t/containers

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n-%i
Restart=on-failure
TimeoutStopSec=70
ExecStart=/home/dan/repos/podman/bin/podman run --name=container-quizzical_satoshi-%i --cgroups=no-conmon --rm --sdnotify=conmon -d -ti fedora:35
Type=notify
NotifyAccess=all

[Install]
WantedBy=default.target

Additional information you deem important (e.g. issue happens only occasionally):

Output of podman version:

$ bin/podman version
Client:       Podman Engine
Version:      4.0.0-dev
API Version:  4.0.0-dev
Go Version:   go1.16.13
Git Commit:   5977fd509582d6dc8727ce8f8a78011888a1dc17-dirty
Built:        Tue Feb 15 10:03:25 2022
OS/Arch:      linux/amd64

Package info (e.g. output of rpm -q podman or apt list podman):

built from source (today)

Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide? (https://github.com/containers/podman/blob/main/troubleshooting.md)

Yes

Additional environment details (AWS, VirtualBox, physical, etc.):

Discovered investigating @eriksjolund's good use of systemd, podman and socket activation examples eriksjolund/mariadb-podman-socket-activation#1

[grooverdan]

With ExecStop removed TimeoutStopSec=70 is probably not needed either. The default KillMode=control-group should given the container and conman the fair chance to cleanly terminate. A systemd aware entry point can also EXTEND_TIMEOUT_USEC if it so desires.

[vrothberg]

Thanks for opening the issue, @grooverdan! Your suggestion sounds good to me. One thing we'd need is the --replace flag in podman run to be sure that name-conflicting containers are removed.

[Luap99]

Please be aware of the consequences. We had this before and it did not work correctly, #11315

There are a number of problems when we do not use podman stop:

1. we have to set the correct stop signal (can be fixed in the unit)
2. we need to use the correct stop timeout (can be fixed in the unit)
3. This is the biggest problem. If conmon exits systemd will think the unit is done and starts killing everything including the cleanup process so we now will leak mounts, network interfaces, etc...

[vrothberg]

This is the biggest problem. If conmon exits systemd will think the unit is done and starts killing everything including the cleanup process so we now will leak mounts, network interfaces, etc...

Very fair point, I didn't consider this yet. The Podman clean-up process created by conmon should probably run outside the unit's cgroup.

[Luap99]

Also see #11304 (comment)

[grooverdan]

Right so, https://github.com/containers/conmon/blob/e2215a1c4c01c25f2fc1206ad4df012d10374b99/src/ctr_exit.c#L222 as the SIGTERM handler should reap_children? I don't think the sdnotify setting should alter the behaviour here.

So addressing the list:

1. This should be the handler for all signals that by default terminate(?)
2. If we wait for the children, the timeout is handled too(?).
3. SendSIGKILL=no to prevent the undue tear-down of conmon.

[vrothberg]

The Podman clean-up process created by conmon should probably run outside the unit's cgroup.

I think this should be enough. @giuseppe WDYT?

[Luap99]

I don't think so, if you have a process that does not respond to sigterm, systemd will wait the timout and then send sigkill to the main pid conmon. This will cause conmon to exit but the container process will keep running AFAICT. The cleanup process will never be started.

We can also not use SendSIGKILL=no because otherwise process will never be terminated if sigterm is ignored
Just test with this unit file:

[Unit]
Documentation=man:podman-generate-systemd(1)
Wants=network-online.target
After=network-online.target
RequiresMountsFor=%t/containers

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=on-failure
TimeoutStopSec=10
ExecStart=podman run --name=testcon --cgroups=no-conmon --rm --sdnotify=conmon -d alpine sleep inf
Type=notify
NotifyAccess=all

[Install]
WantedBy=default.target

[vrothberg]

We can also not use SendSIGKILL=no because otherwise process will never be terminated if sigterm is ignored

I concur. We need systemd to be able to nuke when needed.

[giuseppe]

The Podman clean-up process created by conmon should probably run outside the unit's cgroup.

I think this should be enough. @giuseppe WDYT?

how can we do that? Create a new systemd scope?

[vrothberg]

how can we do that? Create a new systemd scope?

Do you think that could work?

[Luap99]

But this would be to late, no? conmon has to spawn the cleanup process but if conmon is killed with sigkill this is not possible.

[giuseppe]

could we move the cleanup to a ExecStopPost action?