Repository navigation
Conversation
dfdd042 to
b28c810
Compare
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
I think the CI failures are related. I just have a naming concern now. Also, the commit message should say Fixes: instead of Closes:.
| // moveConmonToCgroupAndSignal gets a container's cgroupParent and moves the conmon process to that cgroup | ||
| // it then signals for conmon to start by sending nonce data down the start fd | ||
| func (r *ConmonOCIRuntime) moveConmonToCgroupAndSignal(ctr *Container, cmd *exec.Cmd, startFd *os.File) error { |
There was a problem hiding this comment.
Should this be renamed? Now, it doesn't move only the common process.
There was a problem hiding this comment.
In my issue I suggested libpod-support-<CONTAINER_ID>.scope may be a more accurate name for the control group if it contains more processes, but you may consider this a breaking change. I think keeping the function and control group names in sync is best, i.e., change both or neither.
@giuseppe, Just a heads-up regarding the failing CI checks: the If you look at the bottom of the logs for the browser tests, it fails with: My PR #29803 was hitting this exact same infrastructure failure earlier today, so it has nothing to do with your code changes. The code looks great! link:https://artifacts.dev.testing-farm.io/e84b398e-901d-4825-be77-02520f2dfd97 |
824cd04 to
8ba8e58
Compare
|
[NON-BLOCKING] Packit jobs failed. @podman-container-tools/packit-jobs please check. Everyone else, feel free to ignore. |
8ba8e58 to
27e3e1f
Compare
yes, CI failures were related, we need this ugly workaround for now: https://github.com/podman-container-tools/container-libs/pull/1207/changes#r4084790065 |
a9ea208 to
a3f6017
Compare
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Code LGTM,
PTAL @sbrivio-rh
a3f6017 to
9047b0a
Compare
Signed-off-by: Giuseppe Scrivano <giuseppe@scrivano.org>
The only behaviour change is that a GetLimits() failure now skips the cgroup instead of creating it with empty resources. GetLimits() cannot actually fail today, it only ever returns a nil error. Signed-off-by: Giuseppe Scrivano <giuseppe@scrivano.org>
When podman owns the conmon cgroup it only ever put conmon there. The per-container network helpers stayed in whatever cgroup the invoking podman happened to live in so their resources were not accounted to the container and their lifetime was tied to an unrelated cgroup. Put them in the same cgroup as conmon. Fixes: podman-container-tools#29582 Signed-off-by: Giuseppe Scrivano <giuseppe@scrivano.org>
9047b0a to
7fe579d
Compare
giuseppe
added
the
bloat_approved
|
I've read through most of these changes (excluding vendor/), and everything seems to match what we discussed on the issue. Thanks! I think there are some other related issues, but I'll re-evaluate them once I can test this. Will this only land in a 6.x release? |
When podman owns the conmon cgroup it only ever put conmon there. The per-container network helpers stayed in whatever cgroup the invoking podman happened to live in so their resources were not accounted to the container and their lifetime was tied to an unrelated cgroup.
Put them in the same cgroup as conmon.
Closes: #29582
Needs: podman-container-tools/container-libs#1207