-
Notifications
You must be signed in to change notification settings - Fork 11
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
aws-backend: add support for running instances on multiple subnets #700
Conversation
tests/unit/backend/test_aws.py
Outdated
@@ -70,7 +127,7 @@ def test_aws_instance_config(runner: Runner): | |||
assert instance["TagSpecifications"][1]["ResourceType"] == "volume" | |||
|
|||
|
|||
@mark.skipif(not os.getenv("AWS_ACCESS_KEY_ID"), reason="AWS credentials not found") | |||
@mark.skipif(not os.getenv("AWS_ACCESS_KEY_ID") and not os.getenv("AWS_PROFILE"), reason="AWS credentials not found") |
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.
Note: I use aws SSO profiles for auth, so needed to add a check for the AWS_PROFILE
env var to be able to run the tests.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (20.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
==========================================
- Coverage 86.68% 85.11% -1.58%
==========================================
Files 35 35
Lines 1510 1545 +35
==========================================
+ Hits 1309 1315 +6
- Misses 201 230 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hey @timbrown5 ! Thank you for the contribution! Haven't got the time to review it properly yet, but will do soon, in the meantime, don't worry about codecov complaining, we don't execute those tests in the CI, but do check the linting issues reported (should be easily fixed with a Cheers! |
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.
just a small change request, and let's merge it.
Thanks again
return runner | ||
|
||
def _create(self, runner: Runner, instance_resource: AwsInstance) -> Runner: | ||
instance = self.client.run_instances(**instance_resource) |
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.
@tcarmet, the linter is complaining about this line, but it's not a addition, just moved from the original code. Do you have a preferred way to fix it (I've silenced some warnings before, but I'm not 100% that that's the best plan).
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.
Thanks for letting me know, I'm not sure how to fix it as well, but let's merge it as is, I think it's minor and it won't complain on the next lint checks.
Update the AWS backend (and instance config) to add the ability to run instance on a set of subnets (and
so availability zone).
The main use-case for this is to allow instance to come up in multiple availability zones. A user would create one (or more) subnets in an availability zone and the list the subnets in the
subnet_configs
section. This has two main advantages:To support this (without breaking existing configs) I added a new
subnet_configs
section to the instance config. This is mutually exclusive to the existingsubnet_id
section.If
subnet_configs
is set, then the backend will choose one of the subnets at random and try to bring up the instance there. If this fails. then another subnet will be tried, until one works. If none of the subnets work, an the exception will be re-raised.