podman.service: use sdnotify

[vrothberg]

Commit 2b6dd3f set the killmode of the podman.service to the
systemd default which ultimately lead to the problem that systemd
will kill all processes inside the unit's cgroup and hence kill
all containers whenever the service is stopped.

Fix it by setting the type to sdnotify and the killmode to process.
podman system service will send the necessary notify messages
when the SDNOTIFY_SOCKET is set and unset it right after to prevent
the backend and container runtimes from jumping in between and send
messages as well.

Fixes: #7294
Signed-off-by: Valentin Rothberg rothberg@redhat.com

[vrothberg]

@martinpitt @goochjj PTAL

[vrothberg]

/hold

[vrothberg]

This looks good to me now. Podman will now be stopped as systemd knows it's PID. Also systemd is not complaining about old conmons hanging around but it actually reports them. Below you can see what I'm referring to. I ran a bunch of containers, waited for the service to timeout and then did an image list to fire up the service again:

[root@nebuchadnezzar libpod]# systemctl status podman.service
● podman.service - Podman API Service
     Loaded: loaded (/usr/local/lib/systemd/system/podman.service; enabled; vendor preset: disabled)
     Active: active (running) since Thu 2020-08-13 13:18:14 CEST; 3s ago
TriggeredBy: ● podman.socket
       Docs: man:podman-system-service(1)
   Main PID: 1198502 (podman)
      Tasks: 27 (limit: 19009)
     Memory: 39.0M
        CPU: 191ms
     CGroup: /system.slice/podman.service
             ├─1197016 /usr/bin/conmon --api-version 1 -c b3ef44b91d1108f23c04f19b61b59c8ec1d6f57918ccca6a84bc64bf86eac834 
             ├─1197240 /usr/bin/conmon --api-version 1 -c fb6e0ae46293a97bf166dc3a76e17f549b949b28a6d66601ca27b220ce9431ec 
             ├─1197955 /usr/bin/conmon --api-version 1 -c da360f41e2d854e4c12b0f061b230c04a5f4f7e1abc408e60ef15ec1efc57c9c 
             ├─1198029 /usr/bin/conmon --api-version 1 -c d931f5e0bea1dc2acad2c1c32bedad7cc5b5571d4b5048a1f5d5fbae2ba76d60 
             ├─1198096 /usr/bin/conmon --api-version 1 -c 1b84665fa739d408146bfbf03ab775c9bc18c7b648e13d848bd832dcfb432312 
             ├─1198162 /usr/bin/conmon --api-version 1 -c 1ba81c25316fb91e2f585969a6cc6a2df4f9352fa3e44fa931fe8981f5f89308 
             ├─1198229 /usr/bin/conmon --api-version 1 -c aaabfd54a0fd3e3513ab91c73aaf91e9184df9f420150c1b2e9cd5a2a013d893 
             └─1198502 /usr/local/bin/podman system service

[rhatdan]

/lgtm

[mheon]

LGTM

[vrothberg]

Please wait with merging until we have more acks on it.

[rhatdan]

I thought you had two. But you have control anyways.

[giuseppe]

This looks good to me now. Podman will now be stopped as systemd knows it's PID. Also systemd is not complaining about old conmons hanging around but it actually reports them. Below you can see what I'm referring to. I ran a bunch of containers, waited for the service to timeout and then did an image list to fire up the service again:

why do we have conmon processes in the podman.service cgroup? I thought we moved them to a separate cgroup

[vrothberg]

why do we have conmon processes in the podman.service cgroup? I thought we moved them to a separate cgroup

Could it be because we're unsetting NOTIFY_SOCKET?

[giuseppe]

Could it be because we're unsetting NOTIFY_SOCKET?

that should not happen (at least I don't see we are using it internally in Podman). I don't think the issue is introduced with this change. Or are those old containers that are configured with --cgroup-mode=split?

[vrothberg]

Could it be because we're unsetting NOTIFY_SOCKET?

that should not happen (at least I don't see we are using it internally in Podman). I don't think the issue is introduced with this change. Or are those old containers that are configured with --cgroup-mode=split?

They are created with the remote client. Maybe we're missing that in the remote code paths?

[giuseppe]

They are created with the remote client. Maybe we're missing that in the remote code paths?

this patch seems to do the trick:

diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go
index 18b48a3f6..2a3e44e04 100644
--- a/pkg/api/server/server.go
+++ b/pkg/api/server/server.go
@@ -153,6 +153,7 @@ func (s *APIServer) Serve() error {
        signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)
        errChan := make(chan error, 1)
 
+       os.Unsetenv("INVOCATION_ID")
        go func() {
                <-s.idleTracker.Done()
                logrus.Debugf("API Server idle for %v", s.idleTracker.Duration)

[vrothberg]

this patch seems to do the trick:

Tested successfully, thanks @giuseppe !

[vrothberg]

Allright, now as we've ironed out the last remaining fart, I am good to merge. Thanks all!

[giuseppe]

LGTM

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[goochjj]

LGTM

[TomSweeneyRedHat]

The change LGTM, but it looks like you may have encountered a real error in the tests.

[mheon]

F31 looks like one of our remote flakes...

/lgtm
/hold

[edsantiago]

yes, it's the #7195 flake

[TomSweeneyRedHat]

Thanks, @QiWang19 's #7311 suffers the same issue.

[edsantiago]

/hold

investigating something. Please do not merge yet.

[edsantiago]

All clear. The problem I was looking at turned out to be caused by something else.

[rhatdan]

/hold cancel