[Core] Improve sky down speed for AWS clusters with ports exposed
#6629
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
/smoke-test |
Collaborator
Author
|
/quicktest-core |
Collaborator
Author
|
/smoke-test |
Collaborator
Author
|
/quicktest-core |
3b559a6 to
f6874f2
Compare
Collaborator
Author
|
/quicktest-core |
Collaborator
Author
|
/smoke-test |
Collaborator
Author
|
/smoke-test |
Collaborator
Author
|
/quicktest-core |
Michaelvll
reviewed
Aug 13, 2025
Collaborator
Michaelvll
left a comment
There was a problem hiding this comment.
Thanks @lloyd-brown for making the changes! Just left some small comments : )
Comment on lines
+107
to
+114
| if expected_sg_name != aws_cloud.DEFAULT_SECURITY_GROUP_NAME: | ||
| # Ensure the default security group is created. This is needed | ||
| # to enable us to use the default security group to quickly | ||
| # delete the cluster. If the default security group is not created, | ||
| # we will need to block on instance termination to delete the | ||
| # security group. | ||
| _configure_security_group(ec2, vpc_id, | ||
| aws_cloud.DEFAULT_SECURITY_GROUP_NAME, []) |
Collaborator
There was a problem hiding this comment.
This might not work in the case when user account does not have permission to create security group. Previously, a user can specify the security group name to skip the creation, but now it will always be created? https://docs.skypilot.co/en/latest/reference/config.html#config-yaml-aws-security-group-name
| instances.terminate() | ||
| else: | ||
| # Case 4: We are managing the non-default sg. The default SG does not | ||
| # exist. We must block on instance termination. |
Collaborator
There was a problem hiding this comment.
We must block on instance termination so that ...
Add a reason to the comment : )
sky down speed for AWS
sky down speed for AWSsky down speed for AWS clusters with ports exposed
massaindustries
pushed a commit
to Seeweb/skypilot
that referenced
this pull request
Aug 26, 2025
* Move instances to default sg instead of wait for terminate. * Create default sg if not exist, wait on terminate if sg none. * Configure security group using vpc id. * Remove extended IP rules. * Update comment on sg configure. * Clean up teardown logic. * Bash format.sh.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull requests aims to reduce the amount of time it takes to run sky down on a cluster. The core issue is that when we expose ports we create a new security group that's deletion is blocked on the termination of the created instances. Incorporating Tian's fix we move those instances to the default security group instead of blocking on their termination. I also parallelized the call to change the security group as it's nontrivial (approx. 1s per instance).
This code was tested by creating a cluster with exposed ports with multiple nodes on AWS and then comparing the time it takes to run sky down on that cluster. For example, sky down on a 4 node cluster went from 58 seconds to 13 seconds.
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)