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.

Sign up for GitHub

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
Merged

Apply minimumCPUShare to both task and container CPU shares #3156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions agent/api/task/task_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

taskCPUShares += minimumCPUShare
} else {
taskCPUShares += uint64(container.CPU)
}
}

// If there are are no CPU limits at task or container level,
// default task CPU shares
if taskCPUShares == 0 {
// Set default CPU shares
taskCPUShares = minimumCPUShare
}

return specs.LinuxCPU{
Shares: &taskCPUShares,
}
Expand Down
25 changes: 25 additions & 0 deletions agent/api/task/task_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,31 @@ func TestBuildLinuxResourceSpecWithoutTaskCPUWithContainerCPULimits(t *testing.T
assert.EqualValues(t, expectedLinuxResourceSpec, linuxResourceSpec)
}

// TestBuildLinuxResourceSpecWithoutTaskCPUWithLessThanMinimumContainerCPULimits validates behavior of CPU Shares
// when container CPU share is 1 (less than the current minimumCPUShare which is 2)
func TestBuildLinuxResourceSpecWithoutTaskCPUWithLessThanMinimumContainerCPULimits(t *testing.T) {
task := &Task{
Arn: validTaskArn,
Containers: []*apicontainer.Container{
{
Name: "C1",
CPU: uint(1),
},
},
}
expectedCPUShares := uint64(2)
expectedLinuxResourceSpec := specs.LinuxResources{
CPU: &specs.LinuxCPU{
Shares: &expectedCPUShares,
},
}

linuxResourceSpec, err := task.BuildLinuxResourceSpec(defaultCPUPeriod)

assert.NoError(t, err)
assert.EqualValues(t, expectedLinuxResourceSpec, linuxResourceSpec)
}

// TestBuildLinuxResourceSpecInvalidMem validates the linux resource spec builder
func TestBuildLinuxResourceSpecInvalidMem(t *testing.T) {
taskMemoryLimit := int64(taskMemoryLimit)
Expand Down