Support Docker Volume Configuration in ECS Params #587

PettitWesley · 2018-08-21T07:09:37Z

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

allisaurus · 2018-08-21T21:22:53Z

README.md

+    - name: string
+      scope: string ("shared" or "task")
+      autoprovision: boolean (only valid if scope = "shared")
+      driver: string


Is there a discrete list of valid entries for this field (or others) and we can call out with a comment? Thinking about how we annotated task placement args.

There are a bunch of volume plugins out there (aka volume 'drivers'), and new ones will be created in the future; and putting a list here would be misleading because ECS accepts any string for this value.

Makes sense. Thanks!

I was referring to having comments versus parentheticals, e.g. scope: string // Valid values: "shared" | "task"

allisaurus · 2018-08-21T21:41:51Z

ecs-cli/modules/utils/compose/convert_task_definition.go

+		for _, dockerVol := range ecsParams.TaskDefinition.DockerVolumes {
+			if dockerVol.Name != "" {
+				log.Info("is it right here:")
+				log.Info(dockerVol)


I'm guessing the 2 comment above were for debugging and should be removed?

allisaurus · 2018-08-21T21:46:07Z

ecs-cli/modules/utils/compose/convert_task_definition.go

+			if dockerVol.Name != "" {
+				log.Info("is it right here:")
+				log.Info(dockerVol)
+				volumesWithoutHost[dockerVol.Name] = dockerVol


Seems this would override the value of a volume name specified in composeVolumes with the value of a vol with the same name specified in ecs-params. In this case, do we want to log a msg like "Using ecs-params values specified for volume %s" ?

With this PR, there are no fields valid except for name from Compose, so ECS Params is adding fields, not duplicated anything, so at the moment there is no overriding happening. This might change if we support more fields from compose volumes.

allisaurus · 2018-08-21T21:47:49Z

ecs-cli/modules/utils/compose/convert_task_definition.go

 		ecsVolume := &ecs.Volume{
 			Name: aws.String(volName),
 		}
+		if dVol.Name != "" {


Do we have validation before this that any volume specified in ecs-params includes a Name (or other required fields) ?

...and can we add a test for error case(s) handling if/when we do add validation?

allisaurus · 2018-08-21T21:51:29Z

ecs-cli/modules/utils/compose/convert_task_definition_test.go

+	}
+	options := map[string]string{
+		"pudding": "is Clyde's best friend",
+		"clyde":   "is too nervous to tell her that he loves her",


allisaurus · 2018-08-22T16:25:52Z

ecs-cli/modules/utils/compose/convert_task_definition.go

+			if dockerVol.Name != "" {
+				volumesWithoutHost[dockerVol.Name] = dockerVol
+			} else {
+				return nil, fmt.Errorf("docker_volume.name is a required field")


Since docker_volume.name is not the name of a specific ECS volume field (and the closest volume.name is listed as not required), can we rephrase this err msg to something like: "Name is required when specifying a docker volume" ?

Sure, I'll change it to that. I was thinking that user's would understand that docker_volume.name is the ECS Params yaml.

SoManyHs · 2018-08-22T18:00:45Z

README.md

+    - name: string
+      scope: string ("shared" or "task")
+      autoprovision: boolean (only valid if scope = "shared")
+      driver: string


I was referring to having comments versus parentheticals, e.g. scope: string // Valid values: "shared" | "task"

SoManyHs · 2018-08-22T18:21:08Z

README.md

  * In Docker compose version 2, the `cpu_shares`, `mem_limit`, and `mem_reservation` fields can be specified in either the compose or ECS params file. If they are specified in the ECS params file, the values will override values present in the compose file.

+* `docker_volumes` allows you to create volumes with a [DockerVolumeConfiguration](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/docker-volumes.html). The volumes can be referenced in your compose file by name, even if they were not also specified in the compose file.
+


I wonder if this should be moved to after the example, since line 463 says "Example ecs-params.yml with service resources specified:" and it's a little confusing for that to come after the section that talks about docker_volumes when there is no example of docker volumes.

Also, the link you provide here shows the JSON for how to specify dockerVolumeConfiguration as a top-level member of a volume undervolumes, but our schema doesn't seem to quite match that. It might be useful to have a bulleted list that explains each of our fields explicitly if we're breaking from the ECS JSON schema.

Also, can you remind me again why we aren't nesting the relevant fields under the dockerVolumeConfiguration subkey?

Finally, I see in the docs there is field called host. Is that not needed here?

We decided in the design review that docker_volumes looks nicer than having docker_volume_configuration as the key. We also decided that it looked better if all of the fields were under that key, and not to have an extra layer of nesting (which the Task Def does for backwards compatibility with the old volumes and forwards compatibility with any other types of non-docker volumes that could hypothetically be used in the future).

Host is not compatible with docker volume configuration, AFAIK. The ECS docs might actually need to be edited... let me get back to you on this.

SoManyHs · 2018-08-22T18:22:48Z

ecs-cli/modules/utils/compose/ecs_params_reader_test.go

+    - name: my_volume
+      scope: shared
+      autoprovision: true
+      driver: doggyromcom


🐶 ❤️ 🐶

SoManyHs · 2018-08-22T18:27:47Z

ecs-cli/modules/utils/compose/convert_task_definition.go

-	for _, volName := range hostPaths.VolumeEmptyHost {
+
+	// process docker volumes
+	dockerVolumes := make(map[string]DockerVolume)


dockerVolumes does not appear to be used after it's populated?

oh ughh oops, this is because I originally wrote this as one function and then did a bad job of copy pasting it into the separate function...

SoManyHs · 2018-08-22T18:29:36Z

ecs-cli/modules/utils/compose/convert_task_definition.go

+	}
+
+	if ecsParams != nil {
+		for _, dockerVol := range ecsParams.TaskDefinition.DockerVolumes {


This code seems to be a repeat of the code in lines 312-318. Could we extract that code into its own method that just has the responsibility of processing Docker Volumes from the ecs params? Then if you choose to call it before mergeVolumesWithoutHost, you can pass it into that function directly instead of re-parsing it here.

oh ughh oops, this is because I originally wrote this as one function and then did a bad job of copy pasting it into the separate function...

SoManyHs · 2018-08-22T19:01:43Z

ecs-cli/modules/utils/compose/ecs_params_reader_test.go

+        it: does-not-go-well
+        this: is-not-a-movie
+      labels:
+        pudding: mad


i mean, i feel like tum tum would be the mad pup in this scenario...🐶

PettitWesley · 2018-08-23T19:53:42Z

Tested it:

Compose file:

version: '2'
services:
  container1:
    image: nginx
    volumes:
      - shared:/shared
      - notshared:/notshared
      - /var/log:/sharedwithhost
  container2:
    image: nginx
    volumes:
      - shared:/sharedwithmetoo

volumes:
  shared:
  notshared:

ECS Params:

task_definition:
  docker_volumes:
    - name: shared
      scope: shared
      autoprovision: true
      driver: local
      driver_opts:
        some_option: some_value
      labels:
        testing: dockervolumes

Mountpoints on containers:

{
"mountPoints": [
        {
          "readOnly": false,
          "containerPath": "/shared",
          "sourceVolume": "shared"
        },
        {
          "readOnly": false,
          "containerPath": "/notshared",
          "sourceVolume": "notshared"
        },
        {
          "readOnly": false,
          "containerPath": "/sharedwithhost",
          "sourceVolume": "volume-2"
        }
      ],
"name": "container1"
}
...
{
"mountPoints": [
        {
          "readOnly": false,
          "containerPath": "/sharedwithmetoo",
          "sourceVolume": "shared"
        }
      ],
"name": "container2"
}

Task Def Volumes section:

  "volumes": [
    {
      "name": "volume-2",
      "host": {
        "sourcePath": "/var/log"
      },
      "dockerVolumeConfiguration": null
    },
    {
      "name": "notshared",
      "host": {
        "sourcePath": null
      },
      "dockerVolumeConfiguration": null
    },
    {
      "name": "shared",
      "host": null,
      "dockerVolumeConfiguration": {
        "autoprovision": true,
        "labels": {
          "testing": "dockervolumes"
        },
        "scope": "shared",
        "driver": "local",
        "driverOpts": {
          "some_option": "some_value"
        }
      }
    }
  ]

allisaurus · 2018-08-24T20:27:20Z

README.md

  * In Docker compose version 2, the `cpu_shares`, `mem_limit`, and `mem_reservation` fields can be specified in either the compose or ECS params file. If they are specified in the ECS params file, the values will override values present in the compose file.
  * If you are using a private repository for pulling images, `repository_credentials` allows you to specify an AWS Secrets Manager secret ARN for the name of the secret containing your private repository credentials as a `credential_parameter`.

-Example `ecs-params.yml` with service resources specified:


since you're removing this services example, can you add another sub-bullet under services that says something like: "See sample ecs-params.yml below for example uses."

Is that really needed? The way it reads right now is that we have the schema, then we have explanations, then we have a bunch of examples. I think that makes complete sense and there doesn't need to be a special bullet under services noting that there are examples for this field.

If this is the case of all schema field examples (and not just that you moved or omitted the service one) than I agree it's fine as is.

PettitWesley requested review from SoManyHs and allisaurus August 21, 2018 07:09

PettitWesley changed the title ~~Support Docker Volume Configuration in ECS Params~~ WIP: Support Docker Volume Configuration in ECS Params Aug 21, 2018

PettitWesley removed request for SoManyHs and allisaurus August 21, 2018 07:51

PettitWesley force-pushed the params-docker-volumes branch from 6a17850 to 3a7339b Compare August 21, 2018 17:37

PettitWesley changed the title ~~WIP: Support Docker Volume Configuration in ECS Params~~ Support Docker Volume Configuration in ECS Params Aug 21, 2018

PettitWesley requested review from SoManyHs and allisaurus August 21, 2018 17:37

allisaurus suggested changes Aug 21, 2018

View reviewed changes

allisaurus suggested changes Aug 22, 2018

View reviewed changes

SoManyHs suggested changes Aug 22, 2018

View reviewed changes

Updated AWS SDK Go to v1.15.16

7f7f7fd

PettitWesley force-pushed the params-docker-volumes branch from d70b3a0 to a872cfe Compare August 22, 2018 23:03

allisaurus reviewed Aug 24, 2018

View reviewed changes

allisaurus approved these changes Sep 4, 2018

View reviewed changes

SoManyHs approved these changes Sep 4, 2018

View reviewed changes

Added support for defining docker volume configuration in ECS Params

9fea4a5

PettitWesley force-pushed the params-docker-volumes branch from 6ba5a56 to 9fea4a5 Compare September 5, 2018 23:07

PettitWesley merged commit 1283259 into aws:dev Sep 6, 2018

PettitWesley mentioned this pull request Sep 14, 2018

Support Docker Volume Configuration Via Compose #607

Open

PettitWesley mentioned this pull request Sep 16, 2019

errors with managed docker volume plugins with ecs-cli compose service up #927

Closed

		* In Docker compose version 2, the `cpu_shares`, `mem_limit`, and `mem_reservation` fields can be specified in either the compose or ECS params file. If they are specified in the ECS params file, the values will override values present in the compose file.

		* `docker_volumes` allows you to create volumes with a [DockerVolumeConfiguration](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/docker-volumes.html). The volumes can be referenced in your compose file by name, even if they were not also specified in the compose file.

Support Docker Volume Configuration in ECS Params #587

Support Docker Volume Configuration in ECS Params #587

Uh oh!

Conversation

PettitWesley commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PettitWesley Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PettitWesley Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PettitWesley Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PettitWesley commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PettitWesley Aug 22, 2018 •

edited

Loading

PettitWesley Aug 22, 2018 •

edited

Loading

PettitWesley Aug 22, 2018 •

edited

Loading