Add GPU support to compose service #750
Conversation
Manual TestingNew FunctionalityConfigurationParameters# docker-compose.yml
version: '3'
services:
wordpress:
image: wordpress
ports:
- "80:80"
links:
- mysql
logging:
driver: awslogs
options:
awslogs-group: tutorial-wordpress
awslogs-region: us-east-1
awslogs-stream-prefix: wordpress
mysql:
image: mysql:5.7
environment:
MYSQL_ROOT_PASSWORD: password
logging:
driver: awslogs
options:
awslogs-group: tutorial-mysql
awslogs-region: us-east-1
awslogs-stream-prefix: mysql# ecs-params.yml
version: 1
task_definition:
services:
wordpress:
cpu_shares: 100
mem_limit: 524288000
gpu: "1"
mysql:
cpu_shares: 100
mem_limit: 524288000clusterThe cluster TestsCreate and run a task
Create and run a service
Increase the number of tasks in the service
No more instances available to run tasks
|
| @@ -123,6 +123,15 @@ func reconcileContainerDef(inputCfg *adapter.ContainerConfig, ecsConDef *Contain | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
|
|
|||
| if ecsConDef.Gpu != "" { | |||
There was a problem hiding this comment.
Will ecsConDef.Gpu ever be nil?
There was a problem hiding this comment.
From my understanding, ecsConDef.Gpu matches the gpu field in ecs-params.yml if the field exists. If the field is not in the container definition then it defaults to its zero value which is the empty string "". (I tested this)
So I don't think there can be a scenario where it's nil.
| @@ -66,6 +66,7 @@ type ContainerDef struct { | |||
| MemoryReservation libYaml.MemStringorInt `yaml:"mem_reservation"` | |||
| HealthCheck *HealthCheck `yaml:"healthcheck"` | |||
| Secrets []Secret `yaml:"secrets"` | |||
| Gpu string `yaml:"gpu"` | |||
There was a problem hiding this comment.
I realize that in the docs this field is specified as a string, but can we verify if there's any good reason why this can't be a number? we can always convert to string in our own code (though of course there is the annoying issue of the default empty value of an int being 0 in go, so would have to make sure that's handled correctly).
There was a problem hiding this comment.
I think it's best to make it a string since that's what the API takes, and for the default value reason which you mentioned.
There was a problem hiding this comment.
Also nit: This should be "GPU" to fit golang initialism practices
There was a problem hiding this comment.
@SoManyHs I went with string to keep it consistent with the API as @PettitWesley pointed it out. I'm scared of putting a different type than the backend in case they enable for example values like "two". Then our solution would be more limiting.
@PettitWesley fixed the variable name. I wrote that because I saw Cpu a few lines above it 😜
| @@ -66,6 +66,7 @@ type ContainerDef struct { | |||
| MemoryReservation libYaml.MemStringorInt `yaml:"mem_reservation"` | |||
| HealthCheck *HealthCheck `yaml:"healthcheck"` | |||
| Secrets []Secret `yaml:"secrets"` | |||
| GPU string `yaml:"gpu"` | |||
There was a problem hiding this comment.
In the container definition, GPU resources are specified within a list of 'resourceRequirement' objects, vs. as a flat value. If new resourceRequirement types are added in the future, how do you propose we add them? Flattened as well? What if multiple requirements of the same type are allowed? Wondering what your ideas are here re: keeping this extensible.
One alternative would be to take in a list of these, like we do for secrets:
secrets:
- value_from: string
name: string
...
resource_requirements:
- type: GPU
value: 3
There was a problem hiding this comment.
Ask @efekarakus to explain this offline, he has an explanation for why he chose this approach. I had the same question.
There was a problem hiding this comment.
Discussed offline 😝
There was a problem hiding this comment.
Looks like this was resolved, but Efe was following a design choice I made in a previous design doc.
Issue #, if available: 729
Description of changes: Add GPU support to compose service.
Testing: All unit tests pass, see the comment section for manual testing.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.