-
Notifications
You must be signed in to change notification settings - Fork 249
Add cgroupV2 CPUQuotaPeriodUSec support #367
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 cgroupV2 CPUQuotaPeriodUSec support #367
Conversation
63d749b to
0de39be
Compare
|
Looks like might need to resolve CI running on Ubuntu 20 runners, but IDK if there are any tests we can add for this property? |
0de39be to
ce7fe66
Compare
7e60dce to
0825bd5
Compare
|
I was able to add some unit tests for the property and CI is now only failing due to Ubuntu 20 runner no longer being available from GitHub. |
cgroup2/manager.go
Outdated
| if sdVer := systemdVersion(conn); sdVer >= 242 { | ||
| properties = append(properties, newSystemdProperty("CPUQuotaPeriodUSec", period)) | ||
| } else { | ||
| log.G(context.TODO()).WithField("version", sdVer).Debug("Systemd version is too old to support CPUQuotaPeriodUSec (setting will still be applied to cgroupfs)") |
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.
What does "setting will still be applied to cgroupfs" mean?
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.
Oop, good catch that was a bad copy from opencontainers/cgroups where that library has the cgroupfs fallback and writing some properites to file when not supported by systemd.
cgroup2/manager_test.go
Outdated
| require.NoError(t, err, "failed to connect to systemd") | ||
| defer conn.Close() | ||
|
|
||
| sdVer := systemdVersion(conn) |
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.
suggest perform this check at the beginning so the entire test can be skipped earlier
0825bd5 to
75313b9
Compare
|
Needs #368 |
henry118
left a comment
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.
LGTM. Thanks for working on this.
Signed-off-by: Austin Vazquez <[email protected]>
75313b9 to
88f5938
Compare
|
I rebased this for the CI updates. It should be good to review now. |
|
@containerd/maintainers |
|
@containerd/maintainers |
Addresses #365
This change adds
CPUQuotaPeriodUSecproperty for cgroupsv2. Heavily influenced by github.com/opencontainers/cgroups. Refactors a bit of the existing functions for testability.Added some unit tests for validating the property is properly set.