Skip to content

Commit

Permalink
Flip default flag name and value
Browse files Browse the repository at this point in the history
  • Loading branch information
yhlee-aws committed Aug 16, 2018
1 parent 562752f commit 8b77755
Show file tree
Hide file tree
Showing 9 changed files with 35 additions and 35 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +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_MATCH_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`|
| `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`|

### Persistence

Expand Down
10 changes: 5 additions & 5 deletions agent/api/task/task.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func (task *Task) PostUnmarshalTask(cfg *config.Config,
if err != nil {
return apierrors.NewResourceInitError(task.Arn, err)
}
err = task.initializeDockerVolumes(cfg.SharedVolumeMatchNameOnly, dockerClient, ctx)
err = task.initializeDockerVolumes(cfg.SharedVolumeMatchAll, dockerClient, ctx)
if err != nil {
return apierrors.NewResourceInitError(task.Arn, err)
}
Expand Down Expand Up @@ -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(sharedVolumeMatchNameOnly bool, dockerClient dockerapi.DockerClient, ctx context.Context) error {
func (task *Task) initializeDockerVolumes(SharedVolumeMatchAll 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 {
Expand All @@ -304,7 +304,7 @@ func (task *Task) initializeDockerVolumes(sharedVolumeMatchNameOnly bool, docker
}
} else {
// Agent needs to create shared volume if that's auto provisioned
err := task.addSharedVolumes(sharedVolumeMatchNameOnly, ctx, dockerClient, &task.Volumes[i])
err := task.addSharedVolumes(SharedVolumeMatchAll, ctx, dockerClient, &task.Volumes[i])
if err != nil {
return err
}
Expand Down Expand Up @@ -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(sharedVolumeMatchNameOnly bool, ctx context.Context, dockerClient dockerapi.DockerClient,
func (task *Task) addSharedVolumes(SharedVolumeMatchAll bool, ctx context.Context, dockerClient dockerapi.DockerClient,
vol *TaskVolume) error {

volumeConfig := vol.Volume.(*taskresourcevolume.DockerVolumeConfig)
Expand Down Expand Up @@ -379,7 +379,7 @@ func (task *Task) addSharedVolumes(sharedVolumeMatchNameOnly bool, ctx context.C
}

seelog.Infof("initialize volume: Task [%s]: volume [%s] already exists", task.Arn, volumeConfig.DockerVolumeName)
if sharedVolumeMatchNameOnly {
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)
return nil
}
Expand Down
32 changes: 16 additions & 16 deletions agent/api/task/taskvolume_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func TestInitializeLocalDockerVolume(t *testing.T) {
}

func TestInitializeSharedProvisionedVolume(t *testing.T) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)

Expand Down Expand Up @@ -164,15 +164,15 @@ 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(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, 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) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)

Expand Down Expand Up @@ -203,12 +203,12 @@ 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(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, dockerClient, nil)
assert.Error(t, err, "volume not found for auto-provisioned resource should cause task to fail")
}

func TestInitializeSharedNonProvisionedVolume(t *testing.T) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)

Expand Down Expand Up @@ -244,15 +244,15 @@ func TestInitializeSharedNonProvisionedVolume(t *testing.T) {
Labels: map[string]string{"test": "test"},
},
})
err := testTask.initializeDockerVolumes(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, 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) {
sharedVolumeMatchNameOnly := true
SharedVolumeMatchAll := false

ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)
Expand Down Expand Up @@ -291,15 +291,15 @@ func TestInitializeSharedNonProvisionedVolumeValidateNameOnly(t *testing.T) {
Labels: nil,
},
})
err := testTask.initializeDockerVolumes(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, 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) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)

Expand Down Expand Up @@ -329,14 +329,14 @@ 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(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, 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) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)

Expand Down Expand Up @@ -370,12 +370,12 @@ func TestInitializeSharedAutoprovisionVolumeNotMatchError(t *testing.T) {
Labels: map[string]string{"test": "test"},
},
})
err := testTask.initializeDockerVolumes(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, dockerClient, nil)
assert.Error(t, err, "volume resource details not match should cause task fail")
}

func TestInitializeSharedAutoprovisionVolumeTimeout(t *testing.T) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
ctrl := gomock.NewController(t)
dockerClient := mock_dockerapi.NewMockDockerClient(ctrl)

Expand Down Expand Up @@ -407,12 +407,12 @@ func TestInitializeSharedAutoprovisionVolumeTimeout(t *testing.T) {
dockerClient.EXPECT().InspectVolume(gomock.Any(), gomock.Any(), gomock.Any()).Return(dockerapi.VolumeResponse{
Error: &dockerapi.DockerTimeoutError{},
})
err := testTask.initializeDockerVolumes(sharedVolumeMatchNameOnly, dockerClient, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, dockerClient, nil)
assert.Error(t, err, "volume resource details not match should cause task fail")
}

func TestInitializeTaskVolume(t *testing.T) {
sharedVolumeMatchNameOnly := false
SharedVolumeMatchAll := true
testTask := &Task{
ResourcesMapUnsafe: make(map[string][]taskresource.TaskResource),
Containers: []*apicontainer.Container{
Expand All @@ -437,7 +437,7 @@ func TestInitializeTaskVolume(t *testing.T) {
},
}

err := testTask.initializeDockerVolumes(sharedVolumeMatchNameOnly, nil, nil)
err := testTask.initializeDockerVolumes(SharedVolumeMatchAll, 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")
Expand Down
8 changes: 4 additions & 4 deletions agent/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ const (
// DefaultTaskMetadataBurstRate is set to handle 60 burst requests at once
DefaultTaskMetadataBurstRate = 60

// DefaultSharedVolumeMatchNameOnly is set to true, requiring shared volumes to validate
// across name, driver options, and labels
DefaultSharedVolumeMatchNameOnly = false
// DefaultSharedVolumeMatchAll is set to false, only requiring shared volumes to match
// on names
DefaultSharedVolumeMatchAll = false
)

const (
Expand Down Expand Up @@ -424,7 +424,7 @@ func environmentConfig() (Config, error) {
CgroupPath: os.Getenv("ECS_CGROUP_PATH"),
TaskMetadataSteadyStateRate: steadyStateRate,
TaskMetadataBurstRate: burstRate,
SharedVolumeMatchNameOnly: utils.ParseBool(os.Getenv("ECS_SHARED_VOLUME_MATCH_NAME_ONLY"), false),
SharedVolumeMatchAll: utils.ParseBool(os.Getenv("ECS_SHARED_VOLUME_MATCH_NAME_ONLY"), false),
}, err
}

Expand Down
2 changes: 1 addition & 1 deletion agent/config/config_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func DefaultConfig() Config {
CgroupPath: defaultCgroupPath,
TaskMetadataSteadyStateRate: DefaultTaskMetadataSteadyStateRate,
TaskMetadataBurstRate: DefaultTaskMetadataBurstRate,
SharedVolumeMatchNameOnly: DefaultSharedVolumeMatchNameOnly,
SharedVolumeMatchAll: DefaultSharedVolumeMatchAll,
}
}

Expand Down
2 changes: 1 addition & 1 deletion agent/config/config_unix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +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.SharedVolumeMatchNameOnly, "Default SharedVolumeMatchNameOnly set incorrectly")
assert.False(t, cfg.SharedVolumeMatchAll, "Default SharedVolumeMatchAll set incorrectly")
}

// TestConfigFromFile tests the configuration can be read from file
Expand Down
2 changes: 1 addition & 1 deletion agent/config/config_windows.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func DefaultConfig() Config {
PlatformVariables: platformVariables,
TaskMetadataSteadyStateRate: DefaultTaskMetadataSteadyStateRate,
TaskMetadataBurstRate: DefaultTaskMetadataBurstRate,
SharedVolumeMatchNameOnly: DefaultSharedVolumeMatchNameOnly,
SharedVolumeMatchAll: DefaultSharedVolumeMatchAll,
}
}

Expand Down
2 changes: 1 addition & 1 deletion agent/config/config_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +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.SharedVolumeMatchNameOnly, "Default SharedVolumeMatchNameOnly set incorrectly")
assert.False(t, cfg.SharedVolumeMatchAll, "Default SharedVolumeMatchAll set incorrectly")
}

func TestConfigIAMTaskRolesReserves80(t *testing.T) {
Expand Down
10 changes: 5 additions & 5 deletions agent/config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ type Config struct {
// TaskMetadataBurstRate specifies the burst rate throttle for the task metadata endpoint
TaskMetadataBurstRate int

// SharedVolumeMatchNameOnly is config option used to short-circuit volume validation against a
// 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.
SharedVolumeMatchNameOnly bool
// SharedVolumeMatchAll 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.
SharedVolumeMatchAll bool
}

0 comments on commit 8b77755

Please sign in to comment.