Temporary fix for #1127 #1227

sharanyad · 2018-02-07T00:40:03Z

Summary

Allow a mix of CPU unbounded and bounded tasks for windows

Implementation details

Introduce an agent config variable to allow this behavior
if CPU is not set or set to zero, CPU percent and shares of the container will also be zero, else set to 1 if CPU percent calculates to zero

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes: yes

Description for the changelog

Licensing

This contribution is under the terms of the Apache 2.0 License: yes

sharanyad · 2018-02-07T00:41:01Z

this addresses #1127

samuelkarp · 2018-02-07T00:44:54Z

agent/config/config.go

@@ -346,6 +346,7 @@ func environmentConfig() (Config, error) {
 	}
 	containerMetadataEnabled := utils.ParseBool(os.Getenv("ECS_ENABLE_CONTAINER_METADATA"), false)
 	dataDirOnHost := os.Getenv("ECS_HOST_DATA_DIR")
+	cpuUnboundedTasksEnabled := utils.ParseBool(os.Getenv("ECS_ENABLE_CPU_UNBOUNDED_TASKS"), false)


Can we move this into the Windows-only codepath since this doesn't apply to Linux?

Can we name this in a way that is Windows-specific? ECS_ENABLE_CPU_UNBOUNDED_WINDOWS

Can we name this in a way to set expectations that we might remove it in the future? ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND

samuelkarp · 2018-02-07T00:45:28Z

agent/config/types.go

+
+	// CpuUnboundedTasksEnabled specifies if agent can run a mix of CPU bounded and
+	// unbounded tasks for windows
+	CpuUnboundedTasksEnabled bool


Can we move this to a Windows-only type? Can we put Windows in the name?

Also: s/Cpu/CPU/

aaithal

changes look good. i have some minor nits and comments. You're missing README.md CHANGELOG entries for this.

aaithal · 2018-02-14T16:36:46Z

agent/api/task.go

@@ -145,6 +145,8 @@ type Task struct {

 	// lock is for protecting all fields in the task struct
 	lock sync.RWMutex
+
+	cpuUnboundedWindowsEnabled bool


cpuUnboundedWindowsEnabled is quite a long name. Can you make it shorter? cpuUnboundedWindows perhaps? Enabled seems redundant since its already a bool.

Also, the right thing to do here is to probably create Task struct in task_windows.go and make this a part of that struct. But, we can do that in a separate PR.

aaithal · 2018-02-14T16:38:52Z

agent/api/task_windows.go

@@ -69,8 +71,13 @@ func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) erro
 	// Convert the CPUShares to CPUPercent
 	hostConfig.CPUPercent = hostConfig.CPUShares * percentageFactor / int64(cpuShareScaleFactor)
 	if hostConfig.CPUPercent == 0 {


Can you refactor this as if hostConfig.CPUPercent == 0 && hostConfig.CPUShares != 0 ?

aaithal · 2018-02-14T16:41:20Z

agent/config/config_windows.go

@@ -94,6 +95,8 @@ func (cfg *Config) platformOverrides() {

 	// ensure TaskResourceLimit is disabled
 	cfg.TaskCPUMemLimit = ExplicitlyDisabled
+
+	cfg.CPUUnboundedWindowsEnabled = utils.ParseBool(os.Getenv("ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND"), false)


do we really need the string WORKAROUND in the name here? Seems unnecessary.

suggestion by @samuelkarp to name it this way was to convey this could be a temporary fix/hack, until cpu shares are fixed in windows. Moreover allowing both cpu unbounded and bounded tasks in an instance can have serious consequences like bounded tasks not getting their allotted shares and 'workaround' may warn the customers to use it with caution. wdyt?

Just having a flag named ECS_ENABLE_CPU_UNBOUNDED_WINDOWS would probably indicate that this is a workaround imo. I don't have strong opinions here. The env var name does seem too long, however.

I'd like the env var name to convey that we might remove it as a configuration option at some point (i.e., when we fix the CPU bursting behavior). I think WORKAROUND or HACK is helpful in pointing out that this is not an intended long-term approach.

aaithal · 2018-02-14T19:28:29Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## Unreleased


We're going with the convention of next-version.dev instead of the Unreleased tag. So, this should probably be 1.17.1-dev

aaithal · 2018-02-14T19:29:55Z

README.md

@@ -177,6 +177,7 @@ additional details on each available environment variable.
 | `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` |
 | `ECS_ENABLE_TASK_CPU_MEM_LIMIT` | `true` | Whether to enable task-level cpu and memory limits | `true` | `false` |
 | `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 |
+| `ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_HACK` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |


let's not have the word HACK anywhere in these flags. Feel free to switch back to WORKAROUND, but I'd highly discourage using HACK here.

aaithal · 2018-02-14T19:36:21Z

agent/api/task_unix.go

@@ -40,6 +42,89 @@ const (
 	bytesPerMegabyte  = 1024 * 1024
 )

+// Task is the internal representation of a task in the ECS agent
+type Task struct {


All of this code duplication is leading me to think that may be this is not the best way to do this. Because, adding a new field to the task now requires modifications in 2 different files. Also, 2 * code = 2 * bugs. How about something like this:

// task.go, no build flags type Task struct { ... // all common fields additionalFields } // task_unix.go, with the negative windows platform flag type additionalFields struct { // empty for now } // task_windows.go, with the windows build flag type additionalFields struct { cpuUnboundedEnabled bool }

If that sounds reasonable, we should rewrite config.go in a similar vein as well. I'm sure you can come with a better name for additionalFields :)

I think we should try to use the same struct. The Task API from the end user perspective is going to be the same, and forking it to have different implementations internally may become confusing down the road for us. What would you think about simply ignoring unrelated / unimplemented fields on each platform? This is the pattern used in the Docker API, and it works pretty well.

aaithal · 2018-02-14T19:36:47Z

agent/api/task_windows.go

+
+	// cpuUnboundedEnabled determines whether a mix of unbounded and bounded CPU tasks
+	// are allowed to run in the instance
+	cpuUnboundedEnabled bool


cpuUnbounded is good enough i think.

aaithal · 2018-02-16T18:00:51Z

agent/config/types_windows.go

+
+// PlatformVariables consists of configuration variables specific to Windows
+type PlatformVariables struct {
+		// CPUUnbounded specifies if agent can run a mix of CPU bounded and


nit: tabbing seems off here.

adnxn

changes lgtm

This is a temporary workaround for the issue aws#1127

sharanyad added this to the 1.17.1 milestone Feb 7, 2018

samuelkarp reviewed Feb 7, 2018

View reviewed changes

sharanyad force-pushed the cpu-unbounded-windows branch from ac5c1d5 to cb35812 Compare February 12, 2018 20:11

sharanyad changed the title ~~[WIP] Fix #1127~~ [WIP] Temporary Fix #1127 Feb 12, 2018

sharanyad changed the title ~~[WIP] Temporary Fix #1127~~ [WIP] Temporary Fix for #1127 Feb 12, 2018

sharanyad changed the title ~~[WIP] Temporary Fix for #1127~~ Temporary fix for #1127 Feb 12, 2018

sharanyad force-pushed the cpu-unbounded-windows branch 2 times, most recently from 39d5e00 to ded74dc Compare February 12, 2018 22:16

sharanyad requested review from adnxn, samuelkarp and aaithal February 12, 2018 22:52

aaithal reviewed Feb 14, 2018

View reviewed changes

sharanyad force-pushed the cpu-unbounded-windows branch from bc323cb to 479d19a Compare February 14, 2018 19:31

aaithal reviewed Feb 14, 2018

View reviewed changes

sharanyad force-pushed the cpu-unbounded-windows branch from 479d19a to 77d2a10 Compare February 15, 2018 18:33

aaithal approved these changes Feb 16, 2018

View reviewed changes

adnxn approved these changes Feb 19, 2018

View reviewed changes

api, config: Allow mix of CPU bounded and unbounded tasks for windows

b3021b3

This is a temporary workaround for the issue aws#1127

sharanyad force-pushed the cpu-unbounded-windows branch from 77d2a10 to b3021b3 Compare February 19, 2018 19:23

sharanyad merged commit a62bf3d into aws:dev Feb 19, 2018

fierlion mentioned this pull request Sep 11, 2020

CPU Percent for containers set to 1 on Windows when CPU not defined in task #1127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporary fix for #1127 #1227

Temporary fix for #1127 #1227

sharanyad commented Feb 7, 2018 •

edited

Loading

sharanyad commented Feb 7, 2018

samuelkarp Feb 7, 2018

samuelkarp Feb 7, 2018

aaithal left a comment

aaithal Feb 14, 2018

aaithal Feb 14, 2018

aaithal Feb 14, 2018

sharanyad Feb 14, 2018

aaithal Feb 14, 2018

samuelkarp Feb 14, 2018

aaithal Feb 14, 2018

aaithal Feb 14, 2018 •

edited

Loading

aaithal Feb 14, 2018

petderek Feb 14, 2018

aaithal Feb 14, 2018

aaithal Feb 16, 2018

adnxn left a comment

Temporary fix for #1127 #1227

Temporary fix for #1127 #1227

Conversation

sharanyad commented Feb 7, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

sharanyad commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn left a comment

Choose a reason for hiding this comment

sharanyad commented Feb 7, 2018 •

edited

Loading

aaithal Feb 14, 2018 •

edited

Loading