define maximumCPUShares

Author: singholt

agent/api/task/task_linux.go

@@ -43,7 +43,11 @@ import (
 
 const (
 	// Reference: http://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html
-	minimumCPUShare = 2
+	// These min, max values are defined in the kernel:
+	// https://github.com/torvalds/linux/blob/0bddd227f3dc55975e2b8dfa7fc6f959b062a2c7/kernel/sched/sched.h#L427-L428
+	// We need to make sure task and container cgroups define cpu shares within this range.
+	minimumCPUShares = 2
+	maximumCPUShares = 262144
 
 	minimumCPUPercent = 0
 	bytesPerMegabyte  = 1024 * 1024
@@ -178,8 +182,14 @@ func (task *Task) buildImplicitLinuxCPUSpec() specs.LinuxCPU {
 	// aggregate container CPU shares when present
 	var taskCPUShares uint64
 	for _, container := range task.Containers {
-		if container.CPU < minimumCPUShare {
-			taskCPUShares += minimumCPUShare
+		if container.CPU < minimumCPUShares {
+			taskCPUShares += minimumCPUShares
+		} else if container.CPU > maximumCPUShares {
+			// If one of the containers is requesting more than the max permitted CPU shares,
+			// then we set taskCPUShares to be the maximumCPUShares and break out of this loop.
+			// This is to prevent the taskCPUShares to be more than the max CPU shares permitted by the Linux kernel.
+			taskCPUShares = maximumCPUShares
+			break
 		} else {
 			taskCPUShares += uint64(container.CPU)
 		}
@@ -236,12 +246,18 @@ func (task *Task) overrideCgroupParent(hostConfig *dockercontainer.HostConfig) e
 // Docker silently converts 0 to 1024 CPU shares, which is probably not what we
 // want.  Instead, we convert 0 to 2 to be closer to expected behavior. The
 // reason for 2 over 1 is that 1 is an invalid value (Linux's choice, not Docker's).
+// Similarly, if the container CPU shares is more than the max permitted value (Linux's choice again),
+// we set it to the maximum.
 func (task *Task) dockerCPUShares(containerCPU uint) int64 {
-	if containerCPU <= 1 {
+	if containerCPU < minimumCPUShares {
 		seelog.Debugf(
-			"Converting CPU shares to allowed minimum of 2 for task arn: [%s] and cpu shares: %d",
-			task.Arn, containerCPU)
-		return 2
+			"Converting CPU shares to allowed minimum of %d for task arn: [%s] and cpu shares: %d",
+			minimumCPUShares, task.Arn, containerCPU)
+		return minimumCPUShares
+	} else if containerCPU > maximumCPUShares {
+		seelog.Debugf("Converting CPU shares to allowed maximum of %d for task arn [%s] and cpu shares: %d",
+			maximumCPUShares, task.Arn, containerCPU)
+		return maximumCPUShares
 	}
 	return int64(containerCPU)
 }

agent/api/task/task_linux_test.go

@@ -425,7 +425,7 @@ func TestBuildLinuxResourceSpecWithoutTaskCPULimits(t *testing.T) {
 			},
 		},
 	}
-	expectedCPUShares := uint64(minimumCPUShare)
+	expectedCPUShares := uint64(minimumCPUShares)
 	expectedLinuxResourceSpec := specs.LinuxResources{
 		CPU: &specs.LinuxCPU{
 			Shares: &expectedCPUShares,
@@ -449,7 +449,7 @@ func TestBuildLinuxResourceSpecWithoutTaskCPULimits_WithPidLimits(t *testing.T)
 			},
 		},
 	}
-	expectedCPUShares := uint64(minimumCPUShare)
+	expectedCPUShares := uint64(minimumCPUShares)
 	expectedLinuxResourceSpec := specs.LinuxResources{
 		CPU: &specs.LinuxCPU{
 			Shares: &expectedCPUShares,
@@ -490,7 +490,7 @@ func TestBuildLinuxResourceSpecWithoutTaskCPUWithContainerCPULimits(t *testing.T
 }
 
 // TestBuildLinuxResourceSpecWithoutTaskCPUWithLessThanMinimumContainerCPULimits validates behavior of CPU Shares
-// when container CPU share is 1 (less than the current minimumCPUShare which is 2)
+// when container CPU share is 1 (less than the current minimumCPUShares which is 2)
 func TestBuildLinuxResourceSpecWithoutTaskCPUWithLessThanMinimumContainerCPULimits(t *testing.T) {
 	task := &Task{
 		Arn: validTaskArn,
@@ -1578,3 +1578,145 @@ func TestBuildCNIBridgeModeWithServiceConnect(t *testing.T) {
 		})
 	}
 }
+
+// TestBuildImplicitLinuxCPUSpec tests the task's CPU spec generation when "task.CPU" is not explicitly set.
+func TestBuildImplicitLinuxCPUSpec(t *testing.T) {
+	testCases := []struct {
+		name                string
+		task                *Task
+		expectedTaskCPUSpec specs.LinuxCPU
+	}{
+		{
+			name: "2 containers: each with CPU within permitted range",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(10),
+					},
+					{
+						CPU: uint(20),
+					},
+				},
+			},
+			expectedTaskCPUSpec: specs.LinuxCPU{
+				Shares: aws.Uint64(30),
+			},
+		},
+		{
+			name: "2 containers: 1 of them with CPU below min",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(1),
+					},
+					{
+						CPU: uint(10),
+					},
+				},
+			},
+			expectedTaskCPUSpec: specs.LinuxCPU{
+				Shares: aws.Uint64(uint64(12)),
+			},
+		},
+		{
+			name: "2 containers: 1 of them with CPU above max",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(1),
+					},
+					{
+						CPU: uint(364544),
+					},
+				},
+			},
+			expectedTaskCPUSpec: specs.LinuxCPU{
+				Shares: aws.Uint64(uint64(maximumCPUShares)),
+			},
+		},
+		{
+			name: "3 containers: 1 with CPU within permitted range, 1 above max, and 1 below min",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(5),
+					},
+					{
+						CPU: uint(1),
+					},
+					{
+						CPU: uint(364544),
+					},
+				},
+			},
+			expectedTaskCPUSpec: specs.LinuxCPU{
+				Shares: aws.Uint64(uint64(maximumCPUShares)),
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := tc.task.buildImplicitLinuxCPUSpec()
+			assert.Equal(t, tc.expectedTaskCPUSpec, result)
+		})
+	}
+}
+
+// TestDockerCPUShares tests the dockerCPUShares() conversion method,
+// which is used to send the container shares info is sent to Docker
+func TestDockerCPUShares(t *testing.T) {
+	testCases := []struct {
+		name                    string
+		task                    *Task
+		expectedContainerShares int64
+	}{
+		{
+			name: "container CPU within permitted range",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(10),
+					},
+				},
+			},
+			expectedContainerShares: int64(10),
+		},
+		{
+			name: "container CPU below min",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(1),
+					},
+				},
+			},
+			expectedContainerShares: int64(minimumCPUShares),
+		},
+		{
+			name: "container CPU above max",
+			task: &Task{
+				Arn: validTaskArn,
+				Containers: []*apicontainer.Container{
+					{
+						CPU: uint(364544),
+					},
+				},
+			},
+			expectedContainerShares: int64(maximumCPUShares),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result := tc.task.dockerCPUShares(tc.task.Containers[0].CPU)
+			assert.Equal(t, tc.expectedContainerShares, result)
+		})
+	}
+}