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.

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

kube play: sd-notify integration #15248

Merged
merged 3 commits into from
Aug 11, 2022

Conversation

vrothberg
Copy link
Member

Integrate sd-notify policies into kube play. The policies can be
configured for all contianers via the io.containers.sdnotify
annotation or for indidivual containers via the
io.containers.sdnotify/$name annotation.

The kube play process will wait for all containers to be ready by
waiting for the individual READY=1 messages which are received via
the pkg/systemd/notifyproxy proxy mechanism.

Also update the simple "container" sd-notify test as it did not fully
test the expected behavior which became obvious when adding the new
tests.

Signed-off-by: Valentin Rothberg [email protected]

Add support for specifying sd-notify policies in `kube play`.

Note: this the next step toward auto-update support for kube play. I will add that in another PR to make reviewing easier.

@giuseppe @mheon @edsantiago PTAL

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 9, 2022
@vrothberg
Copy link
Member Author

Argh, final "fixes" before the push broke the test. Will look into it.

@vrothberg
Copy link
Member Author

One more race condition in the test. We need to wait for the socat log. Probably worth adding that to the other tests as well.

@vrothberg
Copy link
Member Author

Opened #15266 in hope to speed up the remote e2e tests. A factor of two slower is screaming for attention.

@vrothberg
Copy link
Member Author

@edsantiago PTanotherL

test/system/helpers.bash Outdated Show resolved Hide resolved
test/system/helpers.bash Outdated Show resolved Hide resolved
@vrothberg vrothberg force-pushed the RUN-1606 branch 3 times, most recently from 7941077 to e5d115e Compare August 10, 2022 14:10
@vrothberg
Copy link
Member Author

[+1439s] # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
[+1439s] # #| FAIL: Timed out waiting for /tmp/podman_bats.Q3dqlq/socat.log
[+1439s] # #^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Looks like we need to bump the timeout ...

@vrothberg
Copy link
Member Author

@edsantiago @mheon PTanotherL

I hope that 15 seconds are sufficient for the log file to pop up on arch64 machine.

@edsantiago
Copy link
Member

I hope that 15 seconds are sufficient for the log file to pop up on arch64 machine.

15 seconds is preposterous. I'm guessing that's not the problem, that it's something different about socat on these hosts. This may require doing the run-in-console thing.

@edsantiago
Copy link
Member

@vrothberg you're going to need to skip_if_aarch64 "FIXME: #15277 sdnotify doesn't work on aarch64"

Please also update the comment on line 145 which says 15074. 15074 was a catchall, #15277 is specific to this bug.

Please also reduce the timeout. 15 seconds really is extreme.

And, someone gonna need to investigate and fix the bug itself.

Add a new package for proxying notify sockets and waiting for the
READY=1 message to appear.  May subject to further changes in
future commits.

Tests make sure that it behaves properly.

Signed-off-by: Valentin Rothberg <[email protected]>
The notify socket can now either be specified via an environment
variable or programatically (where the env is ignored).  The
notify mode and the socket are now also displayed in `container inspect`
which comes in handy for debugging and allows for propper testing.

Signed-off-by: Valentin Rothberg <[email protected]>
Integrate sd-notify policies into `kube play`.  The policies can be
configured for all contianers via the `io.containers.sdnotify`
annotation or for indidivual containers via the
`io.containers.sdnotify/$name` annotation.

The `kube play` process will wait for all containers to be ready by
waiting for the individual `READY=1` messages which are received via
the `pkg/systemd/notifyproxy` proxy mechanism.

Also update the simple "container" sd-notify test as it did not fully
test the expected behavior which became obvious when adding the new
tests.

Signed-off-by: Valentin Rothberg <[email protected]>
@vrothberg
Copy link
Member Author

Thanks a lot, @edsantiago. Updated as suggested.

@edsantiago
Copy link
Member

Your timings, Monsieur:

type distro user local remote container
int fedora-36 root 30:54 46:07 29:51
int ubuntu-2204 root 30:26 51:10
int fedora-36 rootless 31:02
int ubuntu-2204 rootless 28:57
sys fedora-36 root 31:56 27:07
sys fedora-36-aarch64 root 31:35 17:55
sys ubuntu-2204 root 25:32 17:27
sys fedora-36 rootless 29:06 34:28
sys ubuntu-2204 rootless 27:12
bud fedora-36 root 25:16 24:23

@vrothberg
Copy link
Member Author

@giuseppe @flouthoc PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@vrothberg
Copy link
Member Author

Merge me :)

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, flouthoc, giuseppe, vrothberg

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 [edsantiago,flouthoc,giuseppe,vrothberg]

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

@openshift-merge-robot openshift-merge-robot merged commit 92bbae4 into containers:main Aug 11, 2022
@vrothberg vrothberg deleted the RUN-1606 branch August 11, 2022 15:46
@github-actions 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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants