Skip to content

Commit 77d2a10

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

14 files changed

+369
-117
lines changed

‎CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Changelog
22

3+
## 1.17.1-dev
4+
* Bug - Workaround for an issue where CPU percent was set to 1 when CPU was not
5+
set or set to zero(unbounded) in Windows [#1227](https://github.com/aws/amazon-ecs-agent/pull/1227)
6+
37
## 1.17.0
48
* Feature - Support a HTTP endpoint for `awsvpc` tasks to query metadata
59
* Feature - Support Docker health check

‎README.md

+1
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ additional details on each available environment variable.
177177
| `ECS_HOST_DATA_DIR` | `/var/lib/ecs` | The source directory on the host from which ECS_DATADIR is mounted. We use this to determine the source mount path for container metadata files in the case the ECS Agent is running as a container. We do not use this value in Windows because the ECS Agent is not running as container in Windows. | `/var/lib/ecs` | `Not used` |
178178
| `ECS_ENABLE_TASK_CPU_MEM_LIMIT` | `true` | Whether to enable task-level cpu and memory limits | `true` | `false` |
179179
| `ECS_CGROUP_PATH` | `/sys/fs/cgroup` | The root cgroup path that is expected by the ECS agent. This is the path that accessible from the agent mount. | `/sys/fs/cgroup` | Not applicable |
180+
| `ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |
180181

181182
### Persistence
182183

‎agent/api/task.go

+3-14
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,9 @@ type Task struct {
143143
// MemoryCPULimitsEnabled to determine if task supports CPU, memory limits
144144
MemoryCPULimitsEnabled bool `json:"MemoryCPULimitsEnabled,omitempty"`
145145

146+
// platformFields consists of fields specific to linux/windows for a task
147+
platformFields platformFields
148+
146149
// lock is for protecting all fields in the task struct
147150
lock sync.RWMutex
148151
}
@@ -511,20 +514,6 @@ func (task *Task) SetConfigHostconfigBasedOnVersion(container *Container, config
511514
return nil
512515
}
513516

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-
528517
func (task *Task) dockerExposedPorts(container *Container) map[docker.Port]struct{} {
529518
dockerExposedPorts := make(map[docker.Port]struct{})
530519

‎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

+18
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"
@@ -40,6 +41,9 @@ const (
4041
bytesPerMegabyte = 1024 * 1024
4142
)
4243

44+
// platformFields consists of fields specific to Linux for a task
45+
type platformFields struct {}
46+
4347
func (task *Task) adjustForPlatform(cfg *config.Config) {
4448
task.lock.Lock()
4549
defer task.lock.Unlock()
@@ -178,3 +182,17 @@ func (task *Task) overrideCgroupParent(hostConfig *docker.HostConfig) error {
178182
}
179183
return nil
180184
}
185+
186+
// dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
187+
// Docker silently converts 0 to 1024 CPU shares, which is probably not what we
188+
// want. Instead, we convert 0 to 2 to be closer to expected behavior. The
189+
// reason for 2 over 1 is that 1 is an invalid value (Linux's choice, not Docker's).
190+
func (task *Task) dockerCPUShares(containerCPU uint) int64 {
191+
if containerCPU <= 1 {
192+
seelog.Debugf(
193+
"Converting CPU shares to allowed minimum of 2 for task arn: [%s] and cpu shares: %d",
194+
task.Arn, containerCPU)
195+
return 2
196+
}
197+
return int64(containerCPU)
198+
}

‎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

+31-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

@@ -33,11 +34,22 @@ const (
3334
minimumCPUPercent = 1
3435
)
3536

37+
// platformFields consists of fields specific to Windows for a task
38+
type platformFields struct {
39+
// cpuUnbounded determines whether a mix of unbounded and bounded CPU tasks
40+
// are allowed to run in the instance
41+
cpuUnbounded bool
42+
}
43+
3644
var cpuShareScaleFactor = runtime.NumCPU() * cpuSharesPerCore
3745

3846
// adjustForPlatform makes Windows-specific changes to the task after unmarshal
3947
func (task *Task) adjustForPlatform(cfg *config.Config) {
4048
task.downcaseAllVolumePaths()
49+
platformFields := platformFields{
50+
cpuUnbounded: cfg.PlatformVariables.CPUUnbounded,
51+
}
52+
task.platformFields = platformFields
4153
}
4254

4355
// downcaseAllVolumePaths forces all volume paths (host path and container path)
@@ -68,8 +80,11 @@ func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) erro
6880
task.overrideDefaultMemorySwappiness(hostConfig)
6981
// Convert the CPUShares to CPUPercent
7082
hostConfig.CPUPercent = hostConfig.CPUShares * percentageFactor / int64(cpuShareScaleFactor)
71-
if hostConfig.CPUPercent == 0 {
72-
// if the cpu percent is too low, we set it to the minimum
83+
if hostConfig.CPUPercent == 0 && hostConfig.CPUShares != 0 {
84+
// if CPU percent is too low, we set it to the minimum(linux and some windows tasks).
85+
// if the CPU is explicitly set to zero or not set at all, and CPU unbounded
86+
// tasks are allowed for windows, let CPU percent be zero.
87+
// this is a workaround to allow CPU unbounded tasks(https://github.com/aws/amazon-ecs-agent/issues/1127)
7388
hostConfig.CPUPercent = minimumCPUPercent
7489
}
7590
hostConfig.CPUShares = 0
@@ -85,3 +100,17 @@ func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) erro
85100
func (task *Task) overrideDefaultMemorySwappiness(hostConfig *docker.HostConfig) {
86101
hostConfig.MemorySwappiness = memorySwappinessDefault
87102
}
103+
104+
// dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
105+
// Docker silently converts 0 to 1024 CPU shares, which is probably not what we
106+
// want. Instead, we convert 0 to 2 to be closer to expected behavior. The
107+
// reason for 2 over 1 is that 1 is an invalid value (Linux's choice, not Docker's).
108+
func (task *Task) dockerCPUShares(containerCPU uint) int64 {
109+
if containerCPU <= 1 && !task.platformFields.cpuUnbounded {
110+
seelog.Debugf(
111+
"Converting CPU shares to allowed minimum of 2 for task arn: [%s] and cpu shares: %d",
112+
task.Arn, containerCPU)
113+
return 2
114+
}
115+
return int64(containerCPU)
116+
}

0 commit comments

Comments
 (0)