kube play: set service container as main PID when possible #17469
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
What I find amazing: the issue was opened because the RAM usage increased when running |
|
@TomSweeneyRedHat, can we still backport for RHEL or do we need an exception dance? |
pkg/domain/infra/abi/play.go
Outdated
| switch len(notifyProxies) { | ||
| case 0: // Optimization for containers/podman/issues/17345 | ||
| // No container needs sdnotify, so we can mark the | ||
| // service container as the main PID and return early. |
There was a problem hiding this comment.
I find this a bit confusing, I understand this as use the container main pid but it is actually the conmon pid.
There was a problem hiding this comment.
Thanks! Updated the comment.
| case 0: // Optimization for containers/podman/issues/17345 | ||
| // No container needs sdnotify, so we can mark the | ||
| // service container as the main PID and return early. | ||
| data, err := serviceContainer.Inspect(false) |
There was a problem hiding this comment.
Inspect is rather expensive since we look out for permanence I think it would be better to add an accessor for the ConmonPid.
There was a problem hiding this comment.
Nice idea! With the afternoon meetings ahead, I am running out of time but I can add an accessor in another PR.
We did the inspect before 4fa307f so it's at least not a regression.
Commit 4fa307f fixed a number of issues in the sdnotify proxies. Whenever a container runs with a custom sdnotify policy, the proxies need to keep running which in turn required Podman to run and wait for the service container to stop. Improve on that behavior and set the service container as the main PID (instead of Podman) when no container needs sdnotify. Fixes: containers#17345 Signed-off-by: Valentin Rothberg <[email protected]>
|
LGTM |
|
/lgtm |
If a service container was created for a pod, kube play was no longer waiting on it to exit before returning. Looks like this was introduced by containers#17469. Kube play --wait will add tests that will help test this. Just want to fix this before anything is really affected. [NO NEW TESTS NEEDED] Signed-off-by: Urvashi Mohnani <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Commit 4fa307f fixed a number of issues in the sdnotify proxies. Whenever a container runs with a custom sdnotify policy, the proxies need to keep running which in turn required Podman to run and wait for the service container to stop. Improve on that behavior and set the service container as the main PID (instead of Podman) when no container needs sdnotify.
Fixes: #17345
Signed-off-by: Valentin Rothberg [email protected]
Does this PR introduce a user-facing change?
@rhatdan @ygalblum @alexlarsson PTAL
This optimization will also benefit Quadlet. If we plan on cutting a Podman v4.4.2, this may be a nice candidate.