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/configs: move cgroup configs to libct/cgroups #4472

Merged
merged 5 commits into from
Dec 14, 2024

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 24, 2024

This PR moves cgroup-related configuration from libcontainer/configs to libcontainer/cgroups,
while providing backward-compatible aliases.

This is the first step to implement the proposal to split libcontainer/cgroups out of runc (which stems from this k8s discussion).

@kolyshkin
Copy link
Contributor Author

No longer a draft; PTAL @opencontainers/runc-maintainers 🙏🏻

This is the first step to implement the proposal to split libcontainer/cgroups out of runc

@kolyshkin kolyshkin added this to the 1.3.0 milestone Dec 5, 2024
@dims
Copy link
Contributor

dims commented Dec 5, 2024

xref: kubernetes/kubernetes#128157

Copy link
Contributor

@dims dims left a comment

Choose a reason for hiding this comment

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

Love the clean separation. thank you!

(please remove [draft] from the PR title as well?)


import cg "github.com/opencontainers/runc/libcontainer/cgroups/configs"

// Deprecated: use [github.com/opencontainers/runc/libcontainer/cgroups/configs].
Copy link
Member

Choose a reason for hiding this comment

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

Not really important (just an alternative) as these don't point to specific types, I think it's possible to just add the // Deprecated: to the package;

package configs // Deprecated: use [github.com/opencontainers/runc/libcontainer/cgroups/configs].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -21,6 +21,7 @@ import (
"golang.org/x/sys/unix"

"github.com/opencontainers/runc/libcontainer/cgroups"
cgConfig "github.com/opencontainers/runc/libcontainer/cgroups/configs"
Copy link
Member

Choose a reason for hiding this comment

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

do you know if these types are also imported elsewhere separate from the cgroups package itself? considering if that was not the case, these types could potentially be merged with the cgroups package 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question.

Looks like there's no need to have a separate package for cgroups-related structures.

The PR is now updated to use libcontainer/cgroups. The only negative effect is the patch is now bigger (had to replace configs with cgroups everywhere.

PTAL @thaJeztah 🙏🏻

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I wanted to give the changes a quick try to see if nothing bad fell out of it, so I opened some test-PRs in moby to double-check;

@kolyshkin kolyshkin changed the title [draft] libct/configs: move cgroup stuff to libct/cg/configs libct/configs: move cgroup stuff to libct/cg/configs Dec 5, 2024
@kolyshkin kolyshkin changed the title libct/configs: move cgroup stuff to libct/cg/configs libct/configs: move cgroup configs to libct/cgroups Dec 12, 2024
@kolyshkin kolyshkin force-pushed the cg-cfg branch 2 times, most recently from efab503 to 157e5c6 Compare December 12, 2024 02:33
These:

> Error: libcontainer/cgroups/fs2/cpu.go:15:6: var-naming: func isCpuSet should be isCPUSet (revive)
> func isCpuSet(r *cgroups.Resources) bool {
>      ^
> Error: libcontainer/cgroups/fs2/cpu.go:19:6: var-naming: func setCpu should be setCPU (revive)
> func setCpu(dirPath string, r *cgroups.Resources) error {
>      ^

They are going to be shown after next commits because of linter-extra CI
job (which, due to major changes, now thinks it's a new code so extra
linters apply).

Fixing it beforehand.

Signed-off-by: Kir Kolyshkin <[email protected]>
We have quite a few external users of libcontainer/cgroups packages,
and they all have to depend on libcontainer/configs as well.

Let's move cgroup-related configuration to libcontainer/croups.

Signed-off-by: Kir Kolyshkin <[email protected]>
@AkihiroSuda AkihiroSuda merged commit 28b65d3 into opencontainers:main Dec 14, 2024
40 checks passed
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.

5 participants