-
Notifications
You must be signed in to change notification settings - Fork 531
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
[Azure][Storage] Update default storage account naming with subscription id hash #3796
[Azure][Storage] Update default storage account naming with subscription id hash #3796
Conversation
@cblmemo @romilbhardwaj @Michaelvll ran additional backwards compatibility test and noted at the PR description at the top. This is ready for a look! |
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 @landscapepainter.
sky/data/storage.py
Outdated
# subscription, storage account names must be globally unique across all of | ||
# Azure users. Hence, the storage account name includes the subscription | ||
# hash as well to ensure its uniqueness. | ||
DEFAULT_STORAGE_ACCOUNT_NAME = 'sky{region}{user_hash}{subscription_hash}' |
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 like azure allows upto 24 characters for account name. With a 4 char for subscription hash, 8 chars for user hash and 3 chars for sky, we are getting dangerously to the limit (9 characters left). E.g., using region southcentralus
(14 chars) may cause Azure to return an error.
Can you check if we need to shorten it? If so, perhaps we can truncate the user hash and subscription has to 2 characters each?
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 catching this. We indeed have to shorten this. It seems like the longest region name is australiasoutheast
with 18 chars. So keeping subscription hash and user hash to 2 characters each would not work as the length becomes 25 with the prefix, sky
. Also using 1 character for either subscription hash or user hash seemed to be concerning as well. So I hashed the region and kept it to have length of 4. So this default storage account name will have a fixed length of 19 characters now.
@romilbhardwaj This is ready for another look! |
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 @landscapepainter. LGTM if tests pass.
This resolves #3783.
Tested (run the relevant ones):
Code formatting:
bash format.sh
Any manual or new tests for this PR (please specify below)
az login
to subscription Asky launch azure_blob.yaml
with subscription A, which creates/uses default storage account #Aaz logout
andaz login
to subscription Bsky launch azure_blob.yaml
with subscription B, whic creates/uses default storage account #BBackward compatibility tests:
Smoke tests
pytest tests/test_smoke.py::TestStorageWithCredentials --azure
pytest tests/test_smoke.py::test_azure_storage_mounts_with_stop --azure
sky launch comp_test.yaml --cloud azure -y
comp_test.yaml
: