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

Rename the package to github.com/containerd/cgroups/v2 #251

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

kzys
Copy link
Member

@kzys kzys commented Nov 4, 2022

Migrating off from gogo/protobuf is backward-incompatible for some
consumers that use proto-generated structs such as hcsshim.

Signed-off-by: Kazuyoshi Kato katokazu@amazon.com

@kzys kzys force-pushed the add-v2 branch 3 times, most recently from 498e53b to 78ab8d5 Compare November 4, 2022 20:29
@kzys kzys marked this pull request as ready for review November 4, 2022 20:34
@kzys kzys force-pushed the add-v2 branch 3 times, most recently from f0b5393 to 3bb251d Compare November 4, 2022 20:44
v2/manager.go Outdated
@@ -30,7 +30,7 @@ import (
"syscall"
"time"

"github.com/containerd/cgroups/v2/stats"
"github.com/containerd/cgroups/v2/v2/stats"
Copy link
Member Author

@kzys kzys Nov 4, 2022

Choose a reason for hiding this comment

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

This is ugly, but if we move types from v2/v2 to v2, having cgroups v3 (if there is) could be a headache. So I'd stay with this simple-yet-ugly naming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having github.com/containerd/cgroups/v2/cgroup2 (instead of v2/v2) could be an option, by following the filesystem type.

https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html

Copy link
Member

Choose a reason for hiding this comment

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

Can we just rename that to “unified” ?

Copy link
Member

Choose a reason for hiding this comment

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

The cgroup v1 impl can be named “legacy”

Copy link
Member Author

Choose a reason for hiding this comment

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

unified may work, but legacy is not future-proof. Would cgroup v2 be considered as legacy eventually?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, but unified.NewManager doesn't look better than cgroups.NewManager.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm settled to use cgroup2 now.

@kzys
Copy link
Member Author

kzys commented Nov 7, 2022

I'm open to move cgroup v1 files to cgroup1 directory, but I'd like to do that in a different PR since this one already touches 47 files.

@dmcgowan
Copy link
Member

dmcgowan commented Nov 7, 2022

I'm open to move cgroup v1 files to cgroup1 directory, but I'd like to do that in a different PR since this one already touches 47 files.

Does that change need to happen in this PR, this PR looks good. We can make the naming consistent in another PR since they will be separate commits anyway.

@kzys
Copy link
Member Author

kzys commented Nov 7, 2022

Does that change need to happen in this PR

I don't think so.

Migrating off from gogo/protobuf is backward-incompatible for some
consumers that use proto-generated structs such as hcsshim.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@AkihiroSuda
Copy link
Member

Can we now drop "v1" from github.com/containerd/cgroups/v2/stats/v1 ?

@kzys
Copy link
Member Author

kzys commented Nov 7, 2022

Can we now drop "v1" from github.com/containerd/cgroups/v2/stats/v1 ?

Yes. How about doing the following in a follow-up PR?

  • Move cgroups/v2/*.go to cgroups/v2/cgroup1
  • Move cgroups/v2/stats/v1 to cgroups/v2/cgroup1/stats

Then we have the most under cgroups/v2/cgroup1 and cgroups/v2/cgroup2.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Nov 8, 2022

How about doing the following in a follow-up PR?

SGTM

I thought it can be just done in this PR (with a separate commit maybe) but either is fine to me

@kzys kzys merged commit 3e546aa into containerd:main Nov 8, 2022
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.

3 participants