[Storage] Support for storage info in cluster table's metadata #2322

landscapepainter · 2023-07-28T23:40:24Z

This PR resolves #1203.

Currently, when we run sky stop <cluster-name> on a cluster that has cloud storage mounted, the mounting will be lost when restarted with sky start <cluster-name> as the mounting command does not get ran.

In this PR, I save the storage_mounts dictionary as a metadata in cluster table's storage_mounts_metadata when sky launch is initially ran. Then, this is loaded to be used to re-mount when sky start <cluster-name> is ran.

Tested (run the relevant ones):

merge master

merge with master

landscapepainter

This is now ready for a review.

romilbhardwaj

Thanks @landscapepainter! Took a high level look at the code. Haven't tested yet.

romilbhardwaj · 2023-08-12T16:34:31Z

sky/backends/cloud_vm_ray_backend.py

+        for _, storage_obj in storage_mounts.items():
+            for _, store_obj in storage_obj.stores.items():
+                # Update some of the non-picklabe attributes of store instances
+                store_obj.make_picklable()


Can we use and store StorageMetadata here? It was designed to be a picklable representation of Storage objects.

Thanks for pointing this out!

romilbhardwaj · 2023-08-12T16:45:40Z

sky/backends/cloud_vm_ray_backend.py

+            return
+
+        cluster_name = handle.cluster_name
+        cluster_metadata = global_user_state.get_cluster_metadata(cluster_name)


I'm not sure what counts as metadata for a cluster. In other words, should storage objects be instead stored as a part of the handle? Two reasons for this:

get_cluster_metadata and set_cluster_metadata appear to be unused before this... @Michaelvll - was there a specific use case for these methods?

handle seems to store all metadata for a cluster (e.g., name, resources etc.)

To add on, the current implementation I have, it stores storage_mounts dictionary, which is dst[str] : Storage obj. pair, in cluster's metadata. This is useful as I can pass it into sync_file_mounts api just like we run it on sky launch. We need to consider a way to store dst and keep it as a pair with Storage obj. if we want to store only Storage object in handle.

sky/core.py

merge

landscapepainter · 2023-10-02T05:55:03Z

@romilbhardwaj This is ready for a look. Did some additional backwards compatibility tests as well.

state.db created with sky launch in master branch, sky stop/start ran from PR branch
state.dbcreated with sky launch in master branch, sky stop in master branch, sky start in PR branch

romilbhardwaj · 2023-10-02T19:55:30Z

Additionally, can you also run test_smoke.py::test_file_mounts?

landscapepainter · 2023-10-03T17:58:21Z

@romilbhardwaj This is ready for another look. Confirmed pytest tests/test_smoke.py::test_file_mounts passes.

landscapepainter · 2023-10-10T05:42:59Z

@romilbhardwaj This is ready for a look. Additional testing done for sky storage delete and sky start after implementing error raise from _get_bucket when the bucket is externally deleted while sync_on_reconstruction is set to False.

Running sky storage delete where storage has 3 stores and 1 of those are externally deleted
Running sky start with a cluster that has a bucket mounted and deleted externally

Following is a result from running sky start after externally deleting the mounting bucket.

$ sky start doyoung-test -y
I 10-10 05:34:43 cloud_vm_ray_backend.py:1449] To view detailed progress: tail -n100 -f /home/gcpuser/sky_logs/sky-2023-10-10-05-34-43-492829/provision.log
I 10-10 05:34:43 cloud_vm_ray_backend.py:1277] Cluster 'doyoung-test' (status: STOPPED) was previously launched in GCP us-central1. Relaunching in that region.
I 10-10 05:34:47 cloud_vm_ray_backend.py:1887] Launching on GCP us-central1 (us-central1-a)
I 10-10 05:35:21 log_utils.py:45] Head node is up.
I 10-10 05:35:58 cloud_vm_ray_backend.py:1692] Successfully provisioned or found existing VM.
I 10-10 05:36:07 cloud_vm_ray_backend.py:4587] Processing 1 storage mount.
sky.exceptions.StorageExternalDeletionError: The bucket, 'gcs-mount-testing', could not be mounted on cluster 'doyoung-test'. Please verify that the bucket exists. The cluster started successfully without mounting the bucket.

romilbhardwaj

Thanks @landscapepainter! Left some minor comments, should be good to go after they are fixed. Please also run file mounting related smoke tests and some manual backward compatibility tests to make sure the updates in global_user_state don't break for existing storages/clusters.

sky/data/storage.py

sky/exceptions.py

sky/data/storage.py

tests/test_smoke.py

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

romilbhardwaj

Thanks @landscapepainter! Left one comment - should be good to go if all smoke tests and manual backward compatibility tests pass.

sky/data/storage.py

romilbhardwaj

Ran into an issue with backward compatibility:

On master branch, sky launch -c test task.yaml --cloud gcp this YAML:

file_mounts:
  /nfs:
    name: romil-test-bucket
    store: s3
    mode: MOUNT

run: |
  ls /nfs

Switch to this branch, run sky launch -c test task.yaml --cloud gcp again. (this step may not be required, but I ran it)
Stop cluster with sky stop test
sky start test fails with:

I 11-07 17:41:15 cloud_vm_ray_backend.py:1692] Successfully provisioned or found existing VM.
D 11-07 17:41:15 cloud_vm_ray_backend.py:3001] Checking if skylet is running on the head node.
Traceback (most recent call last):
  File "/Users/romilb/tools/anaconda3/bin/sky", line 8, in <module>
    sys.exit(cli())
  File "/Users/romilb/tools/anaconda3/lib/python3.9/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/Users/romilb/tools/anaconda3/lib/python3.9/site-packages/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/utils/common_utils.py", line 311, in _record
    return f(*args, **kwargs)
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/cli.py", line 1162, in invoke
    return super().invoke(ctx)
  File "/Users/romilb/tools/anaconda3/lib/python3.9/site-packages/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/romilb/tools/anaconda3/lib/python3.9/site-packages/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/romilb/tools/anaconda3/lib/python3.9/site-packages/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/utils/common_utils.py", line 332, in _record
    return f(*args, **kwargs)
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/cli.py", line 2573, in start
    core.start(name,
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/utils/common_utils.py", line 332, in _record
    return f(*args, **kwargs)
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/core.py", line 278, in start
    _start(cluster_name,
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/core.py", line 211, in _start
    storage_mounts = backend.get_storage_mounts_metadata(handle.cluster_name)
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/backends/cloud_vm_ray_backend.py", line 4708, in get_storage_mounts_metadata
    storage_mounts[dst] = storage_lib.Storage.from_metadata(
  File "/Users/romilb/Romil/Berkeley/Research/sky-experiments/sky/data/storage.py", line 760, in from_metadata
    mode=override_args.get('mode', metadata.mode))
AttributeError: 'StorageMetadata' object has no attribute 'mode'

landscapepainter · 2023-11-08T06:07:58Z

@romilbhardwaj Re-tested on all the following cases:

Relevant individual smoke tests: pytest tests/test_smoke.py::TestStorageWithCredentials
Relevant individual smoke tests: pytest tests/test_smoke.py::test_file_mounts

Below is the backwards compatibility test you commented about. Confirmed that it passes now. Thanks for the catch.

state.db created with sky launch in master branch, sky stop/start ran from PR branch
state.dbcreated with sky launch in master branch, sky stop in master branch, sky start in PR branch

edge cases

Running sky storage delete where storage has 3 stores and 1 of those are externally deleted
Running sky start with a cluster that has a bucket mounted and deleted externally

Newly added smoke tests:

Relevant individual smoke tests: pytest tests/test_smoke.py::test_aws_storage_mounts_with_stop --aws
Relevant individual smoke tests: pytest tests/test_smoke.py::test_gcp_storage_mounts_with_stop --gcp
Relevant individual smoke tests: pytest tests/test_smoke.py::test_kubernetes_storage_mounts_with_stop --kubernetes

romilbhardwaj

Thanks for the extensive tests @landscapepainter! Should be good to go now.

add storage in cluster's metadata

d1dba9f

landscapepainter marked this pull request as draft July 28, 2023 23:40

landscapepainter added 9 commits August 1, 2023 05:34

add functionality to run mounting with sky start

5b5dc56

nit

b986be6

Merge branch 'master' into cluster-storage-metadata

0eb6954

merge master

update cluster metadata as pickle

f30d3a6

format

f24b524

nit

a843be3

format

59181ae

format

d68c0fa

Merge branch 'master' into cluster-storage-metadata

c70d909

merge with master

landscapepainter marked this pull request as ready for review August 12, 2023 05:26

landscapepainter commented Aug 12, 2023

View reviewed changes

landscapepainter requested review from concretevitamin, Michaelvll and romilbhardwaj August 12, 2023 05:40

romilbhardwaj reviewed Aug 12, 2023

View reviewed changes

landscapepainter added 3 commits August 22, 2023 08:01

update metadata usage

554b118

format

d8687c9

nit

8894a52

landscapepainter mentioned this pull request Sep 1, 2023

[Storage] Adding new storage mode CSYNC #2336

Open

14 tasks

romilbhardwaj self-requested a review September 28, 2023 00:20

landscapepainter added 6 commits September 29, 2023 04:25

update from CSYNC PR

497d39b

Merge branch 'master' into cluster-storage-metadata

b8e5d1b

merge

update add_or_update_cluster()

447392b

nit

5014b8f

nit

2437718

nti

ce3e124

nit

485f8d1

landscapepainter added 2 commits October 3, 2023 14:59

fix

4199c9e

raise error for when bucket is None

e7ea128

landscapepainter added 7 commits October 8, 2023 23:45

testing images

079caff

nit

a49241a

nit

8482144

Merge branch 'master' into cluster-storage-metadata

93c7b7f

fix externally deleted storage edge case

024cf17

nit

542604f

lint

9ed3006

landscapepainter added 3 commits October 10, 2023 05:45

err msg update

47ee781

nit

d15d304

nit

936bec1

romilbhardwaj reviewed Oct 26, 2023

View reviewed changes

landscapepainter and others added 4 commits November 6, 2023 21:16

Update sky/exceptions.py

4052f8d

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@gmail.com>

resolve comments

c029010

update Storage.from_metadata for valid format

d03487c

update smoke test

8334105

romilbhardwaj approved these changes Nov 8, 2023

View reviewed changes

sky/data/storage.py Show resolved Hide resolved

sky/data/storage.py Show resolved Hide resolved

sky/data/storage.py Show resolved Hide resolved

nit

701c8bb

romilbhardwaj requested changes Nov 8, 2023

View reviewed changes

backward compatibility and check list/cloud source

16efcd7

romilbhardwaj approved these changes Nov 8, 2023

View reviewed changes

landscapepainter merged commit 4c4b7bd into skypilot-org:master Nov 8, 2023
18 checks passed

Michaelvll mentioned this pull request Nov 12, 2023

Smoke Test fails on master #2773

Closed

This was referenced Nov 18, 2023

[Storage] Implement storage info. into cluster's metadata to enhance storage/cluster interaction #2278

Closed

[Spot] Auto-translated bucket leakage if the spot job is not submitted correctly #1225

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage] Support for storage info in cluster table's metadata #2322

[Storage] Support for storage info in cluster table's metadata #2322

landscapepainter commented Jul 28, 2023 •

edited

landscapepainter left a comment

romilbhardwaj left a comment

romilbhardwaj Aug 12, 2023

landscapepainter Aug 12, 2023

romilbhardwaj Aug 12, 2023

landscapepainter Aug 13, 2023 •

edited

landscapepainter commented Oct 2, 2023 •

edited

romilbhardwaj commented Oct 2, 2023

landscapepainter commented Oct 3, 2023

landscapepainter commented Oct 10, 2023 •

edited

romilbhardwaj left a comment

romilbhardwaj left a comment

romilbhardwaj left a comment

landscapepainter commented Nov 8, 2023

romilbhardwaj left a comment

[Storage] Support for storage info in cluster table's metadata #2322

[Storage] Support for storage info in cluster table's metadata #2322

Conversation

landscapepainter commented Jul 28, 2023 • edited

landscapepainter left a comment

Choose a reason for hiding this comment

romilbhardwaj left a comment

Choose a reason for hiding this comment

romilbhardwaj Aug 12, 2023

Choose a reason for hiding this comment

landscapepainter Aug 12, 2023

Choose a reason for hiding this comment

romilbhardwaj Aug 12, 2023

Choose a reason for hiding this comment

landscapepainter Aug 13, 2023 • edited

Choose a reason for hiding this comment

landscapepainter commented Oct 2, 2023 • edited

romilbhardwaj commented Oct 2, 2023

landscapepainter commented Oct 3, 2023

landscapepainter commented Oct 10, 2023 • edited

romilbhardwaj left a comment

Choose a reason for hiding this comment

romilbhardwaj left a comment

Choose a reason for hiding this comment

romilbhardwaj left a comment

Choose a reason for hiding this comment

landscapepainter commented Nov 8, 2023

romilbhardwaj left a comment

Choose a reason for hiding this comment

landscapepainter commented Jul 28, 2023 •

edited

landscapepainter Aug 13, 2023 •

edited

landscapepainter commented Oct 2, 2023 •

edited

landscapepainter commented Oct 10, 2023 •

edited