Skip to content
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

Storage Delete for GCP >= 256 Objects #888

Merged
merged 4 commits into from
Jun 6, 2022
Merged

Conversation

michaelzhiluo
Copy link
Collaborator

@michaelzhiluo michaelzhiluo commented Jun 2, 2022

Fixes #724. Now, GCP storage code will detect if the bucket has >=256 objects. If there are more than 256 objects, then it will delete the contents in the bucket first.

Tests

touch ~/Downloads/hallo/{0001..0258}.c

YAML file

file_mounts:
  /checkpoints:
    name: michaels-new-bucket-1
    source: ~/Downloads/hallo
    store: gcs
    mode: COPY

sky launch -c hallo storage.yaml && sky storage delete michaels-new-bucket-1

Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Some comments:

sky/data/storage.py Outdated Show resolved Hide resolved
sky/data/storage.py Outdated Show resolved Hide resolved
sky/data/storage.py Outdated Show resolved Hide resolved
sky/data/storage.py Outdated Show resolved Hide resolved
logger.info(str_line)
retcode = process.wait()
if retcode != 0:
raise exceptions.StorageBucketDeleteError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is defined in #888. Maybe we should merge this PR after that or rebase this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the code base reduction, its kind of hard to rebase and merge different branches. For simplicity, I think let's just merge this PR after #888!

@michaelzhiluo
Copy link
Collaborator Author

Thanks Romil for the review! PTAL

Copy link
Collaborator

@romilbhardwaj romilbhardwaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for fixing this!

@michaelzhiluo michaelzhiluo merged commit 40ba476 into master Jun 6, 2022
suquark pushed a commit that referenced this pull request Jun 7, 2022
* Fix Weilin's bug

* Refactor os.system to subprocess

* Addressed romil's comments
suquark pushed a commit that referenced this pull request Jun 7, 2022
* Fix Weilin's bug

* Refactor os.system to subprocess

* Addressed romil's comments
@Michaelvll Michaelvll deleted the storage_gcp_delete branch December 18, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sky storage delete failed on GCS bucket with more than 256 objects
2 participants