Implement --sdnotify cmdline option to control sd-notify behavior

[goochjj]

--sdnotify conmon-only|container|none

With "conmon-only", we send the MAINPID, and clear the NOTIFY_SOCKET so the OCI
runtime doesn't pass it into the container. We also advertise "ready" when the
OCI runtime finishes to advertise the service as ready.

With "container", we send the MAINPID, and leave the NOTIFY_SOCKET so the OCI
runtime passes it into the container for initialization, and let the container advertise further metadata.

The "none" option does what it's always done in the past - passes NOTIFY_SOCKET
to conmon's process and the OCI runtime processes, and does no manipulation or "help".

This removes the need for hardcoded CID and PID files in the command line, and
the PIDFile directive, as the pid is advertised directly through sd-notify.

Signed-off-by: Joseph Gooch [email protected]

Includes #6689
References #6688

[openshift-ci-robot]

Hi @goochjj. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[goochjj]

The more I think about this I'm not sure "none" is a valid option, though it was essentially requested... Since "none" returns a broken situation. (MAINPID never advertised properly)... Unless there's a case when runc or crun advertises the correct MAINPID.

[mheon]

I think none is "I am not being managed by systemd, please ignore" - we've seen cases where Podman is run as a child of something actually managed by systemd, inherits NOTIFY_SOCKET from the parent (arguably a bug in the parent, but still), and then blocks forever as it tries to notify on an already-used socket.

[goochjj]

I think none is "I am not being managed by systemd, please ignore" - we've seen cases where Podman is run as a child of something actually managed by systemd, inherits NOTIFY_SOCKET from the parent (arguably a bug in the parent, but still), and then blocks forever as it tries to notify on an already-used socket.

That's a reasonable interpretation. In that case I should probably block NOTIFY_SOCKET from being passed down to the OCI runtime and conmon... correct? That way everything will proceed as if the NOTIFY_SOCKET isn't there.

vs. what it does now, which means "libpod does nothing with NOTIFY_SOCKET other than pass it on", which is pretty similar to "container".

[goochjj]

I think none is "I am not being managed by systemd, please ignore" - we've seen cases where Podman is run as a child of something actually managed by systemd, inherits NOTIFY_SOCKET from the parent (arguably a bug in the parent, but still), and then blocks forever as it tries to notify on an already-used socket.

That's a reasonable interpretation. In that case I should probably block NOTIFY_SOCKET from being passed down to the OCI runtime and conmon... correct? That way everything will proceed as if the NOTIFY_SOCKET isn't there.

vs. what it does now, which means "libpod does nothing with NOTIFY_SOCKET other than pass it on", which is pretty similar to "container".

In which case I think "ignore" is a better description.

[mheon]

Ignore works for me - and yes, we should not forward the socket in that case

[rhatdan]

Doesn't Podman have to respond to the ignore and set the ready state for systemd if it sees the flag and is set to ignore? IE Systemd will never mark the unit file as ready unless someone says it is ready.

[goochjj]

Doesn't Podman have to respond to the ignore and set the ready state for systemd if it sees the flag and is set to ignore? IE Systemd will never mark the unit file as ready unless someone says it is ready.

That's true - however, currently Podman doesn't respond at all. Runc/crun do. Ignore is only relevant when some other service is the responsible party.

The default is "container", which is the current behavior, which would mean Runc/Crun need to respond. This PR just makes sure the correct MAINPID is set during the conversation.

conmon-only means Podman is the responsible party - the one responsible for sending READY=1, and keeps runc/crun out of the loop.

ignore would only be used if some other service is calling podman. I.e. some service that is sd-notify enabled, and therefore they are the responsible party. Contrived example:

#!/bin/bash

#I'm a service
systemd-notify --status="Starting up"

#Spawn some containers
podman run --sdnotify ignore --rm -d alpine sleep infinity
podman run --sdnotify ignore --rm -d alpine sleep infinity
podman run --sdnotify ignore --rm -d alpine sleep infinity

#Do other stuff
sleep 1

#And we're good
systemd-notify --ready

Right now, each podman would see the NOTIFY_SOCKET set and assume that it's responsible for telling systemd when it's ready... Which is incorrect, the overall service is the shell script.

The other alternative is people explicitly changing the podman lines to do:

env -u NOTIFY_SOCKET podman run --rm -d alpine sleep infinity

As that's really what ignore is doing.

[vrothberg]

This removes the need for hardcoded CID and PID files in the command line, and
the PIDFile directive, as the pid is advertised directly through sd-notify.

Can you elaborate why this would remove the CID file? I think that's still needed to cleanly stop/remove a container in a unit.

[vrothberg]

Reviewing now. @giuseppe PTAL as well

[vrothberg]

Oh man, I really like this PR. Thanks a ton!

Just a minor comment regarding input validation. @edsantiago , could you have a look as well? I wonder if you have a suggestion how we could test this in CI.

[mheon]

Once comments are addressed, this LGTM

[giuseppe]

good change! LGTM once the comments above are addressed

[goochjj]

This removes the need for hardcoded CID and PID files in the command line, and
the PIDFile directive, as the pid is advertised directly through sd-notify.

Can you elaborate why this would remove the CID file? I think that's still needed to cleanly stop/remove a container in a unit.

CID file can still be used, it's just SystemD doesn't need it so it doesn't need to be hanging out in /run/%n-cid, it'll revert to being in userdata/

PID file can still be used, it's just SystemD doesn't need it so it doesn't need to be hanging out in /run/%n.pid, it'll revert to being in userdata/.... And we don't need to specify it in PIDFile because systemd will receive the appropriate PID via MAINPID= w/ Type=notify.

If for whatever reason people still want to use Type=forking, they can, they'll need to keep the PIDFile directive and put it somewhere it can be found... Otherwise Type=notify just makes things more clean and less hardcoded.

[goochjj]

Oh man, I really like this PR. Thanks a ton!

Just a minor comment regarding input validation. @edsantiago , could you have a look as well? I wonder if you have a suggestion how we could test this in CI.

I too was struggling with this. I can envision the tests - as I did the testing manually, but it's going to require forking which is currently beyond my ability to do in golang.

We'd need a thread that opens a UNIX dgram socket and captures input. Perhaps (tmpdir)/notify.

1. Ensure podman ignores the socket
   Create notify socket
   Monitor notify socket
   set env NOTIFY_SOCKET
   Spawn podman run --rm --sd-notify=ignore alpine ls
   Verify podman returns exit 0
   Verify notify socket received absolutely no data.

2. Verify sd-notify=conmon (heres where it starts to get tricky)
   Create notify socket
   set env NOTIFY_SOCKET
   Proc1) Monitor notify socket
   Proc2) Spawn podman run --rm --sd-notify=conmon alpine ls
   Proc1) wait for receipt of MAINPID=(pid) and verify it points to conmon, descendent of Proc2
   Proc1) Wait for receipt of second MAINPID= and READY=1
   Proc2) verify exit 0

3. Verify sd-notify=container
   Create notify socket
   set env NOTIFY_SOCKET
   Proc1) Monitor notify socket
   Proc2) Spawn podman run --rm --sd-notify=container alpine ls
   Proc1) wait for receipt of MAINPID=(pid) and verify it points to conmon, descendent of Proc2
   Proc1) sleep 3 seconds
   Proc1) Find the socket - /run/runc/(CID)/notify/notify.sock or /run/crun/(CID)/notify/notify - and send READY=1 to it
   Proc1) Verify you get back MAINPID= and READY=1
   Proc2) verify exit 0

If podman exits in Proc2 before we send the READY in Proc1, then runc/crun didn't wait for a notification.

4. Test that not specifying sd-notify does the same thing as 3
5. Test that not setting NOTIFY_SOCKET does the same thing as 1

I'm not sure how to coordinate the test between two threads like that in Go.

[rhatdan]

@giuseppe PTAL

[mheon]

Two flakes, both on the Fedora repos. I'm just going to merge once this goes green.

/lgtm
/hold

[rhatdan]

/hold cancel

[rhatdan]

Thanks @goochjj Nice contribution.