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

Add a cgroup repo/project proposal #144

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Nov 7, 2024

This adds a proposal to split github.com/opencontainers/runc/libcontainer/cgroups packages to a new home at github.com/opencontainers/cgroups, which appeared as a result of discussion at kubernetes/kubernetes#128157 (and kubernetes/kubernetes#128157 (comment) in particular).

Below is a copy-paste of the current draft.


OCI cgroups project proposal

Abstract

Having a common cgroups management implementation in OCI for use in and outside
of runc will ensure long lasting security and interoperability throughout the
container ecosystem.

Proposal

Refactor and move cgroups packages out of runc into a separate project:

https://github.com/opencontainers/runc/tree/master/libcontainer/cgroups

The new project would live in the opencontainers github organization:

https://github.com/opencontainers/cgroups

A sample of how the project would look like is already here:

https://github.com/kolyshkin/opencontainers-cgroups

Initial Maintainers

Initial maintainers of the cgroups project would be:

Code of Conduct

This project would incorporate (by reference) the OCI Code of Conduct.

Governance and Releases

This project would incorporate the Governance and Releases processes from the
OCI project template: https://github.com/opencontainers/project-template.

Project Communications

The proposed project would continue to use existing channels in use by the OCI
developer community for communication including:

  • GitHub for issues and pull requests
  • The dev@opencontainers.org email list
  • The weekly OCI developer community conference call
  • The #OpenContainers IRC channel

Versioning / Roadmap

We'll start with v0.x until everything is stabilized after the split.

The plan is to have frequent releases to accommodate various users.

Frequently Asked Questions (FAQ)

Q: Does this change the OCI Charter or Scope Table?
A: No. Nothing in this proposal is intended to amend the
OCI Charter or
OCI Scope Table.

Q: Why move this out of the runc project?
A: To be able to reuse this in different container projects, while avoiding
code duplication and fragmentation. This involves adding functionality and
cutting releases to accomodate existing users other than runc.

Here's an example: kubernetes needs PSI stats from cgroups, and while the
low-level functionality needed (runc PR #3900)
was merged in July 2023, it was only made available to kubernetes when runc
v1.2.0 was tagged in October 2024. Such delays might result in using
unversioned dependencies, or copying the code around and thus fragmentation.

A longer term goal is make this set of packages even more modular, avoiding
excessive third-party dependencies.

Q: Who are the other target users of these packages?

A: CRI-O, kubernetes, cadvisor.


In order for this vote to pass, at least two thirds of the TOB (6 members) must vote in favour of this proposal:

@samuelkarp
Copy link
Member

samuelkarp commented Nov 7, 2024

New projects require a 2/3 vote of the TOB. The TOB has 9 seats, so this means 6/9 must approve.

@samuelkarp
Copy link
Member

TODO add someone from k8s org

Are there suggestions for this? Or should we remove for now and add later through the project's own governance?

@vbatts
Copy link
Member

vbatts commented Nov 7, 2024 via email

@AkihiroSuda
Copy link
Member

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Nov 7, 2024

TODO add someone from k8s org

Are there suggestions for this? Or should we remove for now and add later through the project's own governance?

I was hoping we can discuss it right here in this PR and add someone before we merge it.
Alas I'm not very familiar with k8s people; maybe @dims, @liggitt or @odinuge will volunteer.

I guess we also need LGTMs from all the to-be-maintainers (whom I've listed boldly without even talking to). Those are:

Please let us know here if you can keep maintaining what used to be opencontainers/runc/libcontainer/cgroups.

@kolyshkin
Copy link
Contributor Author

  • Can we just split the Go module without splitting the repo, for ease of contribution and running CI?

Possibly, but I don't really see the point. To me it looks like

  • contribution is going to be mostly the same, except maybe some new cgroup settings will require two PRs into two repos -- but it's good because we'll have some k8s people look at the changes.
  • CI is going to be more complicated with a monorepo (need to add cd libcontainers/cgroups && go test|mod|etc everywhere).

@AkihiroSuda
Copy link
Member

How will we test PRs for opencontainers/cgroups?
Many regressions cannot be exposed without running the whole runc CI for several distributions. (And sometimes it is not even enough)

@dims
Copy link

dims commented Nov 7, 2024

I am happy to!

@mrunalp
Copy link

mrunalp commented Nov 7, 2024

LGTM

@kolyshkin
Copy link
Contributor Author

How will we test PRs for opencontainers/cgroups?
Many regressions cannot be exposed without running the whole runc CI for several distributions. (And sometimes it is not even enough)

We can obviously test opencontainers/cgroups HEAD in runc repo, but that will only cover code which is already merged.

Good question. The best thing I can come up with is opening a PR in cgroups will result in opening a (linked) PR in a (hypothetical) runc-test repo (which is synced to main runc repo) with the libct/cg code vendored from the original PR. That is too complicated, I agree.

So, how do we currently deal with a case when someone makes a change in one of the packages that runc uses? Let's say it's a change in github.com/coreos/go-systemd or github.com/opencontainers/selinux or github.com/urfave/cli. Let's say the author tests their changes in runc repo. If they do, they will catch a bug early. If they don't, and there is a bug, we'll catch it when a new release is tagged, and we roll back to using a previous release and fix the bug in the next one. I suggest we deal with opencontainers/cgroups in the same way.

The alternative, I guess, is to keep cgroups in the same repo, add go.mod/go.sum to libcontainer/cgroups directory in runc, and make releases from there (marking them with libcontainer/cgroups/vX.Y.Z tag). This way we also have a problem (besides some extra releases and tags in runc repo), or should I say a dilemma -- whether to use a tagged version of cgroups for runc, or use a latest version (via replace github.com/opencontainers/runc/libcontainer/cgroups => ./libcontainer/cgroups directive in runc's go.mod). Problem is, we can not use both (or we need to double our CI jobs).

So, to my mind, it's better to treat cgroups as yet another package (set of packages) which is used by runc, and deal with it accordingly (fix bugs if/when found, test some major changes in both repos etc.)

@AkihiroSuda
Copy link
Member

replace github.com/opencontainers/runc/libcontainer/cgroups => ./libcontainer/cgroups

Maybe we can do an experiment with this, and then consider splitting the repo if it fails?

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@sudo-bmitch
Copy link
Contributor

sudo-bmitch commented Nov 7, 2024

In addition to verifying the new maintainers are on-board, I'd like to hear from the existing @opencontainers/runc-maintainers.

@haircommander
Copy link

I could help maintain from k8s side too. I've also had some experience in the library from using it in CRI-O

@AkihiroSuda I think @thaJeztah 's point here is the main reason to stick with opencontainers/cgroup for now. It feels less risky. then, when moved, we can always reevaluate the API and potentially merge the two repos (containerd/cgroups->libcontainer/cgroups)

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM.

Regarding the concerns that @AkihiroSuda raised, quite good questions, I think what @kolyshkin suggested seems good to start. Having a quick look, is not the most active place in runc the libcontainers/cgroups:

runc/libcontainer/cgroups :main$ git log . | grep ^Date| grep 2024 | wc -l
17
runc/libcontainer/cgroups :main$ git log . | grep ^Date| grep 2023 | wc -l
44
runc/libcontainer/cgroups :main$ git log . | grep ^Date| grep 2022 | wc -l
27
runc/libcontainer/cgroups :main$ cd ../../
runc :main$ git log . | grep ^Date| grep 2022 | wc -l
310
runc :main$ git log . | grep ^Date| grep 2023 | wc -l
426
runc :main$ git log . | grep ^Date| grep 2024 | wc -l
286

Doing this will clearly improve the current situation, if it ends up causing some other issues, when we know the actual issues we can think how to solve them.

The bringing it back to the runc repo and use a different go.mod, etc. might be an option. But it has many downsides too, I wouldn't want to start with that. I think the new repo is the best option to try now.

@odinuge
Copy link

odinuge commented Nov 7, 2024

Big +1 on this @kolyshkin, thanks for pushing. I'm also a big fan of this proposal instead of a fork.

I've also been working here a bit from the k8s side, and I'm happy to volunteer. So feel free to tag me in for reviews. I've poked at a lot of this code, and things like the systemd integration is very brittle, so having both people from k8s and the runc/opencontainer communities work together seems like a good idea here.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Updated the proposal; here's the diff:

-* TODO add someone from k8s org
+* Odin Ugedal <odin@uged.al> (@odinuge)
+* Peter Hunt <pehunt@redhat.com> (@haircommander)
+* Davanum Srinivas <davanum@gmail.com> (@dims)

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

LGTM
Cgroup is one of the most important core dependencies of runc, so I have a strong suggestion to add a runc(or containerd) integration test in CI to test PRs, but this may cause CI fail if the PR breaks some APIs or aims to fix a bug. Maybe we should have our own integration tests in the new repo. I think the time spent on CI is worth it.

@sudo-bmitch
Copy link
Contributor

@thaJeztah are you okay with being included as a moderator on this?

@kolyshkin thank you for adding the k8s maintainers. Are there any other pending changes to this before the TOB should vote on it? (We vote on the commit digest.)

@kolyshkin
Copy link
Contributor Author

@kolyshkin thank you for adding the k8s maintainers. Are there any other pending changes to this before the TOB should vote on it? (We vote on the commit digest.)

I do not have anything to add, thanks!

@thaJeztah
Copy link
Member

Sorry, I'm a bit slow on my notifications.

Generally, LGTM; for the testing part; yes, there may be some situations where it's slightly more complicated to verify changes; that said, indeed it's possible to add a replace rule in runc if someone is working on changes in cgroups that have corresponding changes in runc. To some extent, I consider the extra hurdle to take a benefit; it makes anyone making changes more aware if they're making changes to a public API (which should always have some friction 😄).

As to merging with containerd/cgroups; I don't think that needs to be a blocker for this effort; starting with a v0.x.x tag still allows for some flexibility on defining the public / stable API (we should definitely have a good look at what parts we're comfortable with being "public"), and we can maintain aliases in the old location for some time to help the transition. Definitely would be great if we could at some point consolidate the implementations (I don't think anyone really benefits from having 2 separate projects for this).

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

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

LGTM

@utam0k
Copy link
Member

utam0k commented Nov 12, 2024

Is the scope of this proposal only in the Go language? I'm looking for the possibility of supporting Rust as well.

@sudo-bmitch
Copy link
Contributor

Is the scope of this proposal only in the Go language? I'm looking for the possibility of supporting Rust as well.

The runc folder linked above is only Go code. I think rust would be a separate request, and would require enough maintainers that want to manage it to step up.

@thaJeztah
Copy link
Member

If other languages are to be added, one option could be to use go-cgroups as name for the repository (golang allows such prefixes, but still calling it as cgroups when importing).

@cyphar
Copy link
Member

cyphar commented Nov 14, 2024

@utam0k While it would be nice for us to have less of a Go-only focus in OCI, I do wonder what a user would expect from a Rust and Go implementation of some library functionality. Would they want feature parity? If so, there's a lot of ugly stuff you would need to reimplement in Rust that is mostly legacy stuff borne out of runc's history -- though hopefully we can remove some of that when we make cgroups a separate module.

How usable (as a library) do you feel is the cgroup code in youki?

@sudo-bmitch
Copy link
Contributor

This proposal has been approved with 7 approvals and 2 no-votes:

Approved:

No vote:

@sudo-bmitch sudo-bmitch merged commit 7b09788 into opencontainers:main Nov 14, 2024
1 check passed
@dims
Copy link

dims commented Nov 14, 2024

no-votes ... abstension?

@kolyshkin
Copy link
Contributor Author

Thanks everyone. I will start on implementing this in December.

@caniszczyk
Copy link
Contributor

FYI I created https://github.com/opencontainers/cgroups @kolyshkin

@utam0k
Copy link
Member

utam0k commented Dec 4, 2024

@utam0k While it would be nice for us to have less of a Go-only focus in OCI, I do wonder what a user would expect from a Rust and Go implementation of some library functionality. Would they want feature parity? If so, there's a lot of ugly stuff you would need to reimplement in Rust that is mostly legacy stuff borne out of runc's history -- though hopefully we can remove some of that when we make cgroups a separate module.

How usable (as a library) do you feel is the cgroup code in youki?

Indeed. I agree with you.

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

Successfully merging this pull request may close these issues.