diff --git a/CHANGELOG.md b/CHANGELOG.md index aa185e4965d..f61ded8f88c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,8 @@ # Changelog -## 1.21.0-dev +## 1.20.2-dev * Bug - Fixed a bug where unrecognized task cannot be stopped [#1467](https://github.com/aws/amazon-ecs-agent/pull/1467) +* Enhancement - Added ECS config field to provide optionally comparing shared volumes' full details (driver options and labels), or names only [#1519](https://github.com/aws/amazon-ecs-agent/pull/1519) ## 1.20.1 * Bug - Fixed a bug where the agent couldn't be upgraded if there are tasks that diff --git a/README.md b/README.md index d5232a87a93..6ef207088d2 100644 --- a/README.md +++ b/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 default Docker behavior. If a volume is shared across instances, this should be set to `false`. | `false` | `false`| ### Persistence diff --git a/agent/api/task/task.go b/agent/api/task/task.go index e5399baef61..d8e8347484d 100644 --- a/agent/api/task/task.go +++ b/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.SharedVolumeMatchFullConfig, dockerClient, ctx) if err != nil { return apierrors.NewResourceInitError(task.Arn, err) } @@ -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 { for i, vol := range task.Volumes { // No need to do this for non-docker volume, eg: host bind/empty volume if vol.Type != DockerVolumeType { @@ -304,7 +304,7 @@ func (task *Task) initializeDockerVolumes(dockerClient dockerapi.DockerClient, c } } else { // Agent needs to create shared volume if that's auto provisioned - err := task.addSharedVolumes(ctx, dockerClient, &task.Volumes[i]) + err := task.addSharedVolumes(sharedVolumeMatchFullConfig, ctx, dockerClient, &task.Volumes[i]) if err != nil { return err } @@ -336,7 +336,7 @@ func (task *Task) addTaskScopedVolumes(ctx context.Context, dockerClient dockera } // addSharedVolumes adds shared volume into task resources and updates container dependency -func (task *Task) addSharedVolumes(ctx context.Context, dockerClient dockerapi.DockerClient, +func (task *Task) addSharedVolumes(SharedVolumeMatchFullConfig bool, ctx context.Context, dockerClient dockerapi.DockerClient, vol *TaskVolume) error { volumeConfig := vol.Volume.(*taskresourcevolume.DockerVolumeConfig) @@ -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 !SharedVolumeMatchFullConfig { + seelog.Infof("initialize volume: Task [%s]: ECS_SHARED_VOLUME_MATCH_FULL_CONFIG is set to false and volume with name [%s] is found", task.Arn, volumeConfig.DockerVolumeName) + return nil + } + // validate all the volume metadata fields match to the configuration if len(volumeMetadata.DockerVolume.Labels) == 0 && len(volumeMetadata.DockerVolume.Labels) == len(volumeConfig.Labels) { seelog.Infof("labels are both empty or null: Task [%s]: volume [%s]", task.Arn, volumeConfig.DockerVolumeName) diff --git a/agent/api/task/taskvolume_test.go b/agent/api/task/taskvolume_test.go index e29a75ca0ae..fa10ec72518 100644 --- a/agent/api/task/taskvolume_test.go +++ b/agent/api/task/taskvolume_test.go @@ -133,7 +133,9 @@ func TestInitializeLocalDockerVolume(t *testing.T) { } func TestInitializeSharedProvisionedVolume(t *testing.T) { + sharedVolumeMatchFullConfig := true ctrl := gomock.NewController(t) + defer ctrl.Finish() dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) testTask := &Task{ @@ -163,7 +165,7 @@ 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(sharedVolumeMatchFullConfig, dockerClient, nil) assert.NoError(t, err) assert.Len(t, testTask.ResourcesMapUnsafe, 0, "no volume resource should be provisioned by agent") @@ -171,7 +173,9 @@ func TestInitializeSharedProvisionedVolume(t *testing.T) { } func TestInitializeSharedProvisionedVolumeError(t *testing.T) { + sharedVolumeMatchFullConfig := true ctrl := gomock.NewController(t) + defer ctrl.Finish() dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) testTask := &Task{ @@ -201,12 +205,14 @@ func TestInitializeSharedProvisionedVolumeError(t *testing.T) { // Expect the volume does not exists on the instance dockerClient.EXPECT().InspectVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(dockerapi.VolumeResponse{Error: errors.New("volume not exist")}) - err := testTask.initializeDockerVolumes(dockerClient, nil) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, dockerClient, nil) assert.Error(t, err, "volume not found for auto-provisioned resource should cause task to fail") } func TestInitializeSharedNonProvisionedVolume(t *testing.T) { + sharedVolumeMatchFullConfig := true ctrl := gomock.NewController(t) + defer ctrl.Finish() dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) testTask := &Task{ @@ -241,7 +247,55 @@ func TestInitializeSharedNonProvisionedVolume(t *testing.T) { Labels: map[string]string{"test": "test"}, }, }) - err := testTask.initializeDockerVolumes(dockerClient, nil) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, dockerClient, nil) + + assert.NoError(t, err) + assert.Len(t, testTask.ResourcesMapUnsafe, 0, "no volume resource should be provisioned by agent") + assert.Len(t, testTask.Containers[0].TransitionDependenciesMap, 0, "resource already exists") +} + +func TestInitializeSharedNonProvisionedVolumeValidateNameOnly(t *testing.T) { + sharedVolumeMatchFullConfig := false + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) + + testTask := &Task{ + ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource), + Containers: []*apicontainer.Container{ + { + MountPoints: []apicontainer.MountPoint{ + { + SourceVolume: "shared-volume-test", + ContainerPath: "/ecs", + }, + }, + TransitionDependenciesMap: make(map[apicontainerstatus.ContainerStatus]apicontainer.TransitionDependencySet), + }, + }, + Volumes: []TaskVolume{ + { + Name: "shared-volume-test", + Type: "docker", + Volume: &taskresourcevolume.DockerVolumeConfig{ + Scope: "shared", + Autoprovision: true, + DriverOpts: map[string]string{"type": "tmpfs"}, + Labels: map[string]string{"domain": "test"}, + }, + }, + }, + } + + // Expect the volume already exists on the instance + dockerClient.EXPECT().InspectVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(dockerapi.VolumeResponse{ + DockerVolume: &docker.Volume{ + Options: map[string]string{}, + Labels: nil, + }, + }) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, dockerClient, nil) assert.NoError(t, err) assert.Len(t, testTask.ResourcesMapUnsafe, 0, "no volume resource should be provisioned by agent") @@ -249,7 +303,9 @@ func TestInitializeSharedNonProvisionedVolume(t *testing.T) { } func TestInitializeSharedAutoprovisionVolumeNotFoundError(t *testing.T) { + sharedVolumeMatchFullConfig := true ctrl := gomock.NewController(t) + defer ctrl.Finish() dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) testTask := &Task{ @@ -278,14 +334,16 @@ func TestInitializeSharedAutoprovisionVolumeNotFoundError(t *testing.T) { } dockerClient.EXPECT().InspectVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(dockerapi.VolumeResponse{Error: errors.New("not found")}) - err := testTask.initializeDockerVolumes(dockerClient, nil) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, dockerClient, nil) assert.NoError(t, err) assert.Len(t, testTask.ResourcesMapUnsafe, 1, "volume resource should be provisioned by agent") assert.Len(t, testTask.Containers[0].TransitionDependenciesMap, 1, "volume resource should be in the container dependency map") } func TestInitializeSharedAutoprovisionVolumeNotMatchError(t *testing.T) { + sharedVolumeMatchFullConfig := true ctrl := gomock.NewController(t) + defer ctrl.Finish() dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) testTask := &Task{ @@ -318,12 +376,14 @@ func TestInitializeSharedAutoprovisionVolumeNotMatchError(t *testing.T) { Labels: map[string]string{"test": "test"}, }, }) - err := testTask.initializeDockerVolumes(dockerClient, nil) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, dockerClient, nil) assert.Error(t, err, "volume resource details not match should cause task fail") } func TestInitializeSharedAutoprovisionVolumeTimeout(t *testing.T) { + sharedVolumeMatchFullConfig := true ctrl := gomock.NewController(t) + defer ctrl.Finish() dockerClient := mock_dockerapi.NewMockDockerClient(ctrl) testTask := &Task{ @@ -354,11 +414,12 @@ func TestInitializeSharedAutoprovisionVolumeTimeout(t *testing.T) { dockerClient.EXPECT().InspectVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(dockerapi.VolumeResponse{ Error: &dockerapi.DockerTimeoutError{}, }) - err := testTask.initializeDockerVolumes(dockerClient, nil) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, dockerClient, nil) assert.Error(t, err, "volume resource details not match should cause task fail") } func TestInitializeTaskVolume(t *testing.T) { + sharedVolumeMatchFullConfig := true testTask := &Task{ ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource), Containers: []*apicontainer.Container{ @@ -383,7 +444,7 @@ func TestInitializeTaskVolume(t *testing.T) { }, } - err := testTask.initializeDockerVolumes(nil, nil) + err := testTask.initializeDockerVolumes(sharedVolumeMatchFullConfig, nil, nil) assert.NoError(t, err) assert.Len(t, testTask.ResourcesMapUnsafe, 1, "expect the resource map has an empty volume resource") assert.Len(t, testTask.Containers[0].TransitionDependenciesMap, 1, "expect a volume resource as the container dependency") diff --git a/agent/config/config.go b/agent/config/config.go index c629886bcc7..198e01c6c5e 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -420,6 +420,7 @@ func environmentConfig() (Config, error) { CgroupPath: os.Getenv("ECS_CGROUP_PATH"), TaskMetadataSteadyStateRate: steadyStateRate, TaskMetadataBurstRate: burstRate, + SharedVolumeMatchFullConfig: utils.ParseBool(os.Getenv("ECS_SHARED_VOLUME_MATCH_FULL_CONFIG"), false), }, err } diff --git a/agent/config/config_test.go b/agent/config/config_test.go index a0cf862c0e6..5e3b8bfa38e 100644 --- a/agent/config/config_test.go +++ b/agent/config/config_test.go @@ -87,6 +87,7 @@ func TestEnvironmentConfig(t *testing.T) { defer setTestEnv("ECS_INSTANCE_ATTRIBUTES", "{\"my_attribute\": \"testing\"}")() defer setTestEnv("ECS_ENABLE_TASK_ENI", "true")() defer setTestEnv("ECS_TASK_METADATA_RPS_LIMIT", "1000,1100")() + defer setTestEnv("ECS_SHARED_VOLUME_MATCH_FULL_CONFIG", "true")() additionalLocalRoutesJSON := `["1.2.3.4/22","5.6.7.8/32"]` setTestEnv("ECS_AWSVPC_ADDITIONAL_LOCAL_ROUTES", additionalLocalRoutesJSON) setTestEnv("ECS_ENABLE_CONTAINER_METADATA", "true") @@ -125,6 +126,7 @@ func TestEnvironmentConfig(t *testing.T) { assert.True(t, conf.ContainerMetadataEnabled, "Wrong value for ContainerMetadataEnabled") assert.Equal(t, 1000, conf.TaskMetadataSteadyStateRate) assert.Equal(t, 1100, conf.TaskMetadataBurstRate) + assert.True(t, conf.SharedVolumeMatchFullConfig, "Wrong value for SharedVolumeMatchFullConfig") } func TestTrimWhitespaceWhenCreating(t *testing.T) { @@ -381,6 +383,14 @@ func TestInvalidImagePullBehavior(t *testing.T) { assert.Equal(t, cfg.ImagePullBehavior, ImagePullDefaultBehavior, "Wrong value for ImagePullBehavior") } +func TestSharedVolumeMatchFullConfigEnabled(t *testing.T) { + defer setTestRegion()() + defer setTestEnv("ECS_SHARED_VOLUME_MATCH_FULL_CONFIG", "true")() + cfg, err := NewConfig(ec2.NewBlackholeEC2MetadataClient()) + assert.NoError(t, err) + assert.True(t, cfg.SharedVolumeMatchFullConfig, "Wrong value for SharedVolumeMatchFullConfig") +} + func TestParseImagePullBehavior(t *testing.T) { testcases := []struct { name string diff --git a/agent/config/config_unix.go b/agent/config/config_unix.go index 6d709285401..bc43dd41abd 100644 --- a/agent/config/config_unix.go +++ b/agent/config/config_unix.go @@ -68,6 +68,7 @@ func DefaultConfig() Config { CgroupPath: defaultCgroupPath, TaskMetadataSteadyStateRate: DefaultTaskMetadataSteadyStateRate, TaskMetadataBurstRate: DefaultTaskMetadataBurstRate, + SharedVolumeMatchFullConfig: false, // only requiring shared volumes to match on name, which is default docker behavior } } diff --git a/agent/config/config_unix_test.go b/agent/config/config_unix_test.go index 6b5c4b0b287..b0713dd1fa8 100644 --- a/agent/config/config_unix_test.go +++ b/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") } // TestConfigFromFile tests the configuration can be read from file diff --git a/agent/config/config_windows.go b/agent/config/config_windows.go index b48d94ea46a..c0d30bee133 100644 --- a/agent/config/config_windows.go +++ b/agent/config/config_windows.go @@ -91,6 +91,7 @@ func DefaultConfig() Config { PlatformVariables: platformVariables, TaskMetadataSteadyStateRate: DefaultTaskMetadataSteadyStateRate, TaskMetadataBurstRate: DefaultTaskMetadataBurstRate, + SharedVolumeMatchFullConfig: false, //only requiring shared volumes to match on name, which is default docker behavior } } diff --git a/agent/config/config_windows_test.go b/agent/config/config_windows_test.go index 59f63e2353d..d19e865147b 100644 --- a/agent/config/config_windows_test.go +++ b/agent/config/config_windows_test.go @@ -58,6 +58,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") } func TestConfigIAMTaskRolesReserves80(t *testing.T) { diff --git a/agent/config/types.go b/agent/config/types.go index 5d790b5b694..df3c7d440bf 100644 --- a/agent/config/types.go +++ b/agent/config/types.go @@ -218,4 +218,10 @@ type Config struct { // TaskMetadataBurstRate specifies the burst rate throttle for the task metadata endpoint TaskMetadataBurstRate int + + // SharedVolumeMatchFullConfig is config option used to short-circuit volume validation against a + // provisioned volume, if false (default). If true, we perform deep comparison including driver options + // and labels. For comparing shared volume across 2 instances, this should be set to false as docker's + // default behavior is to match name only, and does not propagate driver options and labels through volume drivers. + SharedVolumeMatchFullConfig bool }