Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass NOTIFY_SOCKET and LISTEN_FDS env to OCI RUntime if set #271

Closed
wants to merge 1 commit into from
Closed

Pass NOTIFY_SOCKET and LISTEN_FDS env to OCI RUntime if set #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions libpod/oci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"time"

"github.com/containerd/cgroups"
"github.com/coreos/go-systemd/activation"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to make this conditional at all, or is it OK if libpod does this in CRI-O as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cmd.Env = append(cmd.Env, fmt.Sprintf("NOTIFY_SOCKET=%s", notify))
}
if listenfds, ok := os.LookupEnv("LISTEN_FDS"); ok {
cmd.Env = append(cmd.Env, fmt.Sprintf("LISTEN_FDS=%s", listenfds))
fds := activation.Files(false)
cmd.ExtraFiles = append(cmd.ExtraFiles, fds...)
}

err = cmd.Start()
if err != nil {
Expand Down
11 changes: 11 additions & 0 deletions test/e2e/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,17 @@ var _ = Describe("Podman run", func() {
Expect(session.OutputToString()).To(ContainSubstring("15"))
})

It("podman run notify_socket", func() {
sock := "/run/sock"
os.Setenv("NOTIFY_SOCKET", sock)
session := podmanTest.Podman([]string{"run", "--rm", ALPINE, "printenv", "NOTIFY_SOCKET"})
session.Wait(10)
Expect(session.ExitCode()).To(Equal(0))
match, _ := session.GrepString(sock)
Expect(match).Should(BeTrue())
os.Unsetenv("NOTIFY_SOCKET")
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway to work up a test for LISTEN_FDS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


It("podman run log-opt", func() {
log := filepath.Join(podmanTest.TempDir, "/container.log")
session := podmanTest.Podman([]string{"run", "--rm", "--log-opt", fmt.Sprintf("path=%s", log), ALPINE, "ls"})
Expand Down
52 changes: 52 additions & 0 deletions vendor/github.com/coreos/go-systemd/activation/files.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 60 additions & 0 deletions vendor/github.com/coreos/go-systemd/activation/listeners.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 37 additions & 0 deletions vendor/github.com/coreos/go-systemd/activation/packetconns.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.