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

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

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
 

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)

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,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(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 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,15 +247,65 @@ 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")
 	assert.Len(t, testTask.Containers[0].TransitionDependenciesMap, 0, "resource already exists")
 }
 
 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")

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
 }
 

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

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
 	}
 }
 

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

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
 	}
 }
 

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) {

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
 }