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

Store cgroup manager on a per-container basis #7970

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

mheon
Copy link
Member

@mheon mheon commented Oct 8, 2020

When we create a container, we assign a cgroup parent based on the current cgroup manager in use. This parent is only usable with the cgroup manager the container is created with, so if the default cgroup manager is later changed or overridden, the container will not be able to start.

To solve this, store the cgroup manager that created the container in container configuration, so we can guarantee a container with a systemd cgroup parent will always be started with systemd cgroups.

Unfortunately, this is very difficult to test in CI, due to the fact that we hard-code cgroup manager on all invocations of Podman in CI.

Fixes #7830

When we create a container, we assign a cgroup parent based on
the current cgroup manager in use. This parent is only usable
with the cgroup manager the container is created with, so if the
default cgroup manager is later changed or overridden, the
container will not be able to start.

To solve this, store the cgroup manager that created the
container in container configuration, so we can guarantee a
container with a systemd cgroup parent will always be started
with systemd cgroups.

Unfortunately, this is very difficult to test in CI, due to the
fact that we hard-code cgroup manager on all invocations of
Podman in CI.

Fixes containers#7830

Signed-off-by: Matthew Heon <[email protected]>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2020
@rhatdan
Copy link
Member

rhatdan commented Oct 8, 2020

LGTM

1 similar comment
@TomSweeneyRedHat
Copy link
Member

LGTM

@jwhonce
Copy link
Member

jwhonce commented Oct 8, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2020
@openshift-merge-robot openshift-merge-robot merged commit e9961e0 into containers:master Oct 8, 2020
edsantiago added a commit to edsantiago/libpod that referenced this pull request Oct 14, 2020
 - run --userns=keep-id: confirm that $HOME gets set (containers#8013)

 - inspect: confirm that JSON output is a sane number of
   lines (10 or more), not an unreadable one-liner (containers#8011
   and containers#8021). Do so with image, pod, network, volume
   because the code paths might be different.

 - cgroups: confirm that 'run' preserves cgroup manager (containers#7970)

 - sdnotify: reenable tests, and hope CI doesn't hang. This
   test was disabled on August 18 because CI jobs were hanging
   and timing out. My suspicion was that it was containers#7316, which
   in turn seems to have hinged on conmon containers#182. The latter
   was merged on Sep 16, so let's cross our fingers and see
   what happens.

Also: remove inaccurate warning from a networking test.

And, wow, fix is_cgroupsv2(), it has never actually worked.

Signed-off-by: Ed Santiago <[email protected]>
@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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Containers created with cgroup_manager=cgroupfs fail obscurely with new default settings
6 participants