[autoscaler][AWS] Make sure subnets belong to same VPC as user-specified security groups #13558

DmitriGekhtman · 2021-01-19T16:54:27Z

Why are these changes needed?

This PR addresses a problem in autoscaler/_private/aws/config.py.
Currently _configure_subnets does not take into account security groups specified in the head_node and worker_nodes fields.
Thus, it's possible that a subnet is chosen that belongs to a different VPC than user-specified security groups. In this situation, a cluster can't be launched.

This PR fixes that problem by detecting the VPC of security groups specified in head_node and worker_nodes configs and restricting to subnets in that VPC.

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

I also did some manual tests of _configure_subnets and of ray up to make sure things were working as expected.

DmitriGekhtman · 2021-01-19T19:45:37Z

looks like test_autoscaler_aws isn't happy... better go fix that.

DmitriGekhtman · 2021-01-20T01:53:27Z

Currently,
test_autoscaler_aws.test_create_sg_different_vpc_same_rules and test_autoscaler_aws.test_create_sg_with_custom_inbound_rules_and_name are failing.

I'm not sure I fully understand the context behind the test failures (and vpc / sg / subnet setup in general).
@allenyin55 could you take a look if you have a second?

allenyin55 · 2021-01-20T03:06:48Z

test_autoscaler_aws.test_create_sg_different_vpc_same_rules and test_autoscaler_aws.test_create_sg_with_custom_inbound_rules_and_name are testing logics for creating security groups and they stub out the ec2 and IAM client. With your code change we need to update how the ec2 client is stubbed. You can find the stubs in tests/aws/utils/stubs.py

DmitriGekhtman · 2021-01-20T03:27:47Z

Thanks, will look into that.

…

On Tue, Jan 19, 2021 at 19:07 Allen ***@***.***> wrote: test_autoscaler_aws.test_create_sg_different_vpc_same_rules and test_autoscaler_aws.test_create_sg_with_custom_inbound_rules_and_name are testing logics for creating security groups and they stub out the ec2 and IAM client. With your code change we need to update how the ec2 client is stubbed. You can find the stubs in tests/aws/utils/stubs.py — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#13558 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APAQTK7WAPYC3GCKZC5O2LTS2ZCFLANCNFSM4WJCAXDA> .

DmitriGekhtman · 2021-01-20T16:04:17Z

Stubs are now fixed and tests pass.

Should I add a unit test to test_autoscaler_aws or is this PR fine as is?

AmeerHajAli · 2021-01-20T18:34:38Z

@DmitriGekhtman , lets add a unit test to make sure everything is OK.

DmitriGekhtman · 2021-01-20T18:45:24Z

@DmitriGekhtman , lets add a unit test to make sure everything is OK.

Will do.

DmitriGekhtman · 2021-01-22T04:19:17Z

Added a unit test.

DmitriGekhtman · 2021-01-22T04:30:32Z

python/ray/autoscaler/_private/aws/config.py

+        "should belong to the same VPC."
+    cli_logger.doassert(len(vpc_ids) <= 1, multiple_vpc_msg)
+    assert len(vpc_ids) <= 1, multiple_vpc_msg
+


If the user specifies security groups for both head and worker, and the security groups belong to different VPCs, this will throw an error.

We could have it support user-specified security groups in different VPCs for head and workers.
This would match the behavior for subnets: #8374 .
I think I won't bother with that, unless reviewers think I should.

If we wanted to go further down that path, we would want at some point to support different VPCs for each node type in a multi node-type config.

AmeerHajAli

This looks good to me!
Nice job @DmitriGekhtman !
Let's see what @thomasdesr thinks about it :-).

thomasdesr

I'm comfortable with this as is, but it'd be great to have a unit test that demonstrates the bug.

E.g.

stub describe_subnets to return many subnets (1000?) each in different VPCs
bootstrap a config with explicitly security groups configuration targeting a single VPC
confirm the subnets returned after configuring are both in the security group's VPC and not any others.

DmitriGekhtman · 2021-01-26T00:31:42Z

Added a unit test that demonstrates that the bug is fixed. Changed the filtering logic a bit to enable the unit test.

python/ray/tests/aws/utils/constants.py

Co-authored-by: Thomas Desrosiers <681004+thomasdesr@users.noreply.github.com>

DmitriGekhtman · 2021-01-26T17:56:56Z

@ericl I think this is good to merge.

AmeerHajAli · 2021-01-26T22:04:04Z

@ericl, can you please merge this?

…ied security groups (ray-project#13558) * initial commit * Filter subnets by security groups' VPCs * fix stubs * wip * Fix inbound rule logic. Tests WIP. * wip * unit test * example yaml * Unit test tests for bug being fixed * Update python/ray/tests/aws/utils/constants.py Co-authored-by: Thomas Desrosiers <681004+thomasdesr@users.noreply.github.com> Co-authored-by: Thomas Desrosiers <681004+thomasdesr@users.noreply.github.com>

…r-specified security groups (ray-project#13558)" This reverts commit 748813a.

DmitriGekhtman added this to the Serverless Autoscaling milestone Jan 19, 2021

DmitriGekhtman assigned thomasdesr, ericl and AmeerHajAli Jan 19, 2021

DmitriGekhtman added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 19, 2021

AmeerHajAli requested review from thomasdesr and ericl January 19, 2021 20:10

DmitriGekhtman assigned allenyin55 Jan 20, 2021

DmitriGekhtman added tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. labels Jan 20, 2021

ericl removed their assignment Jan 20, 2021

AmeerHajAli added @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. and removed tests-ok The tagger certifies test failures are unrelated and assumes personal liability. labels Jan 20, 2021

DmitriGekhtman removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 22, 2021

DmitriGekhtman commented Jan 22, 2021

View reviewed changes

AmeerHajAli approved these changes Jan 24, 2021

View reviewed changes

thomasdesr reviewed Jan 25, 2021

View reviewed changes

DmitriGekhtman added 5 commits January 25, 2021 14:04

initial commit

bd25707

Filter subnets by security groups' VPCs

58d22cc

fix stubs

7ba499c

wip

c692cb8

Fix inbound rule logic. Tests WIP.

45e9374

DmitriGekhtman added 4 commits January 25, 2021 14:04

wip

5af9934

unit test

bcd1548

example yaml

40ebe6a

Unit test tests for bug being fixed

3db7bbb

DmitriGekhtman force-pushed the aws-subnet-sg-compatibility branch from 8f289c5 to 3db7bbb Compare January 26, 2021 00:29

AmeerHajAli requested a review from thomasdesr January 26, 2021 00:34

thomasdesr reviewed Jan 26, 2021

View reviewed changes

python/ray/tests/aws/utils/constants.py Outdated Show resolved Hide resolved

Update python/ray/tests/aws/utils/constants.py

cb77eea

Co-authored-by: Thomas Desrosiers <681004+thomasdesr@users.noreply.github.com>

AmeerHajAli added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 26, 2021

DmitriGekhtman removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 26, 2021

DmitriGekhtman added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jan 26, 2021

ericl merged commit 40234ad into ray-project:master Jan 28, 2021

fishbone added a commit to fishbone/ray that referenced this pull request Feb 16, 2021

Revert "[autoscaler][AWS] Make sure subnets belong to same VPC as use…

65cc74f

…r-specified security groups (ray-project#13558)" This reverts commit 748813a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[autoscaler][AWS] Make sure subnets belong to same VPC as user-specified security groups #13558

[autoscaler][AWS] Make sure subnets belong to same VPC as user-specified security groups #13558

DmitriGekhtman commented Jan 19, 2021 •

edited

Loading

DmitriGekhtman commented Jan 19, 2021

DmitriGekhtman commented Jan 20, 2021

allenyin55 commented Jan 20, 2021

DmitriGekhtman commented Jan 20, 2021 via email

DmitriGekhtman commented Jan 20, 2021

AmeerHajAli commented Jan 20, 2021

DmitriGekhtman commented Jan 20, 2021 •

edited

Loading

DmitriGekhtman commented Jan 22, 2021

DmitriGekhtman Jan 22, 2021 •

edited

Loading

DmitriGekhtman Jan 22, 2021

AmeerHajAli left a comment •

edited

Loading

thomasdesr left a comment

DmitriGekhtman commented Jan 26, 2021

DmitriGekhtman commented Jan 26, 2021

AmeerHajAli commented Jan 26, 2021

[autoscaler][AWS] Make sure subnets belong to same VPC as user-specified security groups #13558

[autoscaler][AWS] Make sure subnets belong to same VPC as user-specified security groups #13558

Conversation

DmitriGekhtman commented Jan 19, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

DmitriGekhtman commented Jan 19, 2021

DmitriGekhtman commented Jan 20, 2021

allenyin55 commented Jan 20, 2021

DmitriGekhtman commented Jan 20, 2021 via email

DmitriGekhtman commented Jan 20, 2021

AmeerHajAli commented Jan 20, 2021

DmitriGekhtman commented Jan 20, 2021 • edited Loading

DmitriGekhtman commented Jan 22, 2021

DmitriGekhtman Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

DmitriGekhtman Jan 22, 2021

Choose a reason for hiding this comment

AmeerHajAli left a comment • edited Loading

Choose a reason for hiding this comment

thomasdesr left a comment

Choose a reason for hiding this comment

DmitriGekhtman commented Jan 26, 2021

DmitriGekhtman commented Jan 26, 2021

AmeerHajAli commented Jan 26, 2021

DmitriGekhtman commented Jan 19, 2021 •

edited

Loading

DmitriGekhtman commented Jan 20, 2021 •

edited

Loading

DmitriGekhtman Jan 22, 2021 •

edited

Loading

AmeerHajAli left a comment •

edited

Loading