Implement SD-NOTIFY proxy in conmon #8508

rhatdan · 2020-11-29T11:01:50Z

This leverages conmon's ability to proxy the SD-NOTIFY socket.
This prevents locking caused by OCI runtime blocking, waiting for
SD-NOTIFY messages, and instead passes the messages directly up
to the host.

Signed-off-by: Joseph Gooch [email protected]
Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2020-11-29T11:02:01Z

Replaces: #7607

rhatdan · 2020-11-29T11:02:44Z

@goochjj I will take this one over and try to finish it up.

Thanks for the PR.

rhatdan · 2020-11-30T14:19:09Z

@haircommander Is this problem caused by older conmon still?

haircommander · 2020-11-30T14:41:26Z

yeah it looks like it

rhatdan · 2020-11-30T17:46:35Z

@cevich PTAL

libpod/container_internal_linux.go

giuseppe · 2020-12-01T10:43:21Z

libpod/container_internal_linux.go

+	// Bind notify proxy
+	if c.config.SdNotifyMode == define.SdNotifyModeContainer {
+		notifyDir := filepath.Join(c.bundlePath(), "notify")
+		logrus.Debugf("checking notify %q dir", notifyDir)
+		if err := os.MkdirAll(notifyDir, 0755); err != nil {
+			if !os.IsExist(err) {
+				return errors.Wrapf(err, "unable to create notify %q dir", notifyDir)
+			}
+		}
+		if err := label.Relabel(notifyDir, c.MountLabel(), true); err != nil {
+			return errors.Wrapf(err, "relabel failed %q", notifyDir)
+		}
+		logrus.Debugf("add bindmount notify %q dir", notifyDir)
+		if _, ok := c.state.BindMounts["/run/notify"]; !ok {
+			c.state.BindMounts["/run/notify"] = notifyDir
+		}
+	}
+


similarly here, we need the OCI runtime to take care of it. If we pass down the notify socket directly to the container, there is a race condition between the notification being sent and systemd reading it.

@giuseppe We're proxying. PODMAN is now in charge of notification. NOT the OCI runtime. That's the point.

PODMAN creates /run/notify/
PODMAN tells OCI runtime to bind-mount /run/notify to the container's setup folder
CONMON forwards /run/notify/notify.sock ( in container, by way of bind mount ) to --sdnotify-socket on host

OCI runtime is NOT taking care of it.

giuseppe

LGTM

openshift-ci-robot · 2020-12-01T15:41:19Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,rhatdan]

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

cevich · 2020-12-01T16:48:18Z

@cevich PTAL

Not sure what is needed from me. I peeked at the testing results and they all appear related to the changes here, no?

mheon · 2020-12-01T16:51:36Z

We may need new VM images with a newer Conmon.

rhatdan · 2020-12-01T19:26:13Z

Yes @cevich new vms need latest conmon.

cevich · 2020-12-02T16:29:05Z

Yes @cevich new vms need latest conmon.

And newer Ubuntu ☹️ Can be part of WIP #8312 (from containers/automation_images#34)?

cevich · 2020-12-02T16:57:27Z

Note, "No" is acceptable: in that case I'll roll up new images separate from my Ubuntu work, and they can be brought in via this PR.

rhatdan · 2020-12-02T19:45:58Z

@cevich depends on when I can get new Ubuntu. A couple of days, I am fine with it. weeks, I have a problem.

cevich · 2020-12-02T21:15:17Z

Sounds fair, if we get into next week and I'm still stuck, I'll make a new PR to just refresh the current images. There is a risk it will fail with Ubuntu 19, as we're on borrowed-time WRT the package repositories (19 is EOL) remaining available.

Otherwise/also, I've just finished building fresh and am trying those out right now in #8312...

cevich · 2020-12-04T16:28:08Z

...yeah, we've got problems with the new VMs that will probably take a while to solve. But maybe we have some luck here, b/c IIRC brent built new VMs to incorporate docker compose. It's not yet in master, @rhatdan try this see if it works:

diff --git a/.cirrus.yml b/.cirrus.yml
index 0fa51be63..be4c0e459 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -30,7 +30,7 @@ env:
     PRIOR_UBUNTU_NAME: "ubuntu-19"

     # Google-cloud VM Images
-    IMAGE_SUFFIX: "c5402398833246208"
+    IMAGE_SUFFIX: "c4704091098054656"
     FEDORA_CACHE_IMAGE_NAME: "fedora-${IMAGE_SUFFIX}"
     PRIOR_FEDORA_CACHE_IMAGE_NAME: "prior-fedora-${IMAGE_SUFFIX}"
     UBUNTU_CACHE_IMAGE_NAME: "ubuntu-${IMAGE_SUFFIX}"

cevich · 2020-12-04T16:29:17Z

Oh! I can check what version of conmon is in ubuntu-c4704091098054656...looks like 2.0.20~1. Is that new enough?

rhatdan · 2020-12-04T18:36:06Z

I think we need 2.0.21

cevich · 2020-12-07T15:39:25Z

I think we need 2.0.21

okay. Lokesh has a new OBS repo with libseccomp 2.5.1 and other updated packages for us. Building images now...

cevich · 2020-12-07T20:52:29Z

...testing new images.

cevich · 2020-12-08T17:34:44Z

Update: I'm down to ~2-5 failing tests on the new images. These failures seem to be caused by "too old" package versions. We're asking a lot of Lokesh to maintain all these special Ubuntu repos. in the first place, so I'm afraid to ask for updates to the (now defunct) 19.10 repo, in addition to 20.04 and 20.10. At the same time, I don't want to give you a steaming pile to work with, so I'm doing by best to get all the tests passing (PR #8312) first.

cevich · 2020-12-10T16:16:55Z

Update: Getting very close with the new images, though I've had to add skips to workaround a few broken tests/situations.

cevich · 2020-12-11T19:07:50Z

Update: The suite of podman CI tests should be passing now, so this PR is waiting on review and merge of #8312

rhatdan · 2021-01-14T18:30:30Z

Not going to make the 3.0 release. Will have to wait until 3.1

github-actions · 2021-05-29T00:14:41Z

A friendly reminder that this PR had no activity for 30 days.

This leverages conmon's ability to proxy the SD-NOTIFY socket. This prevents locking caused by OCI runtime blocking, waiting for SD-NOTIFY messages, and instead passes the messages directly up to the host. [NO TESTS NEEDED] existing tests cover this use case. Signed-off-by: Joseph Gooch <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>

openshift-ci · 2021-07-16T17:24:59Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,rhatdan]

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

mheon · 2021-08-09T20:07:38Z

What's the status here? I see we have a commit changing the default type of generate systemd units over to notify in the v3.3 branch - are we still using the OCI runtime to provide sdnotify in that case, or is conmon being used? If we're using the OCI runtime, I think we need to revert for 3.3, until this PR can merge.

rhatdan · 2021-08-10T12:47:44Z

I have no idea of the state. I grabbed this PR to try to push it over the edge a while back. but I have no idea what is going on at this state. If @mheon or someone else wants to finish this, I am open to them taking it.

flyingflo · 2021-08-16T13:16:39Z

What is missing besides of a bunch of failing tests? These seem to fail on many pull requests, however. 🤷‍♂️ Don't know why..

vrothberg · 2021-08-16T13:28:11Z

libpod/oci_conmon_linux.go

@@ -1254,6 +1244,12 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p
 		args = append(args, rFlags...)
 	}

+	if ctr.config.SdNotifyMode == define.SdNotifyModeContainer {


Don't we also have to unset NOTIFY_SOCKET to prevent runc and crun from blocking? Not sure if I understood it correctly though.

Well we don't want to pass these on to the OCI Runtimes, yes, since we don't want them handling it.

vrothberg · 2021-08-17T11:26:34Z

@rhatdan suggested that I take over this PR to kick it over the finish line, closing.

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2020

rhatdan mentioned this pull request Nov 29, 2020

Implement SD-NOTIFY proxy in conmon #7607

Closed

rhatdan added the 3.0 Features label Nov 29, 2020

giuseppe reviewed Dec 1, 2020

View reviewed changes

giuseppe approved these changes Dec 1, 2020

View reviewed changes

rhatdan force-pushed the notify branch from f4304f4 to ebd4180 Compare December 1, 2020 21:21

rhatdan force-pushed the notify branch from ebd4180 to e4e3a82 Compare December 8, 2020 11:14

rhatdan force-pushed the notify branch from e4e3a82 to 89ad119 Compare December 12, 2020 10:49

cevich mentioned this pull request Dec 15, 2020

Cirrus: Update VM Images; Both Fedora and Ubuntu "prior" flavors run with CGroupsV1 & runc #8312

Merged

5 tasks

rhatdan force-pushed the notify branch 2 times, most recently from ba737a4 to 7c193c2 Compare January 12, 2021 12:47

rhatdan force-pushed the notify branch from 7c193c2 to 1e9baae Compare January 14, 2021 15:42

rhatdan removed the 3.0 Features label Jan 14, 2021

rhatdan force-pushed the notify branch 2 times, most recently from 5d2d987 to e3aa257 Compare February 1, 2021 14:07

rhatdan force-pushed the notify branch 2 times, most recently from 1beb54d to bab9e74 Compare February 16, 2021 09:55

rhatdan force-pushed the notify branch from bab9e74 to 2070738 Compare March 3, 2021 19:51

rhatdan force-pushed the notify branch from 2070738 to ad13d3d Compare March 11, 2021 11:24

rhatdan force-pushed the notify branch from ad13d3d to 623da39 Compare March 24, 2021 11:49

rhatdan force-pushed the notify branch from 623da39 to c400916 Compare April 15, 2021 13:24

rhatdan force-pushed the notify branch from c400916 to 6587fe7 Compare April 28, 2021 10:29

goochjj mentioned this pull request May 6, 2021

[RFE] Integrate sd_notify with podman health checks #6160

Closed

github-actions bot added the stale-pr label May 29, 2021

flyingflo mentioned this pull request Jul 16, 2021

systemd sd-notify freezes podman #6688

Closed

rhatdan force-pushed the notify branch from 6587fe7 to b965494 Compare July 16, 2021 17:24

github-actions bot removed the stale-pr label Aug 10, 2021

vrothberg reviewed Aug 16, 2021

View reviewed changes

vrothberg closed this Aug 17, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement SD-NOTIFY proxy in conmon #8508

Implement SD-NOTIFY proxy in conmon #8508

rhatdan commented Nov 29, 2020

rhatdan commented Nov 29, 2020

rhatdan commented Nov 29, 2020

rhatdan commented Nov 30, 2020

haircommander commented Nov 30, 2020

rhatdan commented Nov 30, 2020

giuseppe Dec 1, 2020

goochjj Dec 1, 2020

goochjj Dec 1, 2020

giuseppe left a comment

openshift-ci-robot commented Dec 1, 2020

cevich commented Dec 1, 2020

mheon commented Dec 1, 2020

rhatdan commented Dec 1, 2020

cevich commented Dec 2, 2020

cevich commented Dec 2, 2020

rhatdan commented Dec 2, 2020

cevich commented Dec 2, 2020 •

edited

Loading

cevich commented Dec 4, 2020

cevich commented Dec 4, 2020

rhatdan commented Dec 4, 2020

cevich commented Dec 7, 2020

cevich commented Dec 7, 2020

cevich commented Dec 8, 2020

cevich commented Dec 10, 2020 •

edited

Loading

cevich commented Dec 11, 2020

rhatdan commented Jan 14, 2021

github-actions bot commented May 29, 2021

openshift-ci bot commented Jul 16, 2021

mheon commented Aug 9, 2021

rhatdan commented Aug 10, 2021

flyingflo commented Aug 16, 2021

vrothberg Aug 16, 2021

rhatdan Aug 16, 2021

vrothberg commented Aug 17, 2021

Implement SD-NOTIFY proxy in conmon #8508

Implement SD-NOTIFY proxy in conmon #8508

Conversation

rhatdan commented Nov 29, 2020

rhatdan commented Nov 29, 2020

rhatdan commented Nov 29, 2020

rhatdan commented Nov 30, 2020

haircommander commented Nov 30, 2020

rhatdan commented Nov 30, 2020

giuseppe Dec 1, 2020

Choose a reason for hiding this comment

goochjj Dec 1, 2020

Choose a reason for hiding this comment

goochjj Dec 1, 2020

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 1, 2020

cevich commented Dec 1, 2020

mheon commented Dec 1, 2020

rhatdan commented Dec 1, 2020

cevich commented Dec 2, 2020

cevich commented Dec 2, 2020

rhatdan commented Dec 2, 2020

cevich commented Dec 2, 2020 • edited Loading

cevich commented Dec 4, 2020

cevich commented Dec 4, 2020

rhatdan commented Dec 4, 2020

cevich commented Dec 7, 2020

cevich commented Dec 7, 2020

cevich commented Dec 8, 2020

cevich commented Dec 10, 2020 • edited Loading

cevich commented Dec 11, 2020

rhatdan commented Jan 14, 2021

github-actions bot commented May 29, 2021

openshift-ci bot commented Jul 16, 2021

mheon commented Aug 9, 2021

rhatdan commented Aug 10, 2021

flyingflo commented Aug 16, 2021

vrothberg Aug 16, 2021

Choose a reason for hiding this comment

rhatdan Aug 16, 2021

Choose a reason for hiding this comment

vrothberg commented Aug 17, 2021

cevich commented Dec 2, 2020 •

edited

Loading

cevich commented Dec 10, 2020 •

edited

Loading