Skip to content

Commit e4d9d13

Browse files
allisaurusallisaurus
committed
Refactor merging ecs-params service resources with task def vals
1 parent 7d68914 commit e4d9d13

File tree

2 files changed

+173
-55
lines changed

2 files changed

+173
-55
lines changed

‎ecs-cli/modules/utils/compose/convert_task_definition.go

Lines changed: 46 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -241,15 +241,7 @@ func isZero(v reflect.Value) bool {
241241
func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *ContainerDef) (*ecs.ContainerDefinition, error) {
242242
outputContDef := &ecs.ContainerDefinition{}
243243

244-
mem := inputCfg.Memory
245-
memoryReservation := inputCfg.MemoryReservation
246-
247-
if mem != 0 && memoryReservation != 0 && mem < memoryReservation {
248-
return nil, errors.New("mem_limit must be greater than mem_reservation")
249-
}
250-
251244
// Populate ECS container definition, offloading the validation to aws-sdk
252-
outputContDef.SetCpu(inputCfg.CPU)
253245
outputContDef.SetCommand(aws.StringSlice(inputCfg.Command))
254246
outputContDef.SetDnsSearchDomains(aws.StringSlice(inputCfg.DNSSearchDomains))
255247
outputContDef.SetDnsServers(aws.StringSlice(inputCfg.DNSServers))
@@ -264,14 +256,6 @@ func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *C
264256
outputContDef.SetImage(inputCfg.Image)
265257
outputContDef.SetLinks(aws.StringSlice(inputCfg.Links)) // TODO, read from external links
266258
outputContDef.SetLogConfiguration(inputCfg.LogConfiguration)
267-
268-
if mem != 0 {
269-
outputContDef.SetMemory(mem)
270-
}
271-
if memoryReservation != 0 {
272-
outputContDef.SetMemoryReservation(memoryReservation)
273-
}
274-
275259
outputContDef.SetMountPoints(inputCfg.MountPoints)
276260
outputContDef.SetName(inputCfg.Name)
277261
outputContDef.SetPrivileged(inputCfg.Privileged)
@@ -308,52 +292,65 @@ func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *C
308292
outputContDef.LinuxParameters.SetTmpfs(inputCfg.Tmpfs)
309293
}
310294

311-
// Set fields from ecs-params file
295+
// initialize container resources from inputCfg
296+
cpu := inputCfg.CPU
297+
mem := inputCfg.Memory
298+
memRes := inputCfg.MemoryReservation
299+
300+
// Set essential & resource fields from ecs-params file, if present
312301
if ecsContainerDef != nil {
313302
outputContDef.Essential = aws.Bool(ecsContainerDef.Essential)
314303

315-
overrideMsg := "Using ecs-params value as override (was %v but is now %v)"
316-
if ecsContainerDef.Cpu > 0 {
317-
if outputContDef.Cpu != nil && *outputContDef.Cpu > 0 {
318-
log.WithFields(log.Fields{
319-
"option name": "Cpu",
320-
"service name": inputCfg.Name,
321-
}).Infof(overrideMsg, *outputContDef.Cpu, ecsContainerDef.Cpu)
322-
}
323-
outputContDef.Cpu = aws.Int64(ecsContainerDef.Cpu)
324-
}
325-
memInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.Memory))
326-
if memInMB > 0 {
327-
if outputContDef.Memory != nil {
328-
log.WithFields(log.Fields{
329-
"option name": "Memory",
330-
"service name": inputCfg.Name,
331-
}).Infof(overrideMsg, *outputContDef.Memory, memInMB)
332-
}
333-
outputContDef.Memory = aws.Int64(memInMB)
334-
}
335-
memResInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.MemoryReservation))
336-
if memResInMB > 0 {
337-
if outputContDef.MemoryReservation != nil {
338-
log.WithFields(log.Fields{
339-
"option name": "MemoryReservation",
340-
"service name": inputCfg.Name,
341-
}).Infof(overrideMsg, *outputContDef.MemoryReservation, memResInMB)
342-
}
343-
outputContDef.MemoryReservation = aws.Int64(memResInMB)
344-
}
304+
cpu = getResourceValue(inputCfg.Name, cpu, ecsContainerDef.Cpu)
305+
306+
ecsMemInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.Memory))
307+
mem = getResourceValue(inputCfg.Name, mem, ecsMemInMB)
308+
309+
ecsMemResInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.MemoryReservation))
310+
memRes = getResourceValue(inputCfg.Name, memRes, ecsMemResInMB)
311+
}
312+
313+
if mem < memRes {
314+
return nil, errors.New("mem_limit must be greater than mem_reservation")
345315
}
346316

347317
// One or both of memory and memoryReservation is required to register a task definition with ECS
348318
// If neither is provided by 1) compose file or 2) ecs-params, set default
349319
// NOTE: Docker does not set a memory limit for containers
350-
if outputContDef.Memory == nil && outputContDef.MemoryReservation == nil {
351-
outputContDef.SetMemory(defaultMemLimit)
320+
if mem == 0 && memRes == 0 {
321+
mem = defaultMemLimit
322+
}
323+
324+
outputContDef.SetCpu(cpu)
325+
if mem != 0 {
326+
outputContDef.SetMemory(mem)
327+
}
328+
if memRes != 0 {
329+
outputContDef.SetMemoryReservation(memRes)
352330
}
353331

354332
return outputContDef, nil
355333
}
356334

335+
func getResourceValue(serviceName string, inputVal, ecsVal int64) int64 {
336+
if ecsVal == 0 {
337+
return inputVal
338+
}
339+
if inputVal > 0 {
340+
showResourceOverrideMsg(serviceName, inputVal, ecsVal)
341+
}
342+
return ecsVal
343+
}
344+
345+
func showResourceOverrideMsg(serviceName string, oldValue int64, newValue int64) {
346+
overrideMsg := "Using ecs-params value as override (was %v but is now %v)"
347+
348+
log.WithFields(log.Fields{
349+
"option name": "MemoryReservation",
350+
"service name": serviceName,
351+
}).Infof(overrideMsg, oldValue, newValue)
352+
}
353+
357354
// convertToECSVolumes transforms the map of hostPaths to the format of ecs.Volume
358355
func convertToECSVolumes(hostPaths *adapter.Volumes) []*ecs.Volume {
359356
output := []*ecs.Volume{}

‎ecs-cli/modules/utils/compose/convert_task_definition_test.go

Lines changed: 127 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,44 @@ func TestConvertToTaskDefinition(t *testing.T) {
247247
}
248248

249249
// ConvertToContainerDefinition tests
250+
251+
func TestConvertToTaskDefinitionWithECSParams_DefaultMemoryLessThanMemoryRes(t *testing.T) {
252+
// set up containerConfig w/o value for Memory
253+
containerConfig := &adapter.ContainerConfig{
254+
Name: "web",
255+
Image: "httpd",
256+
CPU: int64(5),
257+
}
258+
259+
// define ecs-params value we expect to be present in final containerDefinition
260+
ecsParamsString := `version: 1
261+
task_definition:
262+
services:
263+
web:
264+
mem_reservation: 1g`
265+
266+
content := []byte(ecsParamsString)
267+
268+
tmpfile, err := ioutil.TempFile("", "ecs-params")
269+
assert.NoError(t, err, "Could not create ecs params tempfile")
270+
271+
defer os.Remove(tmpfile.Name())
272+
273+
_, err = tmpfile.Write(content)
274+
assert.NoError(t, err, "Could not write data to ecs params tempfile")
275+
276+
err = tmpfile.Close()
277+
assert.NoError(t, err, "Could not close tempfile")
278+
279+
ecsParamsFileName := tmpfile.Name()
280+
ecsParams, err := ReadECSParams(ecsParamsFileName)
281+
assert.NoError(t, err, "Could not read ECS Params file")
282+
283+
containerConfigs := []adapter.ContainerConfig{*containerConfig}
284+
_, err = convertToTaskDefWithEcsParamsInTest(t, containerConfigs, "", ecsParams)
285+
286+
assert.Error(t, err, "Expected error because memory reservation is greater than memory limit")
287+
}
250288
func TestConvertToTaskDefinitionWithNoSharedMemorySize(t *testing.T) {
251289
containerConfig := &adapter.ContainerConfig{
252290
ShmSize: int64(0),
@@ -757,12 +795,12 @@ task_definition:
757795
content := []byte(ecsParamsString)
758796

759797
tmpfile, err := ioutil.TempFile("", "ecs-params")
760-
assert.NoError(t, err, "Could not create ecs fields tempfile")
798+
assert.NoError(t, err, "Could not create ecs params tempfile")
761799

762800
defer os.Remove(tmpfile.Name())
763801

764802
_, err = tmpfile.Write(content)
765-
assert.NoError(t, err, "Could not write data to ecs fields tempfile")
803+
assert.NoError(t, err, "Could not write data to ecs params tempfile")
766804

767805
err = tmpfile.Close()
768806
assert.NoError(t, err, "Could not close tempfile")
@@ -813,12 +851,12 @@ task_definition:
813851
content := []byte(ecsParamsString)
814852

815853
tmpfile, err := ioutil.TempFile("", "ecs-params")
816-
assert.NoError(t, err, "Could not create ecs fields tempfile")
854+
assert.NoError(t, err, "Could not create ecs params tempfile")
817855

818856
defer os.Remove(tmpfile.Name())
819857

820858
_, err = tmpfile.Write(content)
821-
assert.NoError(t, err, "Could not write data to ecs fields tempfile")
859+
assert.NoError(t, err, "Could not write data to ecs params tempfile")
822860

823861
err = tmpfile.Close()
824862
assert.NoError(t, err, "Could not close tempfile")
@@ -858,12 +896,12 @@ task_definition:
858896
content := []byte(ecsParamsString)
859897

860898
tmpfile, err := ioutil.TempFile("", "ecs-params")
861-
assert.NoError(t, err, "Could not create ecs fields tempfile")
899+
assert.NoError(t, err, "Could not create ecs params tempfile")
862900

863901
defer os.Remove(tmpfile.Name())
864902

865903
_, err = tmpfile.Write(content)
866-
assert.NoError(t, err, "Could not write data to ecs fields tempfile")
904+
assert.NoError(t, err, "Could not write data to ecs params tempfile")
867905

868906
err = tmpfile.Close()
869907
assert.NoError(t, err, "Could not close tempfile")
@@ -885,6 +923,89 @@ task_definition:
885923
}
886924
}
887925

926+
func TestConvertToTaskDefinitionWithECSParams_SomeContainerResourcesProvided(t *testing.T) {
927+
// set up containerConfig w/o value for Memory
928+
containerConfig := &adapter.ContainerConfig{
929+
Name: "web",
930+
Image: "httpd",
931+
CPU: int64(5),
932+
Memory: int64(15),
933+
MemoryReservation: int64(10),
934+
}
935+
936+
// define ecs-params value we expect to be present in final containerDefinition
937+
webMem := int64(20)
938+
939+
ecsParamsString := `version: 1
940+
task_definition:
941+
services:
942+
web:
943+
mem_limit: 20m`
944+
945+
content := []byte(ecsParamsString)
946+
947+
tmpfile, err := ioutil.TempFile("", "ecs-params")
948+
assert.NoError(t, err, "Could not create ecs params tempfile")
949+
950+
defer os.Remove(tmpfile.Name())
951+
952+
_, err = tmpfile.Write(content)
953+
assert.NoError(t, err, "Could not write data to ecs params tempfile")
954+
955+
err = tmpfile.Close()
956+
assert.NoError(t, err, "Could not close tempfile")
957+
958+
ecsParamsFileName := tmpfile.Name()
959+
ecsParams, err := ReadECSParams(ecsParamsFileName)
960+
assert.NoError(t, err, "Could not read ECS Params file")
961+
962+
containerConfigs := []adapter.ContainerConfig{*containerConfig}
963+
taskDefinition, err := convertToTaskDefWithEcsParamsInTest(t, containerConfigs, "", ecsParams)
964+
965+
containerDefs := taskDefinition.ContainerDefinitions
966+
web := findContainerByName("web", containerDefs)
967+
968+
if assert.NoError(t, err) {
969+
// check ecs-params override value is present
970+
assert.Equal(t, webMem, aws.Int64Value(web.Memory), "Expected Memory to match")
971+
// check config values not present in ecs-params are present
972+
assert.Equal(t, containerConfig.CPU, aws.Int64Value(web.Cpu), "Expected CPU to match")
973+
assert.Equal(t, containerConfig.MemoryReservation, aws.Int64Value(web.MemoryReservation), "Expected MemoryReservation to match")
974+
}
975+
}
976+
977+
func TestConvertToTaskDefinitionWithECSParams_MemResGreaterThanMemLimit(t *testing.T) {
978+
containerConfig := &adapter.ContainerConfig{Name: "web"}
979+
ecsParamsString := `version: 1
980+
task_definition:
981+
services:
982+
web:
983+
mem_limit: 10m
984+
mem_reservation: 15m`
985+
986+
content := []byte(ecsParamsString)
987+
988+
tmpfile, err := ioutil.TempFile("", "ecs-params")
989+
assert.NoError(t, err, "Could not create ecs params tempfile")
990+
991+
defer os.Remove(tmpfile.Name())
992+
993+
_, err = tmpfile.Write(content)
994+
assert.NoError(t, err, "Could not write data to ecs params tempfile")
995+
996+
err = tmpfile.Close()
997+
assert.NoError(t, err, "Could not close tempfile")
998+
999+
ecsParamsFileName := tmpfile.Name()
1000+
ecsParams, err := ReadECSParams(ecsParamsFileName)
1001+
assert.NoError(t, err, "Could not read ECS Params file")
1002+
1003+
containerConfigs := []adapter.ContainerConfig{*containerConfig}
1004+
_, err = convertToTaskDefWithEcsParamsInTest(t, containerConfigs, "", ecsParams)
1005+
1006+
assert.Error(t, err, "Expected error if mem_reservation was more than mem_limit")
1007+
}
1008+
8881009
func TestConvertToTaskDefinition_WithTaskSize(t *testing.T) {
8891010
containerConfigs := testContainerConfigs([]string{"mysql", "wordpress"})
8901011
ecsParamsString := `version: 1

0 commit comments

Comments
 (0)