[Storage] Add storage translation for newly created buckets #3671
Conversation
There was a problem hiding this comment.
Thanks @romilbhardwaj for catching this! I found that it might make more sense to do the translation locally, instead of skipping the check on controller. See comments below. Wdyt?
sky/data/storage.py
Outdated
| @@ -325,6 +326,13 @@ def __deepcopy__(self, memo): | |||
|
|
|||
| def _validate_existing_bucket(self): | |||
| """Validates the storage fields for existing buckets.""" | |||
| if controller_utils.is_running_on_jobs_controller(): | |||
There was a problem hiding this comment.
Instead of skipping the check on the controller, we may want to translate those storage entries into external ones in controller_utils.maybe_translate_local_file_mounts_and_sync_up before submitting to the controller.
Reason: if the storage entry does not contain store field, it might be created on one cloud provider locally, but the controller may create another one on a different cloud provider:
file_mounts:
/output_path:
name: my-bucket
mode: MOUNT…o controller_new_storage_fix
romilbhardwaj
changed the title
|
Thanks @Michaelvll - updated the implementation to translate storage entries to externally sourced storages. Updated PR title and description too. Running tests. |
There was a problem hiding this comment.
Thanks for the update @romilbhardwaj! The PR looks mostly good to me. : )
| 'persistent': storage_obj.persistent, | ||
| 'mode': storage_lib.StorageMode.MOUNT.value, | ||
| }) | ||
| updated_mount_storages[storage_path] = new_storage |
There was a problem hiding this comment.
Do we need to set force_delete to True, so that when persistent=False, the jobs controller can delete the storage, even if it is considered as external storage on the controller?
| task.update_storage_mounts(updated_mount_storages) | ||
|
|
||
|
|
||
| def is_running_on_jobs_controller() -> bool: |
There was a problem hiding this comment.
Do we still need this movement? It seems the function is not used at places other than dashboard.py?
Closes #3666.
We skip the storage name validation because it is already done when the job is submitted from the client and the guardrails are not required for the controller.We now translate storage objects which create new buckets to become externally sourced buckets.
E.g.,
is translated to:
Also updated smoke tests to test for writing to an object store.
Tested (run the relevant ones):
pytest -v tests/test_smoke.py::test_managed_jobs_storage --kubernetespytest -v tests/test_smoke.py::test_managed_jobs_storage --awspytest -v tests/test_smoke.py::test_managed_jobs_storage --gcppytest tests/test_smoke.py::TestStorageWithCredentials