Pass NOTIFY_SOCKET and LISTEN_FDS env to OCI RUntime if set #271
Conversation
rhatdan
force-pushed
the
sd_notify
branch
2 times, most recently
from
4c51cd0 to
4da0149
Compare
rhatdan
changed the title
|
☔ The latest upstream changes (presumably dd133a1) made this pull request unmergeable. Please resolve the merge conflicts. |
rhatdan
force-pushed
the
sd_notify
branch
3 times, most recently
from
082eb77 to
8d0fd7e
Compare
| @@ -210,6 +211,14 @@ func (r *OCIRuntime) createContainer(ctr *Container, cgroupParent string) (err e | |||
| // 0, 1 and 2 are stdin, stdout and stderr | |||
| cmd.Env = append(r.conmonEnv, fmt.Sprintf("_OCI_SYNCPIPE=%d", 3)) | |||
| cmd.Env = append(cmd.Env, fmt.Sprintf("_OCI_STARTPIPE=%d", 4)) | |||
| if notify, ok := os.LookupEnv("NOTIFY_SOCKET"); ok { | |||
There was a problem hiding this comment.
Do we want to make this conditional at all, or is it OK if libpod does this in CRI-O as well?
There was a problem hiding this comment.
I don't see any reason to make this conditional. It will not cause any issue. All that happens if the NOTIFY_SOCKET environment is set, is the app will attempt to write to the socket to tell systemd that it is up and ready to receive packets.
| match, _ := session.GrepString(sock) | ||
| Expect(match).Should(BeTrue()) | ||
| os.Unsetenv("NOTIFY_SOCKET") | ||
| }) |
There was a problem hiding this comment.
Anyway to work up a test for LISTEN_FDS?
There was a problem hiding this comment.
You would have to open a socket, and then set the flags, I guess. Maybe this is something we could do in the test suite.
There was a problem hiding this comment.
@TomSweeneyRedHat Actually looking at this deeper, it is very difficult to create a test for this, but doing so I found other issues. Conmon needs to setup LISTEN_PID correctly for this to work.
cri-o/cri-o#1300
I think we should merge this and then attempt to setup a manual test in systemd to make sure it all works.
The way this is supposed to work is systemd will create a process, then will set the LISTEN_FDS=#PASSED in FDS and a LISTEN_PID which is equal to the PID that podman WILL execute with. At which point systemd execs podman. PODMAN then needs to pass these variables down to conmon.
conmon needs to modify LISTEN_PID to the PID that it is going to launch the OCI RUntime with (runc). Then RUNC will modify LISTEN_PID one more time to PID=1, which will launch the container process.
Getting podman to run with the correct LISTEN_PID, is something we could work with in the future, not sure how easy that would be to do with GOLANG. Much easier with bash or C.
There was a problem hiding this comment.
When I replied earlier I was trying to think of a way to test it, but couldn't concoct one off the top of my head. Chicken and egg thing. Thanks for taking a look into it and I'm glad you found some things that should be chased due to that investigation.
|
☔ The latest upstream changes (presumably 095aaaa) made this pull request unmergeable. Please resolve the merge conflicts. |
In order to have sd_notify from systemd to work in containers we need to pass down the NOTIFY_SOCKET environment variable to the container. LISTEN_FDS, tells the application inside of the container to use socket activation and grab the FDS that are leaked into the container. Signed-off-by: Daniel J Walsh <[email protected]>
|
Ok this shell script gets us closer to being able to test. But we need the updated conmon to pass the proper flags to runc. I will try to hack that up on Monday. |
|
LGTM |
1 similar comment
|
LGTM |
|
@rh-atomic-bot r=umohnani8 |
|
📌 Commit 38fb1d8 has been approved by |
|
⚡ Test exempted: pull fully rebased and already tested. |
Windows: Add runtime DNS and del bug fix
github-actions
bot
added
the
locked - please file new issue/PR
In order to have sd_notify from systemd to work in containers
we need to pass down the NOTIFY_SOCKET environment variable to
the container.
LISTEN_FDS, tells the application inside of the container to use
socket activation and grab the FDS that are leaked into the container.
Signed-off-by: Daniel J Walsh [email protected]