-
Notifications
You must be signed in to change notification settings - Fork 921
[core] sky status db query optimization (3) #6871
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
Merged
Merged
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
2315c9f to
4e8dfdb
Compare
Collaborator
Author
|
/quicktest-core (PASSING) |
Collaborator
Author
|
/smoke-test --aws https://buildkite.com/skypilot-1/smoke-tests/builds/2831 (PASSING) |
Collaborator
Author
|
/smoke-test --aws --postgres https://buildkite.com/skypilot-1/smoke-tests/builds/2833 |
bc84702 to
9739433
Compare
9739433 to
4b4ac2a
Compare
lloyd-brown
reviewed
Aug 28, 2025
Collaborator
lloyd-brown
left a comment
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.
Looks good!
lloyd-brown
approved these changes
Aug 28, 2025
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.
The purpose of this PR is to reduce the number of
global_user_state.get_cluster_yaml_dictcalls during handling ofsky status.Currently,
global_user_state.get_cluster_yaml_dictis called once perbackend_utils.ssh_credential_from_yaml, which is called in_update_record_with_credentials_and_resources_str. This function is called once per cluster. When an API server has many clusters, the number of DB calls increase linearly.By renaming and refactoring
_update_record_with_credentials_and_resources_strinto_update_records_with_credentials_and_resources_str, we can allow the function to make a single db call to get all ssh credentials for the clusters. Appropriate changes are made inbackend_utils.pyandglobal_user_state.pyto facilitate this change.Testing: Testing is done entirely with smoke tests. Since
sky statusis one of the most comprehensively tested calls in smoke tests, smoke test should be enough to ensure lack of regression.Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)