Refactor I/O and add SD_NOTIFY proxy support #182

goochjj · 2020-06-24T18:39:13Z

Refactored all the conn_sock functionality to be more generic. It can deal
with different types of sockets, stream vs dgram, and reuses all the same
callbacks, shutdown and async functionality.

Conmon creates a notify socket which podman bind-mounts into the container,
and passes in via the spec's environment variables. Conmon relays the
READY=1 signal. This is similar to what runc and crun do, but doing it in
conmon and NOT passing NOTIFY_SOCKET to the OCI runtime allows us to start
up properly without runc and crun blocking on the "start" command.

It would also be trivial to add more proxied sockets, i.e. the /dev/log
proof of concept I did would now be super easy, if we wanted to revisit that.

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

goochjj · 2020-06-24T18:41:00Z

@haircommander PTAL

This is another take on what I had done w/ the syslog proxy. You had requested the console sockets and udp sockets share more code - I've done that by moving some flags and state into the structures used to manage the sockets. In addition this implements a SD-NOTIFY proxy using the same constructs.

SD-NOTIFY conversation from here containers/podman#6688

src/conmon.c

rhatdan · 2020-06-25T11:49:44Z

I am a little worried about SELinux here, making sure that the container processes are still allowed to write the SD_NOTIFY socket. The path needs to be writeable from the container, and the listener has to be ok.
Most likely this will work, but we should test this on an SELinux enforced system, with the container writing to the socket.

goochjj · 2020-06-25T13:31:04Z

I am a little worried about SELinux here, making sure that the container processes are still allowed to write the SD_NOTIFY socket. The path needs to be writeable from the container, and the listener has to be ok.
Most likely this will work, but we should test this on an SELinux enforced system, with the container writing to the socket.

Agreed, I need to revisit podman doing the bind mount - specifically setting the mount label appropriately.

Also even though I'm creating the socket as 777, it's getting reset to 750. Haven't figured that out yet.

goochjj · 2020-06-25T14:13:16Z

I am a little worried about SELinux here, making sure that the container processes are still allowed to write the SD_NOTIFY socket. The path needs to be writeable from the container, and the listener has to be ok.
Most likely this will work, but we should test this on an SELinux enforced system, with the container writing to the socket.

libpod now relabels /run/notify

goochjj · 2020-06-25T15:02:51Z

I am a little worried about SELinux here, making sure that the container processes are still allowed to write the SD_NOTIFY socket. The path needs to be writeable from the container, and the listener has to be ok.
Most likely this will work, but we should test this on an SELinux enforced system, with the container writing to the socket.

libpod now relabels /run/notify

And I get this

[589620.624683] audit: type=1400 audit(1593097271.023:4559): avc:  denied  { sendto } for  pid=1644255 comm="systemd-notify" path="/var/lib/containers/storage/overlay-containers/7393144d563f1d3acb7a33003cbc43077d62748484963a5d9184406c4a7589e9/userdata/notify/notify.sock" scontext=system_u:system_r:container_init_t:s0:c355,c461 tcontext=unconfined_u:system_r:container_runtime_t:s0 tclass=unix_dgram_socket permissive=0

Do I need to change the tcontext somehow? Suggestions?

src/conn_sock.c

goochjj · 2020-07-01T13:57:03Z

Note @rhatdan this does work because in podman I'm setting an appropriate label on the bind mount. Tested with and without selinux, root and rootless.

I did hardcode in the NOTIFY_SOCKET inside the container, if we want that to be optional - maybe an additional env variable of something like NOTIFY_SOCKET_INSIDE or something can indicate the destination?

haircommander · 2020-07-01T14:16:27Z

src/conmon.c

@@ -84,6 +84,16 @@ int main(int argc, char *argv[])
 		exit(0);
 	}

+	char *notify_socket_path = getenv("NOTIFY_SOCKET");


is there a reason to expose this via env vs cli?

Just that it's how the notify socket is usually passed from systemd et al - but podman is always our parent, so I'm fine with whatever.

Maybe --notify-socket-host and --notify-socket-destdir or something - that can get rid of the hardcoded path of /run/notify/notify.sock in the container, and instead have it be (notify-socket-destdir)/notify.sock in the container.

yes I like that idea. then it can be exposed via cli on podman's side if the user wants to change it.

could we include the commit 52546c919be261e74847981b4db1227537c47345 into the commit that first introduced the change?

Also, I think we need a way to disable the notify socket to be handled by conmon, there are cases when it is helpful to use the handling in the OCI runtime. For example when the user needs $RUNTIME start $CTR to wait for the container to be ready.

src/conn_sock.c

src/conn_sock.h

src/globals.c

haircommander · 2020-07-01T14:32:01Z

sorry it took so long to finally review. all together it looks great. thank you very much for taking this on.

I'd also like a review from @giuseppe

goochjj · 2020-07-01T16:22:18Z

Also FYI, even if the notify socket cmdline/env are passed.. right now podman mkdir's the notify folder because I have it set the mount label, and pass the mount label as a bind mount to the spec. If the calling process doesn't create that socket, but a NOTIFY_SOCKET is passed, it'll error with a cryptic error.

It was just an order of operations thing - that the runtime spec and mounts get done before conmon is called.

We could, instead, throw a nice error , like "notify socket passed but no folder created" or something.

giuseppe

thanks for the patch. I think it is a great idea but I've not managed to get it to work:

$ NOTIFY_SOCKET=/run/user/1000/systemd/notify podman --conmon ~/src/conmon/bin/conmon  run --rm -ti fedora sh -c 'printenv NOTIFY_SOCKET; systemd-notify --ready'
Error: container create failed (no logs from conmon): EOF

src/conn_sock.c

giuseppe · 2020-07-02T07:07:20Z

src/conmon.c

@@ -84,6 +84,16 @@ int main(int argc, char *argv[])
 		exit(0);
 	}

+	char *notify_socket_path = getenv("NOTIFY_SOCKET");


could we include the commit 52546c919be261e74847981b4db1227537c47345 into the commit that first introduced the change?

Also, I think we need a way to disable the notify socket to be handled by conmon, there are cases when it is helpful to use the handling in the OCI runtime. For example when the user needs $RUNTIME start $CTR to wait for the container to be ready.

goochjj · 2020-07-02T14:19:02Z

thanks for the patch. I think it is a great idea but I've not managed to get it to work:

$ NOTIFY_SOCKET=/run/user/1000/systemd/notify podman --conmon ~/src/conmon/bin/conmon  run --rm -ti fedora sh -c 'printenv NOTIFY_SOCKET; systemd-notify --ready'
Error: container create failed (no logs from conmon): EOF

@giuseppe you need goochjj/podman#1

goochjj · 2020-07-02T14:20:38Z

@giuseppe re: commit, yes, I would squash this whole PR before merging.

goochjj · 2020-07-02T14:24:17Z

@giuseppe Re: letting OCI runtime do Notify - I'm ok with that, but I think it basically means extending the --sdnotify option in podman.

from containers/podman#6693:

--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.

Perhaps "container" should be split. I'm not sure into what and what. Maybe "ociruntime" and "podman". Then we'd have the groundwork to tweak the options passed to conmon.

The advantage of having conmon do the proxy is exactly to prevent CRUN/RUNC from blocking on start, to fix containers/podman#6688.

src/conmon.c

mheon · 2020-08-17T15:10:25Z

@goochjj Can you rebase?

Let's see about getting this moved forward and merged, I'd love to have this in Podman 2.1.x

goochjj · 2020-08-18T12:13:42Z

@haircommander @mheon What's the deal with the tests...

haircommander · 2020-08-18T12:15:00Z

f28 and f29 are EOL, and the images haven't been updated. It is on my list this week to update them. sorry for the inconvenience

goochjj · 2020-08-18T12:15:51Z

Heh no worries, as long as it's not a blocker :-D

I did run make fmt locally

haircommander · 2020-08-21T15:11:10Z

hey @goochjj CI is (mostly) fixed, please rebase!

rhatdan · 2020-09-11T10:42:02Z

@haircommander @goochjj Other then the static build fail above is this ready to go in? Would like to get this in to close a Podman issue.

goochjj · 2020-09-11T13:01:31Z

Yep, should be.

haircommander · 2020-09-11T13:30:01Z

I think there are some unresolved review comments from @giuseppe

goochjj · 2020-09-11T13:38:42Z

I think there are some unresolved review comments from @giuseppe

If so you'll need to tell me what needs to happen. Last I can see, I responded to @giuseppe and he didn't respond back.

haircommander · 2020-09-11T13:41:53Z

I'm sorry about the nitpicking @goochjj if you are done with carrying this PR, we can have someone else take over it. A lot relies on conmon and the testing is really subpar, so please excuse my anxiety

goochjj · 2020-09-11T14:14:13Z

Let me go through and see if I can verify the resolved things

goochjj · 2020-09-11T14:33:25Z

Resolved just about everything except the open "Should we use ENV or a command-line argument"

goochjj · 2020-09-11T16:10:45Z

@haircommander As far as I know I've completed all I need to - let me know if there's something I need to look at, or you want to further discuss ENV vs cmdline. I can squash the PR whenever you want.

src/conmon.c

haircommander · 2020-09-11T19:03:43Z

test for cri-o regressions: cri-o/cri-o#4187

haircommander · 2020-09-12T04:41:06Z

hold this, there are some failures I need to investigate

rhatdan · 2020-09-15T12:53:45Z

@haircommander Whats up with this one?

src/conn_sock.c

Refactored all the conn_sock functionality to be more generic. It can deal with different types of sockets, stream vs dgram, and reuses all the same callbacks, shutdown and async functionality. Conmon creates a notify socket which podman bind-mounts into the container, and passes in via the spec's environment variables. Conmon relays the READY=1 signal. This is similar to what runc and crun do, but doing it in conmon and NOT passing NOTIFY_SOCKET to the OCI runtime allows us to start up properly without runc and crun blocking on the "start" command. It would also be trivial to add more proxied sockets, i.e. the /dev/log proof of concept I did would now be super easy, if we wanted to revisit that. Signed-off-by: Joseph Gooch <[email protected]>

haircommander · 2020-09-16T14:26:33Z

LGTM assuming cri-o/cri-o#4187 passes
@giuseppe PTAL

giuseppe

LGTM

goochjj force-pushed the socket-handling-revamp branch from e8747ba to 4032e7c Compare June 24, 2020 18:47

goochjj mentioned this pull request Jun 24, 2020

systemd sd-notify freezes podman containers/podman#6688

Closed

goochjj force-pushed the socket-handling-revamp branch 2 times, most recently from 22741d9 to 70ecb8c Compare June 24, 2020 19:02

haircommander reviewed Jun 24, 2020

View reviewed changes

src/conmon.c Outdated Show resolved Hide resolved

haircommander reviewed Jun 25, 2020

View reviewed changes

src/conn_sock.c Outdated Show resolved Hide resolved

haircommander reviewed Jul 1, 2020

View reviewed changes

src/conn_sock.c Outdated Show resolved Hide resolved

haircommander reviewed Jul 1, 2020

View reviewed changes

src/conn_sock.c Outdated Show resolved Hide resolved

haircommander reviewed Jul 1, 2020

View reviewed changes

src/conn_sock.c Outdated Show resolved Hide resolved

haircommander reviewed Jul 1, 2020

View reviewed changes

src/conn_sock.c Outdated Show resolved Hide resolved

haircommander reviewed Jul 1, 2020

View reviewed changes

src/conn_sock.h Outdated Show resolved Hide resolved

haircommander reviewed Jul 1, 2020

View reviewed changes

src/globals.c Show resolved Hide resolved

giuseppe reviewed Jul 2, 2020

View reviewed changes

src/conmon.c Outdated Show resolved Hide resolved

mheon mentioned this pull request Aug 17, 2020

podman run -d: hangs when $NOTIFY_SOCKET is set containers/podman#7316

Closed

goochjj force-pushed the socket-handling-revamp branch 2 times, most recently from 6a4784b to 1fc2dbf Compare August 18, 2020 11:22

goochjj force-pushed the socket-handling-revamp branch from 1fc2dbf to 6270ce7 Compare August 21, 2020 23:01

goochjj force-pushed the socket-handling-revamp branch from e202333 to 388b405 Compare September 11, 2020 15:04

goochjj force-pushed the socket-handling-revamp branch from 388b405 to 2eb7590 Compare September 11, 2020 16:31

goochjj mentioned this pull request Sep 11, 2020

Implement SD-NOTIFY proxy in conmon containers/podman#7607

Closed

haircommander reviewed Sep 11, 2020

View reviewed changes

src/conmon.c Outdated Show resolved Hide resolved

goochjj force-pushed the socket-handling-revamp branch from 2eb7590 to 6628524 Compare September 11, 2020 17:40

haircommander reviewed Sep 15, 2020

View reviewed changes

src/conn_sock.c Outdated Show resolved Hide resolved

goochjj force-pushed the socket-handling-revamp branch from 0a36f41 to 28799f9 Compare September 16, 2020 13:54

giuseppe approved these changes Sep 16, 2020

View reviewed changes

haircommander merged commit 59c2817 into containers:master Sep 16, 2020

Refactor I/O and add SD_NOTIFY proxy support #182

Refactor I/O and add SD_NOTIFY proxy support #182

Conversation

goochjj commented Jun 24, 2020

goochjj commented Jun 24, 2020

rhatdan commented Jun 25, 2020

goochjj commented Jun 25, 2020

goochjj commented Jun 25, 2020

goochjj commented Jun 25, 2020

goochjj commented Jul 1, 2020

haircommander Jul 1, 2020

Choose a reason for hiding this comment

goochjj Jul 1, 2020

Choose a reason for hiding this comment

haircommander Jul 1, 2020

Choose a reason for hiding this comment

giuseppe Jul 2, 2020

Choose a reason for hiding this comment

haircommander commented Jul 1, 2020

goochjj commented Jul 1, 2020

giuseppe left a comment

Choose a reason for hiding this comment

giuseppe Jul 2, 2020

Choose a reason for hiding this comment

goochjj commented Jul 2, 2020 • edited Loading

goochjj commented Jul 2, 2020

goochjj commented Jul 2, 2020

mheon commented Aug 17, 2020

goochjj commented Aug 18, 2020

haircommander commented Aug 18, 2020

goochjj commented Aug 18, 2020

haircommander commented Aug 21, 2020

rhatdan commented Sep 11, 2020

goochjj commented Sep 11, 2020

haircommander commented Sep 11, 2020

goochjj commented Sep 11, 2020

haircommander commented Sep 11, 2020

goochjj commented Sep 11, 2020

goochjj commented Sep 11, 2020

goochjj commented Sep 11, 2020

haircommander commented Sep 11, 2020

haircommander commented Sep 12, 2020

rhatdan commented Sep 15, 2020

haircommander commented Sep 16, 2020

giuseppe left a comment

Choose a reason for hiding this comment

goochjj commented Jul 2, 2020 •

edited

Loading