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

Apply minimumCPUShare to both task and container CPU shares #3156

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

yinyic
Copy link
Contributor

@yinyic yinyic commented Mar 25, 2022

Summary

taskCPUShares is used when creating task cgroup spec. We have a minimumCPUShare (equals 2) but it currently applies to container CPU shares only (more context under cpu section of the API doc).

This value should be applicable to task CPU share as well for the following reasons:

  1. We sum up container CPU shares and add that to task CPU share. Because a task must have at least one container, and the container CPU share will be >= 2, then task CPU share should naturally be >= 2.
  2. Cgroupv2 requires CPU share to be >= 2 due to this conversion from CPU shares to CPU weight. This is where the conversion happens in the containerd/cgroups library that we use.

Implementation details

When add each container CPU share to task CPU share, use minimumCPUShare if container CPU share is less than that.

Testing

New tests cover the changes: unit test

Additional manual testing: applied this commit to the cgroupv2 branch, started Agent on an AL2022 instance where cgroupv2 is enabled, and verified that task with container cpu share equals 1 can be started.

Description for the changelog

Apply minimumCPUShare to both task and container CPU shares

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -148,18 +148,13 @@ func (task *Task) buildImplicitLinuxCPUSpec() specs.LinuxCPU {
// aggregate container CPU shares when present
var taskCPUShares uint64
for _, container := range task.Containers {
if container.CPU > 0 {
if container.CPU < minimumCPUShare {
Copy link
Contributor

@singholt singholt Mar 25, 2022

Choose a reason for hiding this comment

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

if a task has 2 containers with no container-level CPU defined (container.CPU = 0) and no task-level CPU defined (task.CPU). then agent would set taskCPUShares to 4 but we want it to be 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - I believe that should be the correct behavior because we later on bump container CPU shares to 2, if the value specified in task def is smaller (or unspecified). So if you have 2 containers they will both have CPUShares=2 when passed to docker, which makes sense for the task CPU share to be 4 in that case.

@yinyic yinyic merged commit d5041e2 into aws:dev Mar 28, 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.

4 participants