sd-notify: do not hang when NOTIFY_SOCKET is used with create #1807

giuseppe · 2018-05-25T16:15:14Z

if NOTIFY_SOCKET is used, do not block the main runc process waiting for events on the notify socket. Bind mount the parent directory of the notify socket, so that "start" can create the socket and it is still accessible from the container.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

giuseppe · 2018-05-25T16:16:24Z

/cc @rhatdan @mrunalp

dqminh · 2018-06-02T22:53:40Z

@giuseppe can you describe a minimal test case for this ? Would be nice to setup an integration test too.

With this, do we even need to pass notifySocket to signalHandler anymore ?

giuseppe · 2018-06-03T17:17:26Z

@dqminh currently if the NOTIFY_SOCKET environment variable is set for "runc create" then runc just hangs indefinitely.
signalHandler.forward() is still the caller for the new process. Would you prefer to move it somewhere else?

rhatdan · 2018-06-04T12:59:27Z

@giuseppe Couldn't you move the wait for ever functionality out of the run and into the start?

giuseppe · 2018-06-04T13:23:13Z

I cannot do that as the socket needs to be opened by create so that it can be bind mounted into the container. We would need to unlink it before creating it again with start (and the container would not see it)

rhatdan · 2018-06-20T20:55:08Z

@dqminh We are waiting for reply from you.
@avagin @caniszczyk @crosbymichael @cyphar @hqhq, @mrunalp @rjnagal @vmarmol PTAL

Would love to get podman support for sd-notify so we can run podman/runc with sd-notify containers.

mrunalp · 2018-07-09T17:33:28Z

The cost of the extra process falls on the users of the socket so I am fine with this as there is no simpler way.

mrunalp · 2018-07-09T17:33:33Z

LGTM

rhatdan · 2018-07-09T19:14:16Z

@dqminh @avagin @caniszczyk @crosbymichael @cyphar @hqhq, @rjnagal @vmarmol PTAL

rhatdan · 2018-07-27T15:06:03Z

Could someone @dqminh @avagin @caniszczyk @crosbymichael @cyphar @hqhq, @rjnagal @vmarmol PTAL???

dqminh · 2018-08-05T22:38:25Z

Hi, sorry for the delay !

Having to spawn one more process is unfortunate, but i cant think of a better way if we want to keep the same flow where the forwarding of notification is setup when we first create the container. (However in this case perhaps we should not overload init and instead give the process a separate operation ).

Another way i can think of is to have Create just handling the mounting of socket in host to socket in container and setup correct env variable (perhaps set MAINPID ?), but dont actually start handling readiness notification at all. Instead we defer the handling of notifications' forwarding to Start. So Start will be something like

read state of the container if required ( path to notify socket on the host needs to be here somewhere)
if notify socket path exists, then start listener
notify the container process that it should start executing
start forwarding messages

In this case, NOTIFY_SOCKET=something runc create will be completed immediately, but runc start of such container will also act as the sd_notify forwarder hence will stay alive until such forwarding completed. Basically a change in behavior, but we dont need to execute another long-live process without context attached.

Does that make sense ?

giuseppe · 2018-08-06T12:09:11Z

what stopped me from doing that in the first iteration was that we cannot create the socket once the container is already created (and the socket bind mounted), so that the listener had to be created from create.
I've done an additional change now, where we bind mount the parent directory of the socket, so we can create the socket later.
With this change we lose the ability to keep in the container the same file name specified in the host NOTIFY_SOCKET, in the container it is always /run/notify/notify.sock but I don't think it really matters for the applications.

dqminh · 2018-08-06T17:34:20Z

signals.go

@@ -70,7 +70,7 @@ func (h *signalHandler) forward(process *libcontainer.Process, tty *tty, detach
 			h.notifySocket.run(pid1)
 			return 0, nil
 		} else {
-			go h.notifySocket.run(0)
+			h.notifySocket.run(os.Getpid())


question: does it matter here that we set PID to runc process ? Cant we always set it to the container's process instead ?

if we keep runc running, the container's process won't be a direct child of systemd and systemd won't get the SIGCHLD once it terminates. Differently, it will get the SIGCHLD for the runc process and can link it to the service being terminated.

dqminh · 2018-08-06T17:35:09Z

@giuseppe Thanks ! This looks great and much simpler than before.

giuseppe · 2018-08-09T07:35:38Z

does the new version look fine? @dqminh @mrunalp

TomSweeneyRedHat · 2018-08-16T21:30:38Z

Re-ping @dqminh @mrunalp . This is part of the fix for a Podman issue containers/podman#746

giuseppe · 2018-08-27T07:32:55Z

ping, can this be finally reviewed?

crosbymichael · 2018-08-29T19:46:27Z

I don't know enough about this to be able to give a good review or even test. Someone other maintainers will have to as I wouldn't feel comfortable either way. Sorry.

crawford · 2018-10-02T23:15:05Z

What does this need in order to be merged? I'd love to have sdnotify support and I'm happy to help out with this PR where I can.

mrunalp · 2018-10-02T23:52:19Z

LGTM

mrunalp · 2018-10-02T23:52:30Z

@dqminh Could you take a look?

giuseppe · 2019-02-22T22:03:46Z

can someone please take a look?

filbranden · 2019-03-15T02:53:47Z

@giuseppe In #2014 I'm adding support for integration tests in an environment where systemd is running and also a Travis-CI job to run those tests on PRs.

Do you think you might be able to add an integration test that would test the NOTIFY_SOCKET feature, perhaps on top of that PR?

I guess having a test case pass would make it easier to get this merged.

Cheers!
Filipe

giuseppe · 2019-03-15T16:27:38Z

@filbranden thanks for the suggestion. I've looked into it but I don't see any way for using UNIX datagram sockets from a busybox container so that I could communicate back with the systemd socket

filbranden · 2019-03-15T17:22:44Z

@giuseppe Yes busybox will probably not be enough... Can we consider a slightly larger minimal image that would have enough tools (like a somewhat more complete nc) to perform the test? Something based on Alpine perhaps?

Or, alternatively, can we consider copying a binary into the unpacked tree of busybox image before running the runc test on it?

Let me know if I can help in any way.

PR #2014 has been merged, so rebasing on top of latest head should already get you the tests including systemd and the Travis CI config to run them as part of every pending PR...

Cheers!
Filipe

giuseppe · 2019-03-15T18:36:47Z

yes, using Alpine will help, then I can run the equivalent of:

$ systemd-run --unit foo --service-type=notify -p NotifyAccess=all podman run --rm alpine sh -c 'apk add nmap-ncat; echo READY=1 | ncat -u -U $NOTIFY_SOCKET ; sleep 100'
$ systemctl is-active foo

giuseppe · 2020-02-17T11:50:14Z

The PR was opened almost 2 years ago.

Since there is no interest in fixing the issue, I am closing it.

saschagrunert · 2020-03-11T13:40:21Z

The PR was opened almost 2 years ago.

Since there is no interest in fixing the issue, I am closing it.

Hey @giuseppe, now is the time where we're also interested in fixing this bug. May I ask you to re-open the issue that we can finally merge it? 😇

rhatdan · 2020-03-11T13:45:06Z

Yes @giuseppe Let's reopen and see if we can get @AkihiroSuda to review.

AkihiroSuda · 2020-03-11T13:59:33Z

Does crun already have this? Can we have an integration test?

notify_socket.go

giuseppe · 2020-03-11T14:18:38Z

Does crun already have this? Can we have an integration test?

yes crun already supports it. As well as the runc distributed by Fedora/RHEL, where we are backporting this patch.

notify_socket.go

if NOTIFY_SOCKET is used, do not block the main runc process waiting for events on the notify socket. Bind mount the parent directory of the notify socket, so that "start" can create the socket and it is still accessible from the container. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

AkihiroSuda · 2020-03-12T20:23:37Z

LGTM, but I'd like to see integration test suite if possible. Can be follow-up PR if difficult.

mrunalp · 2020-03-13T15:04:17Z

LGTM

* Running the etcd container with NOTIFY_SOCKET mounted (to use systemd Type=notify) causes podman to hang so for now just use exec * opencontainers/runc#1807

runc and podman to work with sd_notify

giuseppe mentioned this pull request May 25, 2018

Running containers inside of a container environment (with docker-compose.yml) using podman? containers/podman#746

Closed

giuseppe force-pushed the notify-no-block branch from 0cd0d2c to ecf53c2 Compare June 3, 2018 17:15

giuseppe force-pushed the notify-no-block branch 2 times, most recently from db96894 to cd9b959 Compare August 6, 2018 12:03

dqminh reviewed Aug 6, 2018

View reviewed changes

crawford mentioned this pull request Oct 2, 2018

Revert "templates: change etcd-member to notify type" openshift/machine-config-operator#106

Merged

crawford mentioned this pull request Oct 2, 2018

deployment on libvirt hanging forever openshift/installer#393

Closed

crawford mentioned this pull request Oct 15, 2018

asset/ignition: add event on bootstrap completion openshift/installer#459

Merged

crawford mentioned this pull request Oct 25, 2018

templates: switch kubelet systemd unit to type notify openshift/machine-config-operator#146

Merged

giuseppe force-pushed the notify-no-block branch from cd9b959 to 7ae4c8d Compare November 10, 2018 20:38

giuseppe mentioned this pull request Feb 13, 2019

Reenable two skipped tests in run_e2e.go containers/podman#2328

Closed

giuseppe force-pushed the notify-no-block branch from a52f7bf to e3b3789 Compare March 15, 2019 17:59

giuseppe force-pushed the notify-no-block branch from e3b3789 to 3d99c51 Compare October 3, 2019 18:57

giuseppe closed this Feb 17, 2020

AkihiroSuda reopened this Mar 11, 2020

AkihiroSuda added the kind/bug label Mar 11, 2020

AkihiroSuda reviewed Mar 11, 2020

View reviewed changes

notify_socket.go Outdated Show resolved Hide resolved

giuseppe force-pushed the notify-no-block branch from 3d99c51 to e6e99e9 Compare March 12, 2020 09:56

AkihiroSuda reviewed Mar 12, 2020

View reviewed changes

notify_socket.go Show resolved Hide resolved

AkihiroSuda reviewed Mar 12, 2020

View reviewed changes

notify_socket.go Show resolved Hide resolved

giuseppe force-pushed the notify-no-block branch from e6e99e9 to 25fd4a6 Compare March 12, 2020 20:21

mrunalp merged commit 167e33c into opencontainers:master Mar 13, 2020

smithfarm mentioned this pull request Mar 20, 2020

cephadm bootstap hangs in mgr dump ceph/ceph-salt#130

Closed

fazlk28 mentioned this pull request Feb 15, 2021

FATAL failed waiting for Kubernetes API: Get "https://api.milan46.conlab.ocp:6443/version?timeout=32s": Service Unavailable openshift/installer#4643

Closed

liuyangxy pushed a commit to fedora-riscv/runc that referenced this pull request May 13, 2023

Add patch opencontainers/runc#1807 to allow

4625afd

runc and podman to work with sd_notify

liuyangxy pushed a commit to fedora-riscv/runc that referenced this pull request May 13, 2023

Add patch opencontainers/runc#1807 to allow

b9e53c1

runc and podman to work with sd_notify

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sd-notify: do not hang when NOTIFY_SOCKET is used with create #1807

sd-notify: do not hang when NOTIFY_SOCKET is used with create #1807

giuseppe commented May 25, 2018 •

edited

Loading

giuseppe commented May 25, 2018

dqminh commented Jun 2, 2018

giuseppe commented Jun 3, 2018

rhatdan commented Jun 4, 2018

giuseppe commented Jun 4, 2018

rhatdan commented Jun 20, 2018

mrunalp commented Jul 9, 2018

mrunalp commented Jul 9, 2018 •

edited by caniszczyk

Loading

rhatdan commented Jul 9, 2018

rhatdan commented Jul 27, 2018

dqminh commented Aug 5, 2018

giuseppe commented Aug 6, 2018

dqminh Aug 6, 2018

giuseppe Aug 6, 2018

dqminh commented Aug 6, 2018

giuseppe commented Aug 9, 2018

TomSweeneyRedHat commented Aug 16, 2018 •

edited

Loading

giuseppe commented Aug 27, 2018

crosbymichael commented Aug 29, 2018

crawford commented Oct 2, 2018

mrunalp commented Oct 2, 2018 •

edited by caniszczyk

Loading

mrunalp commented Oct 2, 2018

giuseppe commented Feb 22, 2019

filbranden commented Mar 15, 2019

giuseppe commented Mar 15, 2019

filbranden commented Mar 15, 2019

giuseppe commented Mar 15, 2019

giuseppe commented Feb 17, 2020

saschagrunert commented Mar 11, 2020

rhatdan commented Mar 11, 2020

AkihiroSuda commented Mar 11, 2020

giuseppe commented Mar 11, 2020 •

edited

Loading

AkihiroSuda commented Mar 12, 2020 •

edited

Loading

mrunalp commented Mar 13, 2020 •

edited by AkihiroSuda

Loading

sd-notify: do not hang when NOTIFY_SOCKET is used with create #1807

sd-notify: do not hang when NOTIFY_SOCKET is used with create #1807

Conversation

giuseppe commented May 25, 2018 • edited Loading

giuseppe commented May 25, 2018

dqminh commented Jun 2, 2018

giuseppe commented Jun 3, 2018

rhatdan commented Jun 4, 2018

giuseppe commented Jun 4, 2018

rhatdan commented Jun 20, 2018

mrunalp commented Jul 9, 2018

mrunalp commented Jul 9, 2018 • edited by caniszczyk Loading

rhatdan commented Jul 9, 2018

rhatdan commented Jul 27, 2018

dqminh commented Aug 5, 2018

giuseppe commented Aug 6, 2018

dqminh Aug 6, 2018

Choose a reason for hiding this comment

giuseppe Aug 6, 2018

Choose a reason for hiding this comment

dqminh commented Aug 6, 2018

giuseppe commented Aug 9, 2018

TomSweeneyRedHat commented Aug 16, 2018 • edited Loading

giuseppe commented Aug 27, 2018

crosbymichael commented Aug 29, 2018

crawford commented Oct 2, 2018

mrunalp commented Oct 2, 2018 • edited by caniszczyk Loading

mrunalp commented Oct 2, 2018

giuseppe commented Feb 22, 2019

filbranden commented Mar 15, 2019

giuseppe commented Mar 15, 2019

filbranden commented Mar 15, 2019

giuseppe commented Mar 15, 2019

giuseppe commented Feb 17, 2020

saschagrunert commented Mar 11, 2020

rhatdan commented Mar 11, 2020

AkihiroSuda commented Mar 11, 2020

giuseppe commented Mar 11, 2020 • edited Loading

AkihiroSuda commented Mar 12, 2020 • edited Loading

mrunalp commented Mar 13, 2020 • edited by AkihiroSuda Loading

giuseppe commented May 25, 2018 •

edited

Loading

mrunalp commented Jul 9, 2018 •

edited by caniszczyk

Loading

TomSweeneyRedHat commented Aug 16, 2018 •

edited

Loading

mrunalp commented Oct 2, 2018 •

edited by caniszczyk

Loading

giuseppe commented Mar 11, 2020 •

edited

Loading

AkihiroSuda commented Mar 12, 2020 •

edited

Loading

mrunalp commented Mar 13, 2020 •

edited by AkihiroSuda

Loading