-
Notifications
You must be signed in to change notification settings - Fork 619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow unbounded ram tasks windows #2239
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,9 @@ import ( | |
|
||
const ( | ||
minDockerClientAPIVersion = dockerclient.Version_1_24 | ||
|
||
nonZeroMemoryReservationValue = 1 | ||
expectedMemoryReservationValue = 0 | ||
) | ||
|
||
func TestPostUnmarshalWindowsCanonicalPaths(t *testing.T) { | ||
|
@@ -254,6 +257,52 @@ func TestCPUPercentBasedOnUnboundedEnabled(t *testing.T) { | |
} | ||
} | ||
|
||
func TestWindowsMemoryReservationOption(t *testing.T) { | ||
// Testing sending a task to windows overriding MemoryReservation value | ||
rawHostConfigInput := dockercontainer.HostConfig{ | ||
Resources: dockercontainer.Resources{ | ||
MemoryReservation: nonZeroMemoryReservationValue, | ||
}, | ||
} | ||
|
||
rawHostConfig, err := json.Marshal(&rawHostConfigInput) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
testTask := &Task{ | ||
Arn: "arn:aws:ecs:us-east-1:012345678910:task/c09f0188-7f87-4b0f-bfc3-16296622b6fe", | ||
Family: "myFamily", | ||
Version: "1", | ||
Containers: []*apicontainer.Container{ | ||
{ | ||
Name: "c1", | ||
DockerConfig: apicontainer.DockerConfig{ | ||
HostConfig: strptr(string(rawHostConfig)), | ||
}, | ||
}, | ||
}, | ||
PlatformFields: PlatformFields{ | ||
RamUnbounded: false, | ||
}, | ||
} | ||
|
||
// With RamUnbounded set to false, MemoryReservation is not overridden | ||
config, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion) | ||
if configErr != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be assert. same below There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can use require.NoError(t, err) if you want the exit behavior you get from t.fatal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
t.Fatal(configErr) | ||
} | ||
assert.EqualValues(t, nonZeroMemoryReservationValue, config.MemoryReservation) | ||
|
||
// With RamUnbounded set to true, tasks with no memory hard limit will have their memory reservation set to zero | ||
testTask.PlatformFields.RamUnbounded = true | ||
config, configErr = testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion) | ||
if configErr != nil { | ||
t.Fatal(configErr) | ||
} | ||
assert.EqualValues(t, expectedMemoryReservationValue, config.MemoryReservation) | ||
} | ||
|
||
func TestGetCanonicalPath(t *testing.T) { | ||
testcases := []struct { | ||
name string | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ func DefaultConfig() Config { | |
dataDir := filepath.Join(ecsRoot, "data") | ||
platformVariables := PlatformVariables{ | ||
CPUUnbounded: false, | ||
RAMUnbounded: false, | ||
} | ||
return Config{ | ||
DockerEndpoint: "npipe:////./pipe/docker_engine", | ||
|
@@ -118,8 +119,10 @@ func (cfg *Config) platformOverrides() { | |
cfg.TaskCPUMemLimit = ExplicitlyDisabled | ||
|
||
cpuUnbounded := utils.ParseBool(os.Getenv("ECS_ENABLE_CPU_UNBOUNDED_WINDOWS_WORKAROUND"), false) | ||
ramUnbounded := utils.ParseBool(os.Getenv("ECS_ENABLE_RAM_UNBOUNDED_WINDOWS_WORKAROUND"), false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe these need unit tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added tests to check default config |
||
platformVariables := PlatformVariables{ | ||
CPUUnbounded: cpuUnbounded, | ||
RAMUnbounded: ramUnbounded, | ||
} | ||
cfg.PlatformVariables = platformVariables | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would be a good counterpoint for calling this
ECS_ENABLE_MEMORY_UNBOUNDED_WINDOWS_WORKAROUND
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/allow tasks with no memory reservation/ignore the memory reservation parameter (soft limit)/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im okay with this