From a9dd886840f5525d99a28528009e506f80aae796 Mon Sep 17 00:00:00 2001
From: Onur Filiz <onfiliz@amazon.com>
Date: Mon, 30 Apr 2018 13:59:35 -0700
Subject: [PATCH] Fargate: Fix default container memory resource limit

---
 providers/aws/fargate/container.go      | 18 +++++----
 providers/aws/fargate/container_test.go | 49 ++++++++++++++++++-------
 2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/providers/aws/fargate/container.go b/providers/aws/fargate/container.go
index b475a7142..de9ccb9b3 100644
--- a/providers/aws/fargate/container.go
+++ b/providers/aws/fargate/container.go
@@ -25,8 +25,8 @@ const (
 	containerLogOptionStreamPrefix = "awslogs-stream-prefix"
 
 	// Default container resource limits.
-	containerDefaultCPULimit    = VCPU / 4
-	containerDefaultMemoryLimit = 512 * MiB
+	containerDefaultCPULimit    int64 = VCPU / 4
+	containerDefaultMemoryLimit int64 = 512 // * MiB
 )
 
 // Container is the representation of a Kubernetes container in Fargate.
@@ -166,12 +166,6 @@ func (cntr *container) setResourceRequirements(reqs *corev1.ResourceRequirements
 	// hard limit, which when exceeded, causes the container to be killed. MemoryReservation is a
 	// the amount of resources reserved for the container. At least one must be specified.
 	//
-	var quantity resource.Quantity
-	var reqQuantity resource.Quantity
-	var limQuantity resource.Quantity
-	var ok bool
-	var reqOk bool
-	var limOk bool
 
 	// Use the defaults if the container does not have any resource requirements.
 	cpu := containerDefaultCPULimit
@@ -180,6 +174,9 @@ func (cntr *container) setResourceRequirements(reqs *corev1.ResourceRequirements
 
 	// Compute CPU requirements.
 	if reqs != nil {
+		var quantity resource.Quantity
+		var ok bool
+
 		// Fargate tasks do not share resources with other tasks. Therefore the task and each
 		// container in it must be allocated their resource limits. Hence limits are preferred
 		// over requests.
@@ -200,6 +197,11 @@ func (cntr *container) setResourceRequirements(reqs *corev1.ResourceRequirements
 
 	// Compute memory requirements.
 	if reqs != nil {
+		var reqQuantity resource.Quantity
+		var limQuantity resource.Quantity
+		var reqOk bool
+		var limOk bool
+
 		// Find the memory request and limit, if available.
 		if reqs.Requests != nil {
 			reqQuantity, reqOk = reqs.Requests[corev1.ResourceMemory]
diff --git a/providers/aws/fargate/container_test.go b/providers/aws/fargate/container_test.go
index 617b5aa79..a563049db 100644
--- a/providers/aws/fargate/container_test.go
+++ b/providers/aws/fargate/container_test.go
@@ -124,16 +124,30 @@ func TestContainerResourceRequirementsWithRequestsAndLimits(t *testing.T) {
 // are translated to Fargate container resource requests correctly.
 func TestContainerResourceRequirementsTranslations(t *testing.T) {
 	type testCase struct {
-		requestedCPU          string
-		requestedMemoryInMiBs string
-		expectedCPU           int64
-		expectedMemoryInMiBs  int64
+		requestedCPU         string
+		requestedMemory      string
+		expectedCPU          int64
+		expectedMemoryInMiBs int64
 	}
 
 	// Expected and observed CPU quantities are in units of 1/1024th vCPUs.
 	var testCases = []testCase{
-		{"1m", "100Ki", 1, 1},
-		{"100m", "500Ki", 102, 1},
+		// Missing or partial resource requests.
+		{"", "", 256, 512},
+		{"100m", "", 102, 512},
+		{"", "256Mi", 256, 256},
+
+		// Minimum CPU request.
+		{"1m", "1Mi", 1, 1},
+
+		// Small memory request rounded up to the next MiB.
+		{"250m", "1Ki", 256, 1},
+		{"250m", "100Ki", 256, 1},
+		{"250m", "500Ki", 256, 1},
+		{"250m", "1024Ki", 256, 1},
+		{"250m", "1025Ki", 256, 2},
+
+		// Common combinations.
 		{"200m", "300Mi", 204, 300},
 		{"500m", "500Mi", 512, 500},
 		{"1000m", "512Mi", 1024, 512},
@@ -141,18 +155,27 @@ func TestContainerResourceRequirementsTranslations(t *testing.T) {
 		{"1500m", "1000Mi", 1536, 1000},
 		{"1500m", "1024Mi", 1536, 1024},
 		{"2", "2Gi", 2048, 2048},
-		{"8", "30Gi", 8192, 30 * 1024},
+		{"4", "30Gi", 4096, 30 * 1024},
+
+		// Very large requests.
+		{"8", "42Gi", 8192, 42 * 1024},
+		{"10", "128Gi", 10240, 128 * 1024},
 	}
 
 	for _, tc := range testCases {
 		t.Run(
-			fmt.Sprintf("cpu:%s,memory:%s", tc.requestedCPU, tc.requestedMemoryInMiBs),
+			fmt.Sprintf("cpu:%s,memory:%s", tc.requestedCPU, tc.requestedMemory),
 			func(t *testing.T) {
 				reqs := corev1.ResourceRequirements{
-					Limits: map[corev1.ResourceName]resource.Quantity{
-						corev1.ResourceCPU:    resource.MustParse(tc.requestedCPU),
-						corev1.ResourceMemory: resource.MustParse(tc.requestedMemoryInMiBs),
-					},
+					Limits: map[corev1.ResourceName]resource.Quantity{},
+				}
+
+				if tc.requestedCPU != "" {
+					reqs.Limits[corev1.ResourceCPU] = resource.MustParse(tc.requestedCPU)
+				}
+
+				if tc.requestedMemory != "" {
+					reqs.Limits[corev1.ResourceMemory] = resource.MustParse(tc.requestedMemory)
 				}
 
 				cntrSpec := anyContainerSpec
@@ -164,7 +187,7 @@ func TestContainerResourceRequirementsTranslations(t *testing.T) {
 				assert.Truef(t,
 					*cntr.definition.Cpu == tc.expectedCPU && *cntr.definition.Memory == tc.expectedMemoryInMiBs,
 					"requested (cpu:%v memory:%v) expected (cpu:%v memory:%v) observed (cpu:%v memory:%v)",
-					tc.requestedCPU, tc.requestedMemoryInMiBs,
+					tc.requestedCPU, tc.requestedMemory,
 					tc.expectedCPU, tc.expectedMemoryInMiBs,
 					*cntr.definition.Cpu, *cntr.definition.Memory)
 			})