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.
What does "setting will still be applied to cgroupfs" mean?
There was a problem hiding this comment.
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.
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.
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.