Fix memory limit validation for compose 3

SoManyHs · SoManyHs · commit f7495b898e13 · 2018-07-05T12:25:22.000-07:00
Previously, specifying memory reservation without memory limit would cause a validation error. This fix allows either value to be specified without the other, in either docker compose or ecs-params. Fixes aws#546
diff --git a/ecs-cli/modules/cli/compose/project/project_parseV1V2.go b/ecs-cli/modules/cli/compose/project/project_parseV1V2.go
@@ -75,6 +75,11 @@ func convertV1V2ToContainerConfig(context *project.Context, serviceName string,
 	memory := adapter.ConvertToMemoryInMB(int64(service.MemLimit))
 	memoryReservation := adapter.ConvertToMemoryInMB(int64(service.MemReservation))
 
+	// Validate memory and memory reservation
+	if memory == 0 && memoryReservation != 0 {
+		memory = memoryReservation
+	}
+
 	mountPoints, err := adapter.ConvertToMountPoints(service.Volumes, volumes)
 	if err != nil {
 		return nil, err
diff --git a/ecs-cli/modules/cli/compose/project/project_parseV1V2_test.go b/ecs-cli/modules/cli/compose/project/project_parseV1V2_test.go
@@ -115,7 +115,7 @@ func TestParseV1V2_Version1_HappyPath(t *testing.T) {
   devices:
    - "/dev/sda:/dev/sdd:r"
    - "/dev/sdd:/dev/xdr"
-   - "/dev/sda" 
+   - "/dev/sda"
   dns:
    - 1.2.3.4
   dns_search: search.example.com
@@ -505,6 +505,41 @@ services:
 	assert.Equal(t, expectedEnv, web.Environment, "Expected Environment to match")
 }
 
+func TestParseV1V2_MemoryValidation(t *testing.T) {
+	// Setup docker-compose file
+	memory := int64(128)
+	composeFileString := `version: '2'
+services:
+  web:
+    image: webapp
+    mem_reservation: 128m`
+
+	tmpfile, err := ioutil.TempFile("", "test")
+	assert.NoError(t, err, "Unexpected error in creating test file")
+
+	defer os.Remove(tmpfile.Name())
+
+	_, err = tmpfile.Write([]byte(composeFileString))
+	assert.NoError(t, err, "Unexpected error in writing to test file")
+
+	err = tmpfile.Close()
+	assert.NoError(t, err, "Unexpected error closing file")
+
+	// Set up project
+	project := setupTestProject(t)
+	project.ecsContext.ComposeFiles = append(project.ecsContext.ComposeFiles, tmpfile.Name())
+
+	actualConfigs, err := project.parseV1V2()
+	assert.NoError(t, err, "Unexpected error parsing file")
+
+	// verify wordpress ServiceConfig
+	web, err := getContainerConfigByName("web", actualConfigs)
+	if assert.NoError(t, err) {
+		assert.Equal(t, memory, web.Memory, "Expected Memory to match")
+		assert.Equal(t, memory, web.MemoryReservation, "Expected MemoryReservation to match")
+	}
+}
+
 func getContainerConfigByName(name string, configs *[]adapter.ContainerConfig) (*adapter.ContainerConfig, error) {
 	for _, config := range *configs {
 		if config.Name == name {
diff --git a/ecs-cli/modules/utils/compose/convert_task_definition.go b/ecs-cli/modules/utils/compose/convert_task_definition.go
@@ -166,6 +166,7 @@ func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *C
 	if ecsContainerDef != nil {
 		outputContDef.Essential = aws.Bool(ecsContainerDef.Essential)
 
+		// CPU and Memory are expected to be set here if compose v3 was used
 		cpu = getResourceValue(inputCfg.Name, cpu, ecsContainerDef.Cpu, "CPU")
 
 		ecsMemInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.Memory))
@@ -175,17 +176,23 @@ func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *C
 		memRes = getResourceValue(inputCfg.Name, memRes, ecsMemResInMB, "MemoryReservation")
 	}
 
-	if mem < memRes {
-		return nil, errors.New("mem_limit must be greater than mem_reservation")
-	}
-
 	// One or both of memory and memoryReservation is required to register a task definition with ECS
 	// If neither is provided by 1) compose file or 2) ecs-params, set default
 	// NOTE: Docker does not set a memory limit for containers
 	if mem == 0 && memRes == 0 {
 		mem = defaultMemLimit
 	}
 
+	// Docker compose allows specifying memory reservation with memory, so
+	// we should default to minimum allowable value for memory hard limit
+	if mem == 0 && memRes != 0 {
+		mem = memRes
+	}
+
+	if mem < memRes {
+		return nil, errors.New("mem_limit must be greater than mem_reservation")
+	}
+
 	outputContDef.SetCpu(cpu)
 	if mem != 0 {
 		outputContDef.SetMemory(mem)
@@ -211,7 +218,7 @@ func showResourceOverrideMsg(serviceName string, oldValue int64, newValue int64,
 	overrideMsg := "Using ecs-params value as override (was %v but is now %v)"
 
 	log.WithFields(log.Fields{
-		"option name": option,
+		"option name":  option,
 		"service name": serviceName,
 	}).Infof(overrideMsg, oldValue, newValue)
 }
diff --git a/ecs-cli/modules/utils/compose/convert_task_definition_test.go b/ecs-cli/modules/utils/compose/convert_task_definition_test.go
@@ -332,12 +332,13 @@ func TestConvertToTaskDefinitionLaunchTypeFargate(t *testing.T) {
 }
 
 // Tests for ConvertToContainerDefinition with ECS Params
-func TestConvertToTaskDefinitionWithECSParams_DefaultMemoryLessThanMemoryRes(t *testing.T) {
+func TestConvertToTaskDefinitionWithECSParams_ComposeMemoryLessThanMemoryRes(t *testing.T) {
 	// set up containerConfig w/o value for Memory
 	containerConfig := &adapter.ContainerConfig{
-		Name:  "web",
-		Image: "httpd",
-		CPU:   int64(5),
+		Name:   "web",
+		Image:  "httpd",
+		CPU:    int64(5),
+		Memory: int64(512),
 	}
 
 	// define ecs-params value we expect to be present in final containerDefinition
@@ -942,7 +943,7 @@ task_definition:
 	}
 }
 
-func TestConvertToTaskDefinitionWithECSParams_SomeContainerResourcesProvided(t *testing.T) {
+func TestConvertToTaskDefinitionWithECSParams_MemLimitOnlyProvided(t *testing.T) {
 	// set up containerConfig w/o value for Memory
 	containerConfig := &adapter.ContainerConfig{
 		Name:              "web",
@@ -993,6 +994,48 @@ task_definition:
 	}
 }
 
+func TestConvertToTaskDefinitionWithECSParams_MemReservationOnlyProvided(t *testing.T) {
+	containerConfigs := testContainerConfigs([]string{"web"})
+
+	// define ecs-params value we expect to be present in final containerDefinition
+	webMem := int64(20)
+
+	ecsParamsString := `version: 1
+task_definition:
+  services:
+    web:
+      mem_reservation: 20m`
+
+	content := []byte(ecsParamsString)
+
+	tmpfile, err := ioutil.TempFile("", "ecs-params")
+	assert.NoError(t, err, "Could not create ecs params tempfile")
+
+	defer os.Remove(tmpfile.Name())
+
+	_, err = tmpfile.Write(content)
+	assert.NoError(t, err, "Could not write data to ecs params tempfile")
+
+	err = tmpfile.Close()
+	assert.NoError(t, err, "Could not close tempfile")
+
+	ecsParamsFileName := tmpfile.Name()
+	ecsParams, err := ReadECSParams(ecsParamsFileName)
+	assert.NoError(t, err, "Could not read ECS Params file")
+
+	taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, containerConfigs, "", ecsParams)
+
+	containerDefs := taskDefinition.ContainerDefinitions
+	web := findContainerByName("web", containerDefs)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, webMem, aws.Int64Value(web.Memory), "Expected Memory to match")
+		assert.Equal(t, webMem, aws.Int64Value(web.MemoryReservation), "Expected MemoryReservation to match")
+		// check config values not present in ecs-params are present
+		assert.Empty(t, web.Cpu, "Expected CPU to be empty")
+	}
+}
+
 func TestConvertToTaskDefinitionWithECSParams_MemResGreaterThanMemLimit(t *testing.T) {
 	containerConfig := &adapter.ContainerConfig{Name: "web"}
 	ecsParamsString := `version: 1
@@ -1058,28 +1101,56 @@ task_definition:
 	}
 }
 
-func TestMemReservationHigherThanMemLimit(t *testing.T) {
-	cpu := int64(131072) // 128 * 1024
-	command := "cmd"
-	hostname := "local360"
-	image := "testimage"
-	memory := int64(65536) // 64mb
-	privileged := true
-	readOnly := true
-	user := "user"
-	workingDir := "/var"
+func TestConvertToTaskDefinition_MemLimitOnlyProvided(t *testing.T) {
+	webMem := int64(1048576)
+	containerConfig := &adapter.ContainerConfig{
+		Name:   "web",
+		Memory: webMem,
+	}
+
+	taskDefinition := convertToTaskDefinitionInTest(t, containerConfig, "", "")
+	containerDefs := taskDefinition.ContainerDefinitions
+	web := findContainerByName("web", containerDefs)
 
+	assert.Equal(t, webMem, aws.Int64Value(web.Memory), "Expected Memory to match")
+	assert.Empty(t, web.MemoryReservation, "Expected MemoryReservation to be empty")
+	assert.Empty(t, web.Cpu, "Expected CPU to be empty")
+}
+
+func TestConvertToTaskDefinition_MemReservationOnlyProvided(t *testing.T) {
+	webMem := int64(1048576)
+	containerConfig := &adapter.ContainerConfig{
+		Name:              "web",
+		MemoryReservation: webMem,
+	}
+
+	taskDefinition := convertToTaskDefinitionInTest(t, containerConfig, "", "")
+	containerDefs := taskDefinition.ContainerDefinitions
+	web := findContainerByName("web", containerDefs)
+
+	assert.Equal(t, webMem, aws.Int64Value(web.Memory), "Expected Memory to match")
+	assert.Equal(t, webMem, aws.Int64Value(web.MemoryReservation), "Expected MemoryReservation to match")
+	assert.Empty(t, web.Cpu, "Expected CPU to be empty")
+}
+
+func TestConvertToTaskDefinition_NoMemoryProvided(t *testing.T) {
+	containerConfig := &adapter.ContainerConfig{
+		Name: "web",
+	}
+
+	taskDefinition := convertToTaskDefinitionInTest(t, containerConfig, "", "")
+	containerDefs := taskDefinition.ContainerDefinitions
+	web := findContainerByName("web", containerDefs)
+
+	assert.Equal(t, aws.Int64(defaultMemLimit), web.Memory, "Expected Memory to match default")
+	assert.Empty(t, web.MemoryReservation, "Expected MemoryReservation to be empty")
+	assert.Empty(t, web.Cpu, "Expected CPU to be empty")
+}
+
+func TestMemReservationHigherThanMemLimit(t *testing.T) {
 	containerConfig := adapter.ContainerConfig{
-		CPU:               cpu,
-		Command:           []string{command},
-		Hostname:          hostname,
-		Image:             image,
-		Memory:            int64(524288) * memory,
-		MemoryReservation: int64(1048576) * memory,
-		Privileged:        privileged,
-		ReadOnly:          readOnly,
-		User:              user,
-		WorkingDirectory:  workingDir,
+		Memory:            int64(524288),
+		MemoryReservation: int64(1048576),
 	}
 
 	volumeConfigs := adapter.NewVolumes()
