-
Notifications
You must be signed in to change notification settings - Fork 519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add aws-ecs-1-nvidia variant #2128
Add aws-ecs-1-nvidia variant #2128
Conversation
0659924
to
7bcf511
Compare
(Forced push adds documentation for the variant) |
7bcf511
to
9515e76
Compare
(Forced push fixes link and JSON blob in documentation) |
Signed-off-by: Arnaldo Garcia Rincon <[email protected]>
9515e76
to
1e407b0
Compare
(Forced push fixes JSON blob in documentation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM.
@@ -0,0 +1 @@ | |||
../../../shared-defaults/docker-services.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file has four parts:
- docker service definition + restart command
- docker daemon config file
- container registry mirrors metadata
- container registry credentials metadata
We end up overriding (2) in 53-docker-daemon.toml, and (4) in 52-aws-ecs-1.toml.
It works in the sense that we're ending up with the right values, but I wonder if there's a way to refactor this that's easier to follow.
If nothing comes to mind, I'm OK with this going in. However, it's close to the ceiling of acceptable complexity because it makes migrations in this area even harder to reason about than usual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to propose something to safely improve these configuration files 👍
@@ -1,5 +1,6 @@ | |||
%global _cross_first_party 1 | |||
%global _is_k8s_variant %(if echo %{_cross_variant} | grep -Fqw "k8s"; then echo 1; else echo 0; fi) | |||
%global _is_ecs_variant %(if echo %{_cross_variant} | grep -Fqw "ecs"; then echo 1; else echo 0; fi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking but I'm slightly concerned about expanding this since users who create custom variants with names that contain any of these substrings will accidentally include packages they might not need. I wonder if we should tokenize the variant tuple into fields with awk
then checking the fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! Maybe we should have shared macros with your idea so that we don't increase the verbosity here.
Issue number:
Closes #1074
Description of changes:
Testing done:
In my ECS cluster, I created two daemon services, as follows:
container.resourceRequirement=GPU
I validated both tasks were scheduled in the new variant, for both x86_64/aarch64:
I validated only the task without the GPU requirement was scheduled in the existing variant, for both x86_64:
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.