Windows MemorySwappiness Work Around #924

cabbruzzese · 2017-08-11T18:26:44Z

Summary

Adds workaround to set MemorySwappiness to -1 for Windows

Implementation details

HostConfig is retrieved from the task and deep copied to a new struct. Platform based values are then overridden for this copy of the HostConfig struct.

Testing

New tests added:
task_windows_test.go - TestWindowsPlatformHostConfigOverride
task_windows_test.go - TestWindowsMemorySwappinessOption

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

Bug - Workaround for MemorySwapiness bug for Windows

Licensing

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

samuelkarp

Looks pretty good, just a small suggestion.

HostConfig is retrieved from the task and deep copied to a new struct.

I don't see this happening anywhere and don't think it's necessary anyway...

samuelkarp · 2017-08-11T18:42:06Z

agent/api/task_windows.go

+// This bug is not noticed when no value is passed in. However, the go-dockerclient client version
+// we are using removed the json option omitempty causing this parameter to default to 0 if empty.
+// https://github.com/fsouza/go-dockerclient/commit/72342f96fabfa614a94b6ca57d987eccb8a836bf
+func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) {


nit: I'd probably nest this just a little bit deeper so that the comment you have can be on the part that actually talks about what it does. Something like this:

// platformHostConfigOverrides applies platform-specific overrides to HostConfig options. func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) { task.forceDefaultMemorySwappiness(hostConfig) } // forceDefaultMemorySwappiness sets the MemorySwappiness field to be -1 // Docker for Windows treats some options differently than Linux [...] func (task *Task) forceDefaultMemorySwappiness(hostConfig *docker.HostConfig) { hostConfig.MemorySwappiness = memorySwappinessDefault }

petderek

Looks good. had a few comments.

petderek · 2017-08-11T19:07:49Z

agent/api/task_windows.go

+// we are using removed the json option omitempty causing this parameter to default to 0 if empty.
+// https://github.com/fsouza/go-dockerclient/commit/72342f96fabfa614a94b6ca57d987eccb8a836bf
+func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) {
+	hostConfig.MemorySwappiness = memorySwappinessDefault


I don't like this pattern of passing a reference to data and then modifying it, particularly when we can avoid it.

Would you recommend returning a new copy that has been modified, or moving the method to be attached to hostConfig (i.e. hostConfig.platformOverride())

We talked offline -- this is fine. It'd be nice to refactor this to have a translator class for task api -> host config... but thats outside the scope here.

petderek · 2017-08-11T19:08:41Z

agent/api/task_windows_test.go

@@ -99,3 +103,60 @@ func TestPostUnmarshalWindowsCanonicalPaths(t *testing.T) {
 	assert.Equal(t, expectedTask.Containers, task.Containers, "Containers should be equal")
 	assert.Equal(t, expectedTask.Volumes, task.Volumes, "Volumes should be equal")
 }
+
+func TestWindowsPlatformHostConfigOverride(t *testing.T) {


You could use subtests here.

actually, i lied. don't do that here :)

vsiddharth

Changes look good to me.

Could you please amend the commit messages with the following structure:

pkg: Summary Line

Commit Body

vsiddharth · 2017-08-11T18:38:33Z

agent/api/task_windows_test.go

+	}
+
+	hostConfig := &docker.HostConfig{
+	}


nit: can be in a single line

vsiddharth · 2017-08-11T18:39:30Z

agent/api/task_windows_test.go

+	// Expects MemorySwappiness option to be set to -1
+
+	task := &Task{
+		Arn:                 "myArn",


Can't we just use a minimalist Task object here ?

vsiddharth · 2017-08-11T18:40:09Z

agent/api/task_windows_test.go

+func TestWindowsMemorySwappinessOption(t *testing.T) {
+	// Testing sending a task to windows overriding MemorySwappiness value
+	rawHostConfigInput := docker.HostConfig{
+	}


nit: can be in a single line

vsiddharth · 2017-08-11T18:42:56Z

agent/api/task_unix.go

 const (
 	portBindingHostIP = "0.0.0.0"
+	memorySwappinessDefault = 0


Isn't this unused ?
Can we add a comment here ?

It's required to be defined for a test in task_test.go. Adding a comment

samuelkarp · 2017-08-16T22:14:47Z

Abstracted platformHostConfigOverride in task_windows.go to another
method to clarify the intention of the override. Updated related
comments regarding injection point for HostConfig override values.

Commented on task_unix.go to explain the use of memorySwappinessDefault
constant.

Cleaned up extra lines in task_windows_test.go. Removed unneeded
properties in task object use in TestWindowsPlatformHostConfigOverride.

Your current commit message describes what the changes are, but not why. I'd prefer it was the other way around. 😄

samuelkarp

LGTM, though I'd prefer if you adjusted the commit message before merging.

vsiddharth

LGTM

Adds functionality to override HostConfig values in order to pass proper default parameter values, based on platform, to Docker's API. This addresses the MemorySwappiness value which is being set to 0 by default in go-dockerclient.

Clarified comment for platformHostConfigOverride for easier reading. Fixed file endings.

Moved MemorySwappiness override to its own function and make an injection point for future overrides. Added comments to clarify use of memorySwappinessDefault default in task_unix.go Removed properties in the TestWindowsPlatformHostConfigOverride setup tha were unneeded in order to test the functionality.

…n-swappiness

cabbruzzese requested review from samuelkarp and vsiddharth August 11, 2017 18:26

cabbruzzese changed the title ~~Win swappiness~~ Windows MemorySwappiness Work Around Aug 11, 2017

samuelkarp reviewed Aug 11, 2017

View reviewed changes

petderek reviewed Aug 11, 2017

View reviewed changes

vsiddharth reviewed Aug 11, 2017

View reviewed changes

petderek approved these changes Aug 15, 2017

View reviewed changes

samuelkarp approved these changes Aug 16, 2017

View reviewed changes

vsiddharth approved these changes Aug 17, 2017

View reviewed changes

cabbruzzese added 4 commits August 25, 2017 00:05

api: Added Platform Overrides for Task API

88a2e10

Adds functionality to override HostConfig values in order to pass proper default parameter values, based on platform, to Docker's API. This addresses the MemorySwappiness value which is being set to 0 by default in go-dockerclient.

api: Comment and File Formatting

22baef7

Clarified comment for platformHostConfigOverride for easier reading. Fixed file endings.

Merge branch 'dev' of https://github.com/aws/amazon-ecs-agent into wi…

96ba42c

…n-swappiness

cabbruzzese force-pushed the win-swappiness branch from 6dd6f70 to 96ba42c Compare August 28, 2017 16:24

cabbruzzese merged commit 15463ab into aws:dev Aug 28, 2017

cabbruzzese mentioned this pull request Aug 30, 2017

ECS Windows Agent v1.14.4 #954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows MemorySwappiness Work Around #924

Windows MemorySwappiness Work Around #924

cabbruzzese commented Aug 11, 2017

samuelkarp left a comment

samuelkarp Aug 11, 2017

petderek left a comment

petderek Aug 11, 2017

cabbruzzese Aug 15, 2017

petderek Aug 15, 2017

petderek Aug 11, 2017

petderek Aug 11, 2017

vsiddharth left a comment

vsiddharth Aug 11, 2017

vsiddharth Aug 11, 2017

vsiddharth Aug 11, 2017

vsiddharth Aug 11, 2017

cabbruzzese Aug 15, 2017

samuelkarp commented Aug 16, 2017

samuelkarp left a comment

vsiddharth left a comment

Windows MemorySwappiness Work Around #924

Windows MemorySwappiness Work Around #924

Conversation

cabbruzzese commented Aug 11, 2017

Summary

Implementation details

Testing

Description for the changelog

Licensing

samuelkarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petderek 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

vsiddharth 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

samuelkarp commented Aug 16, 2017

samuelkarp left a comment

Choose a reason for hiding this comment

vsiddharth left a comment

Choose a reason for hiding this comment