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

libct/cg: rm dead code to improve clarity #3136

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Aug 8, 2021

This was initially added by commits 41d9d26 and 4a8f0b4,
apparently to implement docker run --cgroup container:ID (moby/moby#19244),
which was never merged. Therefore, this code is not and was never used.

It needs to be removed mainly because having it makes it much harder to
understand how cgroup manager works (because with this in place we have
not one or two but three sets of cgroup paths to think about).

Note if the paths are known and there is a need to add a PID to existing
cgroup, cgroup manager is not needed at all -- something like
cgroups.WriteCgroupProc or cgroups.EnterPid is sufficient (and the
latter is what runc exec uses in (*setnsProcess).start). If, for some reason,
the cgroup manager is still needed, it's better to write a separate shallow
implementation that would only do WriteCgroupProc/EnterPid from Apply,
and have other methods empty.

This was initially added by commits 41d9d26 and 4a8f0b4,
apparently to implement docker run --cgroup container:ID, which was
never merged. Therefore, this code is not and was never used.

It needs to be removed mainly because having it makes it much harder to
understand how cgroup manager works (because with this in place we have
not one or two but three sets of cgroup paths to think about).

Note if the paths are known and there is a need to add a PID to existing
cgroup, cgroup manager is not needed at all -- something like
cgroups.WriteCgroupProc or cgroups.EnterPid is sufficient (and the
latter is what runc exec uses in (*setnsProcess).start).

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin added this to the 1.1.0 milestone Aug 8, 2021
Copy link
Contributor

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

Makes sense to remove 👍

@kolyshkin
Copy link
Contributor Author

@AkihiroSuda @cyphar @thaJeztah PTAL

@kolyshkin kolyshkin added the kind/refactor refactoring label Aug 18, 2021
@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL. This removes the code that's not used but makes a huge roadblock preventing cgroup manager code understanding (at least this is the way it was for me), so its removal is quite important (if we want more people to work on runc).

Copy link
Contributor

@hqhq hqhq left a comment

Choose a reason for hiding this comment

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

LGTM.

I remembered --cgroup:container:<id> was a useful feature needed by some projects, I'm surprised it's never merged. 😂

@hqhq hqhq merged commit b4b7972 into opencontainers:master Aug 25, 2021
@kolyshkin
Copy link
Contributor Author

I remembered --cgroup:container:<id> was a useful feature needed by some projects, I'm surprised it's never merged.

@hqhq In terms of runc and/or OCI spec, specifying cgroupPath is sufficient for a container to share a cgroup with another container -- no other changes are needed.

OTOH doing this creates some other problems -- see e.g. #3132

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants