-
Notifications
You must be signed in to change notification settings - Fork 640
[Core] Add option to avoid uploading credentials and custom labels for GCP #2904
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
Conversation
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 @Michaelvll, did a pass and some questions.
f'{skypilot_config.loaded_config_path!r} for {cloud}, but it ' | ||
'not supported this cloud, please use \'USER\' instead.') | ||
excluded_clouds = [cloud] | ||
credentials = sky_check.get_cloud_credential_file_mounts(excluded_clouds) |
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.
If it's always a singleton, any reason to make the arg a list?
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.
It was for the generality of the function, as in the future it might be able to allow the remote instance to not have multiple clouds' credential, e.g., if the image has the cloud credential set already. I am ok to make it singleton first, but I guess it does not matter that much to have an argument as a list. : )
Actually a question: Do we want users to toggle this setting vs. always defaulting to use a service account for clouds that support it? |
…upload-credential
I think it might be better to ask the user to specify the config explicitly to turn this on. Reasons:
Wdyt? |
For (2) & (3): Is it true that our main branch currently uses service accounts for AWS/GCP remote nodes? IIRC service accounts take precedence over the credential files present on those nodes in identity discovery. If this is true and the permission issue hasn't caused any troubles, maybe it's fine. #2820 mentioned some permission mismatch which I don't fully understand. For (1): I think there are two cases
Since former is more common, we probably should minimize their config overhead and default to the no-upload behavior if we can make it work. Once the user wants the latter behavior, they should be able to set this flag and have the (maybe an existing) controller use the upload behavior for new jobs. Relatedly we need to test the |
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, some minor nits.
sky/clouds/cloud.py
Outdated
raise NotImplementedError | ||
|
||
def in_cloud_list(self, cloud_list: Iterable['Cloud']) -> bool: |
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.
Indeed the boundary is subjective. Here I think we want to have a minimal cognitive load for readers that browse and learn about the interface. If that reader is a new cloud implementor or someone who wants to know "what does it take to implement a Cloud", I think it's best to leave out such one-liner utility funcs.
Co-authored-by: Zongheng Yang <[email protected]>
Co-authored-by: Zongheng Yang <[email protected]>
Co-authored-by: Zongheng Yang <[email protected]>
…pilot into avoid-upload-credential
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.
LGTM, thanks @Michaelvll!
@@ -40,4 +41,6 @@ | |||
'CLOUD_REGISTRY', | |||
'ProvisionerVersion', | |||
'StatusVersion', | |||
# Utility functions | |||
'cloud_in_list', |
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.
(In the future we can put utils in this init module too)
This is to avoid uploading credentials of the current cloud to be launched, as we can use the service account assigned to the instance to access the buckets and cloud APIs.
Fixes #2820
To enable this feature, a user should specify the following config in the
~/.sky/config.yaml
, and this only supports AWS and GCP for now.Tested (run the relevant ones):
bash format.sh
sky launch --cloud gcp --cpus 2 -i 0 echo hi
sky launch --cloud gcp --cpus 2 -i 0 echo hi
with the following config.yamlsky launch --cloud aws --cpus 2 -i 0 echo hi
with the following config.yamlsky launch --cloud gcp --cpus 2 -i 0 --num-nodes 2 echo hi
pytest tests/test_smoke.py --gcp
pytest tests/test_smoke.py --aws
pytest tests/test_smoke.py::test_fill_in_the_name
pytest tests/test_smoke.py::test_file_mounts --aws
with two private buckets (1 aws, 1 gcp) enabled inusing_file_mounts.yaml
.pytest tests/test_smoke.py::test_file_mounts --gcp
with two private buckets (1 aws, 1 gcp) enabled inusing_file_mounts.yaml
.bash tests/backward_comaptibility_tests.sh