Adding ecs config to flag name only or driver options and label comparison #1519

yhlee-aws · 2018-08-14T23:30:11Z

Summary

Making shared volume comparison configurable

Implementation details

SharedVolumeMatchAll boolean flag is added to ecs config. Default is false.
When false, it shortcircuits shared volume comparison after name matches. This is default Docker behavior.
When true, it compares driver options and labels.

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sharanyad · 2018-08-14T23:32:55Z

agent/config/types.go

+	// provisioned volume, if true. If false, we perform deep comparison including driver options and labels.
+	// For comparing shared volume across 2 instances, this should be set to true as docker does not propagate
+	// driver options and labels through volume drivers.
+	SharedVolumeValidateNameOnly bool


What is the actual environment variable name?

added ECS_SHARED_VOLUME_VALIDATE_NAME_ONLY

sharanyad · 2018-08-15T20:27:57Z

README.md

@@ -183,6 +183,7 @@ additional details on each available environment variable.
 | `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_WORKAROUND` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |
 | `ECS_TASK_METADATA_RPS_LIMIT` | `100,150` | Comma separated integer values for steady state and burst throttle limits for task metadata endpoint | `40,60` | `40,60` |
+| `ECS_SHARED_VOLUME_VALIDATE_NAME_ONLY` | `true` | When `true`, ECS will short circuit shared volume match after name. When `false`, both driver options and labels are compared. If a volume is shared across instances, this should be set to `true`. | `false` | `false`|


should we call it ECS_SHARED_VOLUME_MATCH_NAME_ONLY ? also, we should probably get this reviewed by someone in docs.

I agree, *match_name_only is more descriptive of what this does. Changed.

IMO small README changes like this shouldn't require doc reviewers.

sharanyad · 2018-08-15T20:29:17Z

agent/api/task/task.go

@@ -379,6 +379,11 @@ func (task *Task) addSharedVolumes(ctx context.Context, dockerClient dockerapi.D
 	}

 	seelog.Infof("initialize volume: Task [%s]: volume [%s] already exists", task.Arn, volumeConfig.DockerVolumeName)
+	if cfg.SharedVolumeValidateNameOnly {
+		seelog.Infof("initialize volume: Task [%s]: SharedVolumeValidateNameOnly is set to true and volume with name [%s] is found", task.Arn, volumeConfig.DockerVolumeName)


nit: ECS_SHARED_VOLUME_VALIDATE_NAME_ONLY is set to true...

sharanyad · 2018-08-15T20:34:25Z

agent/api/task/task.go

@@ -226,7 +226,7 @@ func (task *Task) PostUnmarshalTask(cfg *config.Config,
 	if err != nil {
 		return apierrors.NewResourceInitError(task.Arn, err)
 	}
-	err = task.initializeDockerVolumes(dockerClient, ctx)
+	err = task.initializeDockerVolumes(cfg, dockerClient, ctx)


Should we just pass in the bool variable instead of cfg object itself?

fenxiong · 2018-08-15T23:49:54Z

agent/api/task/taskvolume_test.go

+	cfg := config.Config{}
+	cfg.SharedVolumeValidateNameOnly = true
+
+	ctrl := gomock.NewController(t)


i think you need to have 'defer ctrl.Finish()' after that so the EXPECTed call will be checked?

added these on a separate commit

sharanyad · 2018-08-16T22:24:12Z

README.md

@@ -183,6 +183,7 @@ additional details on each available environment variable.
 | `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_WORKAROUND` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |
 | `ECS_TASK_METADATA_RPS_LIMIT` | `100,150` | Comma separated integer values for steady state and burst throttle limits for task metadata endpoint | `40,60` | `40,60` |
+| `ECS_SHARED_VOLUME_MATCH_ALL` | `true` | When `true`, ECS Agent will compare driver options and labels to make sure volumes are identical. When `false`, Agent will short circuit shared volume comparison if the names match. This is the defualt Docker behavior. If a volume is shared across instances, this should be set to `false`. | `false` | `false`|


nit: default.
When 'true', ECS Agent will compare driver options and labels along with name to..

sharanyad · 2018-08-16T22:25:14Z

agent/api/task/task.go

@@ -379,6 +379,11 @@ func (task *Task) addSharedVolumes(ctx context.Context, dockerClient dockerapi.D
 	}

 	seelog.Infof("initialize volume: Task [%s]: volume [%s] already exists", task.Arn, volumeConfig.DockerVolumeName)
+	if !SharedVolumeMatchAll {
+		seelog.Infof("initialize volume: Task [%s]: ECS_SHARED_MATCH_VALIDATE_NAME_ONLY is set to true and volume with name [%s] is found", task.Arn, volumeConfig.DockerVolumeName)


Should be ECS_SHARED_VOLUME_MATCH_ALL is set to false

sharanyad · 2018-08-16T22:27:12Z

agent/api/task/taskvolume_test.go

@@ -163,15 +165,17 @@ func TestInitializeSharedProvisionedVolume(t *testing.T) {

 	// Expect the volume already exists on the instance
 	dockerClient.EXPECT().InspectVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(dockerapi.VolumeResponse{})
-	err := testTask.initializeDockerVolumes(dockerClient, nil)
+	err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, dockerClient, nil)


nit : can just pass in true instead of new variable

I prefer this way since it provides more semantics to the test; true/false value on its own does not show what it is for.

sharanyad · 2018-08-16T22:30:57Z

README.md

@@ -183,6 +183,7 @@ additional details on each available environment variable.
 | `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_WORKAROUND` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |
 | `ECS_TASK_METADATA_RPS_LIMIT` | `100,150` | Comma separated integer values for steady state and burst throttle limits for task metadata endpoint | `40,60` | `40,60` |
+| `ECS_SHARED_VOLUME_MATCH_ALL` | `true` | When `true`, ECS Agent will compare driver options and labels to make sure volumes are identical. When `false`, Agent will short circuit shared volume comparison if the names match. This is the defualt Docker behavior. If a volume is shared across instances, this should be set to `false`. | `false` | `false`|


How about ECS_SHARED_VOLUME_MATCH_FULL_CONFIG or ECS_SHARED_VOLUME_MATCH_ALL_CONFIG?

richardpen · 2018-08-17T20:42:14Z

agent/config/config_unix_test.go

@@ -64,6 +64,7 @@ func TestConfigDefault(t *testing.T) {
 		"Default TaskMetadataSteadyStateRate is set incorrectly")
 	assert.Equal(t, DefaultTaskMetadataBurstRate, cfg.TaskMetadataBurstRate,
 		"Default TaskMetadataBurstRate is set incorrectly")
+	assert.False(t, cfg.SharedVolumeMatchFullConfig, "Default SharedVolumeMatchFullConfig set incorrectly")


Can you also modify the TestEnvironmentConfig in config_test.go to cover this?

I started adding that last night and will finish that up.

Created a PR to this branch to be accepted and it'll be added to this PR.

tiffanyfay · 2018-08-18T02:54:44Z

README.md

@@ -183,6 +183,7 @@ additional details on each available environment variable.
 | `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_WORKAROUND` | `true` | When `true`, ECS will allow CPU unbounded(CPU=`0`) tasks to run along with CPU bounded tasks in Windows. | Not applicable | `false` |
 | `ECS_TASK_METADATA_RPS_LIMIT` | `100,150` | Comma separated integer values for steady state and burst throttle limits for task metadata endpoint | `40,60` | `40,60` |
+| `ECS_SHARED_VOLUME_MATCH_FULL_CONFIG` | `true` | When `true`, ECS Agent will compare name, driver options, and labels to make sure volumes are identical. When `false`, Agent will short circuit shared volume comparison if the names match. This is the defualt Docker behavior. If a volume is shared across instances, this should be set to `false`. | `false` | `false`|


tiffanyfay · 2018-08-18T03:07:51Z

To follow how the other names are, though it's long, but should
SharedVolumeMatchFullConfig -> SharedVolumeMatchFullConfigEnabled
ECS_SHARED_VOLUME_MATCH_FULL_CONFIG -> ECS_ENABLE_SHARED_VOLUME_MATCH_FULL_CONFIG

tiffanyfay · 2018-08-20T18:21:01Z

agent/config/config.go

+
+	// DefaultSharedVolumeMatchFullConfig is set to false, only requiring shared volumes to match
+	// on names
+	DefaultSharedVolumeMatchFullConfig = false


Figured I would comment here as well. Suggestion: remove DefaultSharedVolumeMatchFullConfig in all places as the default is already set later in this file and doesn't need to be repeated in config_windows.go and config_unix.go as they are already defaulting to false. The tests can stay.

yhlee-aws · 2018-08-20T20:47:42Z

"match" is the verb and I don't think it detracts anything by not having "enable" in the name.

sharanyad · 2018-08-20T21:29:41Z

Shorter env var names are preferable and since ECS_SHARED_VOLUME_MATCH_FULL_CONFIG conveys the full meaning, we can leave it as is imo. Naming the bool variable with "enabled" suffix in the code is fine though.

sharanyad

code lgtm. This requires a CHANGELOG entry as well.

sharanyad · 2018-08-21T17:26:54Z

agent/api/task/task.go

@@ -285,7 +285,7 @@ func (task *Task) volumeName(name string) string {

 // initializeDockerVolumes checks the volume resource in the task to determine if the agent
 // should create the volume before creating the container
-func (task *Task) initializeDockerVolumes(dockerClient dockerapi.DockerClient, ctx context.Context) error {
+func (task *Task) initializeDockerVolumes(SharedVolumeMatchFullConfig bool, dockerClient dockerapi.DockerClient, ctx context.Context) error {


nit: this can just be sharedVolumeMatchFullConfig (not a public var)

…rison for shared volumees

yhlee-aws added this to the 1.20.2 milestone Aug 14, 2018

yhlee-aws requested review from sharanyad and richardpen August 14, 2018 23:30

sharanyad reviewed Aug 14, 2018

View reviewed changes

yhlee-aws force-pushed the shared_volume_validation_with_config branch 2 times, most recently from 0150770 to 7e4c529 Compare August 15, 2018 00:34

sharanyad reviewed Aug 15, 2018

View reviewed changes

fenxiong reviewed Aug 15, 2018

View reviewed changes

yhlee-aws force-pushed the shared_volume_validation_with_config branch 2 times, most recently from 3f5aad7 to 562752f Compare August 16, 2018 16:47

fenxiong approved these changes Aug 16, 2018

View reviewed changes

yhlee-aws force-pushed the shared_volume_validation_with_config branch 2 times, most recently from 8b77755 to 8b60428 Compare August 16, 2018 22:16

sharanyad reviewed Aug 16, 2018

View reviewed changes

yhlee-aws force-pushed the shared_volume_validation_with_config branch 4 times, most recently from 99eacc0 to ed97470 Compare August 17, 2018 05:52

richardpen reviewed Aug 17, 2018

View reviewed changes

tiffanyfay reviewed Aug 18, 2018

View reviewed changes

tiffanyfay reviewed Aug 20, 2018

View reviewed changes

yhlee-aws force-pushed the shared_volume_validation_with_config branch 2 times, most recently from 223730b to 10471bf Compare August 21, 2018 00:23

sharanyad reviewed Aug 21, 2018

View reviewed changes

yhlee-aws and others added 3 commits August 21, 2018 11:57

Adding ecs config to flag name only or driver options and label compa…

587270f

…rison for shared volumees

Added testing for SharedVolumeMatchFullConfig in config_test.go

416da6c

adding missing ctrl.Finish calls

8829aa4

yhlee-aws force-pushed the shared_volume_validation_with_config branch from 7361dd2 to 8829aa4 Compare August 21, 2018 18:58

petderek approved these changes Aug 21, 2018

View reviewed changes

yhlee-aws merged commit 610cfff into aws:dev Aug 21, 2018

yhlee-aws deleted the shared_volume_validation_with_config branch August 21, 2018 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ecs config to flag name only or driver options and label comparison #1519

Adding ecs config to flag name only or driver options and label comparison #1519

yhlee-aws commented Aug 14, 2018 •

edited

Loading

sharanyad Aug 14, 2018

yhlee-aws Aug 15, 2018 •

edited

Loading

sharanyad Aug 15, 2018

yhlee-aws Aug 16, 2018

yhlee-aws Aug 16, 2018

sharanyad Aug 15, 2018

sharanyad Aug 15, 2018

fenxiong Aug 15, 2018

yhlee-aws Aug 16, 2018

sharanyad Aug 16, 2018

sharanyad Aug 16, 2018

sharanyad Aug 16, 2018

yhlee-aws Aug 17, 2018

sharanyad Aug 16, 2018

richardpen Aug 17, 2018

tiffanyfay Aug 17, 2018

tiffanyfay Aug 20, 2018

tiffanyfay Aug 18, 2018 •

edited

Loading

tiffanyfay commented Aug 18, 2018

tiffanyfay Aug 20, 2018

yhlee-aws commented Aug 20, 2018

sharanyad commented Aug 20, 2018

sharanyad left a comment

sharanyad Aug 21, 2018

Adding ecs config to flag name only or driver options and label comparison #1519

Adding ecs config to flag name only or driver options and label comparison #1519

Conversation

yhlee-aws commented Aug 14, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

Choose a reason for hiding this comment

yhlee-aws Aug 15, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiffanyfay Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

tiffanyfay commented Aug 18, 2018

Choose a reason for hiding this comment

yhlee-aws commented Aug 20, 2018

sharanyad commented Aug 20, 2018

sharanyad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhlee-aws commented Aug 14, 2018 •

edited

Loading

yhlee-aws Aug 15, 2018 •

edited

Loading

tiffanyfay Aug 18, 2018 •

edited

Loading