-
Notifications
You must be signed in to change notification settings - Fork 23
common: remove Cgroups v1 support (podman6) #387
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
base: main
Are you sure you want to change the base?
Conversation
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6423 |
@containers/container-libs-maintainers I think it'd be nice to have a podman6 or cgv1 or some such label here as well. Mind creating one please? |
ah if it's manual merges here, then no need I guess. |
yeah manual merge, keep the PR as draft until we branched. Still a label to track all the things is still a good idea so I added the "podman 6" label. |
Fixes: RUN-3567 (partly) Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know absolutely nothing about the problem space, this is just looking at the diff + immediately surrounding code.
// Load loads an existing cgroup control. | ||
func Load(path string) (*CgroupControl, error) { | ||
cgroup2, err := IsCgroup2UnifiedMode() | ||
_, err := IsCgroup2UnifiedMode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several other callers of this, and quite a few code paths that could potentially be deleted if this never returns false, nil
. Do we want to update those as well? If not, what’s the exact scope of changes we do want to make?
Assuming we do change all of the callers to expect (_, err)
or (true, nil)
, should wereplace the IsCgroup2UnifiedMode
function by a function with some other name (IsProcCgroupMounted
???), or perhaps remove it entirely?
But then again, note that non-Linux code is hard-coded to return (false, nil)
. Is that still what we want?
} | ||
control := &CgroupControl{ | ||
cgroup2: cgroup2, | ||
cgroup2: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Eliminating this field if it is always true
would be an intermediate step to removing a fair bit of code.)
Resolves: RUN-3567 (partly)