Skip to content

Commit a31a2ca

Browse files
allisaurusallisaurus
committed
[Bugfix] Allow container mem_limit to be null
Previously, if no mem_limt was set on a container, ecs-cli would default it to 512mb, even if task size mem_limit was set. Additionally, if mem_reservation only was set, mem_limit would be set to the same value. This change leaves mem_limit unset (nil) in both cases. Resolves aws#570, aws#606
1 parent 4f19407 commit a31a2ca

File tree

3 files changed

+185
-137
lines changed

3 files changed

+185
-137
lines changed

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

Lines changed: 5 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,6 @@ import (
2424
log "github.com/sirupsen/logrus"
2525
)
2626

27-
const (
28-
defaultMemLimit = 512
29-
)
30-
3127
// TaskDefParams contains basic fields to build an ECS task definition
3228
type TaskDefParams struct {
3329
networkMode string
@@ -85,7 +81,11 @@ func ConvertToTaskDefinition(params ConvertTaskDefParams) (*ecs.TaskDefinition,
8581
return nil, errors.New("Task definition does not have any essential containers")
8682
}
8783

88-
containerDef, err := convertToContainerDef(&containerConfig, ecsContainerDef)
84+
taskVals := taskLevelValues{
85+
MemLimit: taskDefParams.memory,
86+
}
87+
88+
containerDef, err := reconcileContainerDef(&containerConfig, ecsContainerDef, taskVals)
8989
if err != nil {
9090
return nil, err
9191
}
@@ -150,135 +150,6 @@ func ConvertToTaskDefinition(params ConvertTaskDefParams) (*ecs.TaskDefinition,
150150
return taskDefinition, nil
151151
}
152152

153-
// convertToContainerDef transforms each service in docker-compose.yml and
154-
// ecs-params.yml to an equivalent ECS container definition
155-
func convertToContainerDef(inputCfg *adapter.ContainerConfig, ecsContainerDef *ContainerDef) (*ecs.ContainerDefinition, error) {
156-
outputContDef := &ecs.ContainerDefinition{}
157-
158-
// Populate ECS container definition, offloading the validation to aws-sdk
159-
outputContDef.SetCommand(aws.StringSlice(inputCfg.Command))
160-
outputContDef.SetDnsSearchDomains(aws.StringSlice(inputCfg.DNSSearchDomains))
161-
outputContDef.SetDnsServers(aws.StringSlice(inputCfg.DNSServers))
162-
outputContDef.SetDockerLabels(inputCfg.DockerLabels)
163-
outputContDef.SetDockerSecurityOptions(aws.StringSlice(inputCfg.DockerSecurityOptions))
164-
outputContDef.SetEntryPoint(aws.StringSlice(inputCfg.Entrypoint))
165-
outputContDef.SetEnvironment(inputCfg.Environment)
166-
outputContDef.SetExtraHosts(inputCfg.ExtraHosts)
167-
if inputCfg.Hostname != "" {
168-
outputContDef.SetHostname(inputCfg.Hostname)
169-
}
170-
outputContDef.SetImage(inputCfg.Image)
171-
outputContDef.SetLinks(aws.StringSlice(inputCfg.Links)) // TODO, read from external links
172-
outputContDef.SetLogConfiguration(inputCfg.LogConfiguration)
173-
outputContDef.SetMountPoints(inputCfg.MountPoints)
174-
outputContDef.SetName(inputCfg.Name)
175-
outputContDef.SetPrivileged(inputCfg.Privileged)
176-
outputContDef.SetPortMappings(inputCfg.PortMappings)
177-
outputContDef.SetReadonlyRootFilesystem(inputCfg.ReadOnly)
178-
outputContDef.SetUlimits(inputCfg.Ulimits)
179-
180-
if inputCfg.User != "" {
181-
outputContDef.SetUser(inputCfg.User)
182-
}
183-
outputContDef.SetVolumesFrom(inputCfg.VolumesFrom)
184-
if inputCfg.WorkingDirectory != "" {
185-
outputContDef.SetWorkingDirectory(inputCfg.WorkingDirectory)
186-
}
187-
188-
// Set Linux Parameters
189-
outputContDef.LinuxParameters = &ecs.LinuxParameters{Capabilities: &ecs.KernelCapabilities{}}
190-
if inputCfg.CapAdd != nil {
191-
outputContDef.LinuxParameters.Capabilities.SetAdd(aws.StringSlice(inputCfg.CapAdd))
192-
}
193-
if inputCfg.CapDrop != nil {
194-
outputContDef.LinuxParameters.Capabilities.SetDrop(aws.StringSlice(inputCfg.CapDrop))
195-
}
196-
if inputCfg.Devices != nil {
197-
outputContDef.LinuxParameters.SetDevices(inputCfg.Devices)
198-
}
199-
200-
// Only set shmSize if specified. Otherwise we expect this sharedMemorySize for the
201-
// containerDefinition to be null; Docker will by default allocate 64M for shared memory if
202-
// shmSize is null.
203-
if inputCfg.ShmSize != 0 {
204-
outputContDef.LinuxParameters.SetSharedMemorySize(inputCfg.ShmSize)
205-
}
206-
207-
// Only set tmpfs if tmpfs mounts are specified.
208-
if inputCfg.Tmpfs != nil { // will never be nil?
209-
outputContDef.LinuxParameters.SetTmpfs(inputCfg.Tmpfs)
210-
}
211-
212-
// initialize container resources from inputCfg
213-
cpu := inputCfg.CPU
214-
mem := inputCfg.Memory
215-
memRes := inputCfg.MemoryReservation
216-
healthCheck := inputCfg.HealthCheck
217-
218-
// Set essential & resource fields from ecs-params file, if present
219-
if ecsContainerDef != nil {
220-
outputContDef.Essential = aws.Bool(ecsContainerDef.Essential)
221-
222-
// CPU and Memory are expected to be set here if compose v3 was used
223-
cpu = resolveIntResourceOverride(inputCfg.Name, cpu, ecsContainerDef.Cpu, "CPU")
224-
225-
ecsMemInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.Memory))
226-
mem = resolveIntResourceOverride(inputCfg.Name, mem, ecsMemInMB, "MemoryLimit")
227-
228-
ecsMemResInMB := adapter.ConvertToMemoryInMB(int64(ecsContainerDef.MemoryReservation))
229-
230-
memRes = resolveIntResourceOverride(inputCfg.Name, memRes, ecsMemResInMB, "MemoryReservation")
231-
232-
credParam := ecsContainerDef.RepositoryCredentials.CredentialsParameter
233-
234-
if credParam != "" {
235-
outputContDef.RepositoryCredentials = &ecs.RepositoryCredentials{}
236-
outputContDef.RepositoryCredentials.SetCredentialsParameter(credParam)
237-
}
238-
239-
if len(ecsContainerDef.Secrets) > 0 {
240-
outputContDef.SetSecrets(convertToECSSecrets(ecsContainerDef.Secrets))
241-
}
242-
243-
var err error
244-
healthCheck, err = resolveHealthCheck(inputCfg.Name, healthCheck, ecsContainerDef.HealthCheck)
245-
if err != nil {
246-
return nil, err
247-
}
248-
}
249-
250-
// One or both of memory and memoryReservation is required to register a task definition with ECS
251-
// If neither is provided by 1) compose file or 2) ecs-params, set default
252-
// NOTE: Docker does not set a memory limit for containers
253-
if mem == 0 && memRes == 0 {
254-
mem = defaultMemLimit
255-
}
256-
257-
// Docker compose allows specifying memory reservation with memory, so
258-
// we should default to minimum allowable value for memory hard limit
259-
if mem == 0 && memRes != 0 {
260-
mem = memRes
261-
}
262-
263-
if mem < memRes {
264-
return nil, errors.New("mem_limit must be greater than mem_reservation")
265-
}
266-
267-
outputContDef.SetCpu(cpu)
268-
if mem != 0 {
269-
outputContDef.SetMemory(mem)
270-
}
271-
if memRes != 0 {
272-
outputContDef.SetMemoryReservation(memRes)
273-
}
274-
275-
if healthCheck != nil {
276-
outputContDef.SetHealthCheck(healthCheck)
277-
}
278-
279-
return outputContDef, nil
280-
}
281-
282153
func resolveHealthCheck(serviceName string, healthCheck *ecs.HealthCheck, ecsParamsHealthCheck *HealthCheck) (*ecs.HealthCheck, error) {
283154
if ecsParamsHealthCheck != nil {
284155
healthCheckOverride, err := ecsParamsHealthCheck.ConvertToECSHealthCheck()

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ func TestConvertToTaskDefinitionLaunchTypeFargate(t *testing.T) {
358358
assert.Equal(t, "FARGATE", aws.StringValue(taskDefinition.RequiresCompatibilities[0]))
359359
}
360360

361-
// Tests for ConvertToContainerDefinition with ECS Params
361+
// Tests for ConvertToTaskDefinition with ECS Params
362362
func TestConvertToTaskDefinitionWithECSParams_ComposeMemoryLessThanMemoryRes(t *testing.T) {
363363
// set up containerConfig w/o value for Memory
364364
containerConfig := &adapter.ContainerConfig{
@@ -1062,8 +1062,9 @@ task_definition:
10621062
web := findContainerByName("web", containerDefs)
10631063

10641064
if assert.NoError(t, err) {
1065-
assert.Equal(t, webMem, aws.Int64Value(web.Memory), "Expected Memory to match")
10661065
assert.Equal(t, webMem, aws.Int64Value(web.MemoryReservation), "Expected MemoryReservation to match")
1066+
// check mem_limit not set
1067+
assert.Empty(t, web.Memory, "Expected Memory to be nil")
10671068
// check config values not present in ecs-params are present
10681069
assert.Empty(t, web.Cpu, "Expected CPU to be empty")
10691070
}
@@ -1165,7 +1166,7 @@ func TestConvertToTaskDefinition_MemReservationOnlyProvided(t *testing.T) {
11651166
containerDefs := taskDefinition.ContainerDefinitions
11661167
web := findContainerByName("web", containerDefs)
11671168

1168-
assert.Equal(t, webMem, aws.Int64Value(web.Memory), "Expected Memory to match")
1169+
assert.Empty(t, web.Memory, "Expected Memory to be nil")
11691170
assert.Equal(t, webMem, aws.Int64Value(web.MemoryReservation), "Expected MemoryReservation to match")
11701171
assert.Empty(t, web.Cpu, "Expected CPU to be empty")
11711172
}
@@ -1507,6 +1508,29 @@ task_definition:
15071508
}
15081509
}
15091510

1511+
func TestConvertToTaskDefinitionWithECSParams_OnlyTaskMemProvided(t *testing.T) {
1512+
containerConfig := &adapter.ContainerConfig{
1513+
Name: "web",
1514+
}
1515+
ecsParams := &ECSParams{
1516+
TaskDefinition: EcsTaskDef{
1517+
TaskSize: TaskSize{
1518+
Memory: "1gb",
1519+
},
1520+
},
1521+
}
1522+
1523+
containerConfigs := []adapter.ContainerConfig{*containerConfig}
1524+
taskDefinition, err := convertToTaskDefinitionForTest(t, containerConfigs, "", "", ecsParams, nil)
1525+
1526+
containerDefs := taskDefinition.ContainerDefinitions
1527+
web := findContainerByName("web", containerDefs)
1528+
1529+
assert.NoError(t, err)
1530+
assert.Empty(t, web.Memory, "Expected Memory to be nil")
1531+
assert.Equal(t, taskDefinition.Memory, aws.String("1gb"))
1532+
}
1533+
15101534
func TestConvertToTaskDefinitionWithECSParams_HealthCheckOverrideCompose(t *testing.T) {
15111535
containerConfig := &adapter.ContainerConfig{
15121536
Name: "web",

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

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
// Copyright 2015-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License"). You may
4+
// not use this file except in compliance with the License. A copy of the
5+
// License is located at
6+
//
7+
// http://aws.amazon.com/apache2.0/
8+
//
9+
// or in the "license" file accompanying this file. This file is distributed
10+
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
11+
// express or implied. See the License for the specific language governing
12+
// permissions and limitations under the License.
13+
14+
package utils
15+
16+
import (
17+
"errors"
18+
19+
"github.com/aws/amazon-ecs-cli/ecs-cli/modules/cli/compose/adapter"
20+
"github.com/aws/aws-sdk-go/aws"
21+
"github.com/aws/aws-sdk-go/service/ecs"
22+
)
23+
24+
const (
25+
defaultMemLimit = 512
26+
)
27+
28+
type taskLevelValues struct {
29+
MemLimit string
30+
}
31+
32+
// reconcileContainerDef transforms each service in docker-compose.yml and
33+
// ecs-params.yml to an equivalent ECS container definition
34+
func reconcileContainerDef(inputCfg *adapter.ContainerConfig, ecsConDef *ContainerDef, taskVals taskLevelValues) (*ecs.ContainerDefinition, error) {
35+
outputContDef := &ecs.ContainerDefinition{}
36+
37+
// Populate ECS container definition, offloading the validation to aws-sdk
38+
outputContDef.SetCommand(aws.StringSlice(inputCfg.Command))
39+
outputContDef.SetDnsSearchDomains(aws.StringSlice(inputCfg.DNSSearchDomains))
40+
outputContDef.SetDnsServers(aws.StringSlice(inputCfg.DNSServers))
41+
outputContDef.SetDockerLabels(inputCfg.DockerLabels)
42+
outputContDef.SetDockerSecurityOptions(aws.StringSlice(inputCfg.DockerSecurityOptions))
43+
outputContDef.SetEntryPoint(aws.StringSlice(inputCfg.Entrypoint))
44+
outputContDef.SetEnvironment(inputCfg.Environment)
45+
outputContDef.SetExtraHosts(inputCfg.ExtraHosts)
46+
if inputCfg.Hostname != "" {
47+
outputContDef.SetHostname(inputCfg.Hostname)
48+
}
49+
outputContDef.SetImage(inputCfg.Image)
50+
outputContDef.SetLinks(aws.StringSlice(inputCfg.Links)) // TODO, read from external links
51+
outputContDef.SetLogConfiguration(inputCfg.LogConfiguration)
52+
outputContDef.SetMountPoints(inputCfg.MountPoints)
53+
outputContDef.SetName(inputCfg.Name)
54+
outputContDef.SetPrivileged(inputCfg.Privileged)
55+
outputContDef.SetPortMappings(inputCfg.PortMappings)
56+
outputContDef.SetReadonlyRootFilesystem(inputCfg.ReadOnly)
57+
outputContDef.SetUlimits(inputCfg.Ulimits)
58+
59+
if inputCfg.User != "" {
60+
outputContDef.SetUser(inputCfg.User)
61+
}
62+
outputContDef.SetVolumesFrom(inputCfg.VolumesFrom)
63+
if inputCfg.WorkingDirectory != "" {
64+
outputContDef.SetWorkingDirectory(inputCfg.WorkingDirectory)
65+
}
66+
67+
// Set Linux Parameters
68+
outputContDef.LinuxParameters = &ecs.LinuxParameters{Capabilities: &ecs.KernelCapabilities{}}
69+
if inputCfg.CapAdd != nil {
70+
outputContDef.LinuxParameters.Capabilities.SetAdd(aws.StringSlice(inputCfg.CapAdd))
71+
}
72+
if inputCfg.CapDrop != nil {
73+
outputContDef.LinuxParameters.Capabilities.SetDrop(aws.StringSlice(inputCfg.CapDrop))
74+
}
75+
if inputCfg.Devices != nil {
76+
outputContDef.LinuxParameters.SetDevices(inputCfg.Devices)
77+
}
78+
79+
// Only set shmSize if specified. Docker will by default allocate 64M
80+
// for shared memory if shmSize is null.
81+
if inputCfg.ShmSize != 0 {
82+
outputContDef.LinuxParameters.SetSharedMemorySize(inputCfg.ShmSize)
83+
}
84+
85+
// Only set tmpfs if tmpfs mounts are specified.
86+
if inputCfg.Tmpfs != nil { // TODO: will never be nil?
87+
outputContDef.LinuxParameters.SetTmpfs(inputCfg.Tmpfs)
88+
}
89+
90+
// initialize container resources from inputCfg
91+
cpu := inputCfg.CPU
92+
memLimit := inputCfg.Memory
93+
memRes := inputCfg.MemoryReservation
94+
healthCheck := inputCfg.HealthCheck
95+
96+
if ecsConDef != nil {
97+
outputContDef.Essential = aws.Bool(ecsConDef.Essential)
98+
99+
// CPU and Memory are expected to be set here if compose v3 was used
100+
cpu = resolveIntResourceOverride(inputCfg.Name, cpu, ecsConDef.Cpu, "CPU")
101+
102+
ecsMemInMB := adapter.ConvertToMemoryInMB(int64(ecsConDef.Memory))
103+
memLimit = resolveIntResourceOverride(inputCfg.Name, memLimit, ecsMemInMB, "MemoryLimit")
104+
105+
ecsMemResInMB := adapter.ConvertToMemoryInMB(int64(ecsConDef.MemoryReservation))
106+
107+
memRes = resolveIntResourceOverride(inputCfg.Name, memRes, ecsMemResInMB, "MemoryReservation")
108+
109+
credParam := ecsConDef.RepositoryCredentials.CredentialsParameter
110+
111+
if credParam != "" {
112+
outputContDef.RepositoryCredentials = &ecs.RepositoryCredentials{}
113+
outputContDef.RepositoryCredentials.SetCredentialsParameter(credParam)
114+
}
115+
116+
if len(ecsConDef.Secrets) > 0 {
117+
outputContDef.SetSecrets(convertToECSSecrets(ecsConDef.Secrets))
118+
}
119+
120+
var err error
121+
healthCheck, err = resolveHealthCheck(inputCfg.Name, healthCheck, ecsConDef.HealthCheck)
122+
if err != nil {
123+
return nil, err
124+
}
125+
}
126+
127+
// At least one memory value is required to register a task definition.
128+
// If no memory value is set, set default limit
129+
if memLimit == 0 && memRes == 0 && taskVals.MemLimit == "" {
130+
memLimit = defaultMemLimit
131+
}
132+
133+
// if memLimit is set and less than memRes, show error
134+
if memLimit != 0 && memLimit < memRes {
135+
return nil, errors.New("mem_limit must be greater than mem_reservation")
136+
}
137+
138+
if memLimit != 0 {
139+
outputContDef.SetMemory(memLimit)
140+
}
141+
142+
if memRes != 0 {
143+
outputContDef.SetMemoryReservation(memRes)
144+
}
145+
146+
outputContDef.SetCpu(cpu)
147+
148+
if healthCheck != nil {
149+
outputContDef.SetHealthCheck(healthCheck)
150+
}
151+
152+
return outputContDef, nil
153+
}

0 commit comments

Comments
 (0)