Skip to content

Commit ded74dc

Browse files
sharanyadsharanyad
committed
api, config: Allow mix of CPU bounded and unbounded tasks for windows
This is a temporary fix for the issue #1127
1 parent 8a6cd4c commit ded74dc

11 files changed

+508
-118
lines changed

‎agent/api/task.go

+2-14
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,8 @@ type Task struct {
145145

146146
// lock is for protecting all fields in the task struct
147147
lock sync.RWMutex
148+
149+
cpuUnboundedWindowsEnabled bool
148150
}
149151

150152
// PostUnmarshalTask is run after a task has been unmarshalled, but before it has been
@@ -511,20 +513,6 @@ func (task *Task) SetConfigHostconfigBasedOnVersion(container *Container, config
511513
return nil
512514
}
513515

514-
// dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
515-
// Docker silently converts 0 to 1024 CPU shares, which is probably not what we
516-
// want. Instead, we convert 0 to 2 to be closer to expected behavior. The
517-
// reason for 2 over 1 is that 1 is an invalid value (Linux's choice, not Docker's).
518-
func (task *Task) dockerCPUShares(containerCPU uint) int64 {
519-
if containerCPU <= 1 {
520-
seelog.Debugf(
521-
"Converting CPU shares to allowed minimum of 2 for task arn: [%s] and cpu shares: %d",
522-
task.Arn, containerCPU)
523-
return 2
524-
}
525-
return int64(containerCPU)
526-
}
527-
528516
func (task *Task) dockerExposedPorts(container *Container) map[docker.Port]struct{} {
529517
dockerExposedPorts := make(map[docker.Port]struct{})
530518

‎agent/api/task_test.go

+1-98
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
1+
// Copyright 2014-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License"). You may
44
// not use this file except in compliance with the License. A copy of the
@@ -227,57 +227,6 @@ func TestDockerHostConfigRawConfig(t *testing.T) {
227227
assertSetStructFieldsEqual(t, expectedOutput, *config)
228228
}
229229

230-
func TestDockerHostConfigRawConfigMerging(t *testing.T) {
231-
// Use a struct that will marshal to the actual message we expect; not
232-
// docker.HostConfig which will include a lot of zero values.
233-
rawHostConfigInput := struct {
234-
Privileged bool `json:"Privileged,omitempty" yaml:"Privileged,omitempty"`
235-
SecurityOpt []string `json:"SecurityOpt,omitempty" yaml:"SecurityOpt,omitempty"`
236-
}{
237-
Privileged: true,
238-
SecurityOpt: []string{"foo", "bar"},
239-
}
240-
241-
rawHostConfig, err := json.Marshal(&rawHostConfigInput)
242-
if err != nil {
243-
t.Fatal(err)
244-
}
245-
246-
testTask := &Task{
247-
Arn: "arn:aws:ecs:us-east-1:012345678910:task/c09f0188-7f87-4b0f-bfc3-16296622b6fe",
248-
Family: "myFamily",
249-
Version: "1",
250-
Containers: []*Container{
251-
{
252-
Name: "c1",
253-
Image: "image",
254-
CPU: 50,
255-
Memory: 100,
256-
VolumesFrom: []VolumeFrom{{SourceContainer: "c2"}},
257-
DockerConfig: DockerConfig{
258-
HostConfig: strptr(string(rawHostConfig)),
259-
},
260-
},
261-
{
262-
Name: "c2",
263-
},
264-
},
265-
}
266-
267-
hostConfig, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
268-
assert.Nil(t, configErr)
269-
270-
expected := docker.HostConfig{
271-
Privileged: true,
272-
SecurityOpt: []string{"foo", "bar"},
273-
VolumesFrom: []string{"dockername-c2"},
274-
MemorySwappiness: memorySwappinessDefault,
275-
CPUPercent: minimumCPUPercent,
276-
}
277-
278-
assertSetStructFieldsEqual(t, expected, *hostConfig)
279-
}
280-
281230
func TestDockerHostConfigPauseContainer(t *testing.T) {
282231
testTask := &Task{
283232
ENI: &ENI{
@@ -1282,52 +1231,6 @@ func TestApplyExecutionRoleLogsAuthFailNoCredentialsForTask(t *testing.T) {
12821231
assert.Error(t, err)
12831232
}
12841233

1285-
// TestSetConfigHostconfigBasedOnAPIVersion tests the docker hostconfig was correctly set// based on the docker client version
1286-
func TestSetConfigHostconfigBasedOnAPIVersion(t *testing.T) {
1287-
memoryMiB := 500
1288-
testTask := &Task{
1289-
Containers: []*Container{
1290-
{
1291-
Name: "c1",
1292-
CPU: uint(10),
1293-
Memory: uint(memoryMiB),
1294-
},
1295-
},
1296-
}
1297-
1298-
hostconfig, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
1299-
assert.Nil(t, err)
1300-
1301-
config, cerr := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
1302-
assert.Nil(t, cerr)
1303-
1304-
assert.Equal(t, int64(memoryMiB*1024*1024), config.Memory)
1305-
if runtime.GOOS == "windows" {
1306-
assert.Equal(t, int64(minimumCPUPercent), hostconfig.CPUPercent)
1307-
} else {
1308-
assert.Equal(t, int64(10), config.CPUShares)
1309-
}
1310-
assert.Empty(t, hostconfig.CPUShares)
1311-
assert.Empty(t, hostconfig.Memory)
1312-
1313-
hostconfig, err = testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), dockerclient.Version_1_18)
1314-
assert.Nil(t, err)
1315-
1316-
config, cerr = testTask.DockerConfig(testTask.Containers[0], dockerclient.Version_1_18)
1317-
assert.Nil(t, err)
1318-
assert.Equal(t, int64(memoryMiB*1024*1024), hostconfig.Memory)
1319-
if runtime.GOOS == "windows" {
1320-
// cpushares is set to zero on windows
1321-
assert.Empty(t, hostconfig.CPUShares)
1322-
assert.Equal(t, int64(minimumCPUPercent), hostconfig.CPUPercent)
1323-
} else {
1324-
assert.Equal(t, int64(10), hostconfig.CPUShares)
1325-
}
1326-
1327-
assert.Empty(t, config.CPUShares)
1328-
assert.Empty(t, config.Memory)
1329-
}
1330-
13311234
// TestSetMinimumMemoryLimit ensures that we set the correct minimum memory limit when the limit is too low
13321235
func TestSetMinimumMemoryLimit(t *testing.T) {
13331236
testTask := &Task{

‎agent/api/task_unix.go

+15
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"time"
2121

2222
"github.com/aws/amazon-ecs-agent/agent/config"
23+
"github.com/cihub/seelog"
2324
docker "github.com/fsouza/go-dockerclient"
2425
specs "github.com/opencontainers/runtime-spec/specs-go"
2526
"github.com/pkg/errors"
@@ -178,3 +179,17 @@ func (task *Task) overrideCgroupParent(hostConfig *docker.HostConfig) error {
178179
}
179180
return nil
180181
}
182+
183+
// dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
184+
// Docker silently converts 0 to 1024 CPU shares, which is probably not what we
185+
// want. Instead, we convert 0 to 2 to be closer to expected behavior. The
186+
// reason for 2 over 1 is that 1 is an invalid value (Linux's choice, not Docker's).
187+
func (task *Task) dockerCPUShares(containerCPU uint) int64 {
188+
if containerCPU <= 1 {
189+
seelog.Debugf(
190+
"Converting CPU shares to allowed minimum of 2 for task arn: [%s] and cpu shares: %d",
191+
task.Arn, containerCPU)
192+
return 2
193+
}
194+
return int64(containerCPU)
195+
}

‎agent/api/task_unix_test.go

+92-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// +build !windows
22

3-
// Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
// Copyright 2014-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
44
//
55
// Licensed under the Apache License, Version 2.0 (the "License"). You may
66
// not use this file except in compliance with the License. A copy of the
@@ -16,10 +16,12 @@
1616
package api
1717

1818
import (
19+
"encoding/json"
1920
"testing"
2021
"time"
2122

2223
"github.com/aws/amazon-ecs-agent/agent/config"
24+
"github.com/aws/amazon-ecs-agent/agent/engine/dockerclient"
2325

2426
docker "github.com/fsouza/go-dockerclient"
2527
specs "github.com/opencontainers/runtime-spec/specs-go"
@@ -48,6 +50,7 @@ const (
4850

4951
taskVCPULimit = 2.0
5052
taskMemoryLimit = 512
53+
minDockerClientAPIVersion = dockerclient.Version_1_17
5154
)
5255

5356
func TestAddNetworkResourceProvisioningDependencyNop(t *testing.T) {
@@ -297,3 +300,91 @@ func TestPlatformHostConfigOverrideErrorPath(t *testing.T) {
297300
assert.Error(t, err)
298301
assert.Empty(t, dockerHostConfig)
299302
}
303+
304+
func TestDockerHostConfigRawConfigMerging(t *testing.T) {
305+
// Use a struct that will marshal to the actual message we expect; not
306+
// docker.HostConfig which will include a lot of zero values.
307+
rawHostConfigInput := struct {
308+
Privileged bool `json:"Privileged,omitempty" yaml:"Privileged,omitempty"`
309+
SecurityOpt []string `json:"SecurityOpt,omitempty" yaml:"SecurityOpt,omitempty"`
310+
}{
311+
Privileged: true,
312+
SecurityOpt: []string{"foo", "bar"},
313+
}
314+
315+
rawHostConfig, err := json.Marshal(&rawHostConfigInput)
316+
if err != nil {
317+
t.Fatal(err)
318+
}
319+
320+
testTask := &Task{
321+
Arn: "arn:aws:ecs:us-east-1:012345678910:task/c09f0188-7f87-4b0f-bfc3-16296622b6fe",
322+
Family: "myFamily",
323+
Version: "1",
324+
Containers: []*Container{
325+
{
326+
Name: "c1",
327+
Image: "image",
328+
CPU: 50,
329+
Memory: 100,
330+
VolumesFrom: []VolumeFrom{{SourceContainer: "c2"}},
331+
DockerConfig: DockerConfig{
332+
HostConfig: strptr(string(rawHostConfig)),
333+
},
334+
},
335+
{
336+
Name: "c2",
337+
},
338+
},
339+
}
340+
341+
hostConfig, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), minDockerClientAPIVersion)
342+
assert.Nil(t, configErr)
343+
344+
expected := docker.HostConfig{
345+
Privileged: true,
346+
SecurityOpt: []string{"foo", "bar"},
347+
VolumesFrom: []string{"dockername-c2"},
348+
MemorySwappiness: memorySwappinessDefault,
349+
CPUPercent: minimumCPUPercent,
350+
}
351+
352+
assertSetStructFieldsEqual(t, expected, *hostConfig)
353+
}
354+
355+
// TestSetConfigHostconfigBasedOnAPIVersion tests the docker hostconfig was correctly
356+
// set based on the docker client version
357+
func TestSetConfigHostconfigBasedOnAPIVersion(t *testing.T) {
358+
memoryMiB := 500
359+
testTask := &Task{
360+
Containers: []*Container{
361+
{
362+
Name: "c1",
363+
CPU: uint(10),
364+
Memory: uint(memoryMiB),
365+
},
366+
},
367+
}
368+
369+
hostconfig, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), minDockerClientAPIVersion)
370+
assert.Nil(t, err)
371+
372+
config, cerr := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
373+
assert.Nil(t, cerr)
374+
375+
assert.Equal(t, int64(memoryMiB*1024*1024), config.Memory)
376+
assert.Equal(t, int64(10), config.CPUShares)
377+
assert.Empty(t, hostconfig.CPUShares)
378+
assert.Empty(t, hostconfig.Memory)
379+
380+
hostconfig, err = testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), dockerclient.Version_1_18)
381+
assert.Nil(t, err)
382+
383+
config, cerr = testTask.DockerConfig(testTask.Containers[0], dockerclient.Version_1_18)
384+
assert.Nil(t, err)
385+
assert.Equal(t, int64(memoryMiB*1024*1024), hostconfig.Memory)
386+
assert.Equal(t, int64(10), hostconfig.CPUShares)
387+
388+
assert.Empty(t, config.CPUShares)
389+
assert.Empty(t, config.Memory)
390+
}

‎agent/api/task_windows.go

+23-2
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"strings"
2222

2323
"github.com/aws/amazon-ecs-agent/agent/config"
24+
"github.com/cihub/seelog"
2425
docker "github.com/fsouza/go-dockerclient"
2526
)
2627

@@ -38,6 +39,7 @@ var cpuShareScaleFactor = runtime.NumCPU() * cpuSharesPerCore
3839
// adjustForPlatform makes Windows-specific changes to the task after unmarshal
3940
func (task *Task) adjustForPlatform(cfg *config.Config) {
4041
task.downcaseAllVolumePaths()
42+
task.cpuUnboundedWindowsEnabled = cfg.CPUUnboundedWindowsEnabled
4143
}
4244

4345
// downcaseAllVolumePaths forces all volume paths (host path and container path)
@@ -69,8 +71,13 @@ func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) erro
6971
// Convert the CPUShares to CPUPercent
7072
hostConfig.CPUPercent = hostConfig.CPUShares * percentageFactor / int64(cpuShareScaleFactor)
7173
if hostConfig.CPUPercent == 0 {
72-
// if the cpu percent is too low, we set it to the minimum
73-
hostConfig.CPUPercent = minimumCPUPercent
74+
// if CPU percent is too low, we set it to the minimum(linux and some windows tasks).
75+
// if the CPU is explicitly set to zero or not set at all, and CPU unbounded
76+
// tasks are allowed for windows, let CPU percent be zero.
77+
// this is a workaround to allow CPU unbounded tasks(https://github.com/aws/amazon-ecs-agent/issues/1127)
78+
if hostConfig.CPUShares != 0 {
79+
hostConfig.CPUPercent = minimumCPUPercent
80+
}
7481
}
7582
hostConfig.CPUShares = 0
7683
return nil
@@ -85,3 +92,17 @@ func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) erro
8592
func (task *Task) overrideDefaultMemorySwappiness(hostConfig *docker.HostConfig) {
8693
hostConfig.MemorySwappiness = memorySwappinessDefault
8794
}
95+
96+
// dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
97+
// Docker silently converts 0 to 1024 CPU shares, which is probably not what we
98+
// want. Instead, we convert 0 to 2 to be closer to expected behavior. The
99+
// reason for 2 over 1 is that 1 is an invalid value (Linux's choice, not Docker's).
100+
func (task *Task) dockerCPUShares(containerCPU uint) int64 {
101+
if containerCPU <= 1 && !task.cpuUnboundedWindowsEnabled {
102+
seelog.Debugf(
103+
"Converting CPU shares to allowed minimum of 2 for task arn: [%s] and cpu shares: %d",
104+
task.Arn, containerCPU)
105+
return 2
106+
}
107+
return int64(containerCPU)
108+
}

0 commit comments

Comments
 (0)