[Storage] Fix sky storage delete for externally deleted buckets

[romilbhardwaj]

Closes #1322.

If a bucket is deleted externally, then store.delete() now does a no-op (instead of creating and deleting like before). The logging has been updated.

Now:

(base) ➜  repro git:(storage_deletionfix) ✗ SKYPILOT_DEBUG=0 sky storage delete -a
Deleting all storage objects.
# Shows spinner Deleting S3 bucket romilmybucket.
I 04-16 22:47:19 storage.py:1029] S3 bucket romilmybucket may have been deleted externally. Removing from local state.

Tested (run the relevant ones):

• Snippet from [Storage] Confusing logging in sky storage delete #1322
• Storage smoke tests: pytest tests/test_smoke.py::TestStorageWithCredentials and with --cloudflare.
• Backward compatibility testing by creating buckets in master, upgrading, then trying deletion and launching with existing buckets.

[romilbhardwaj]

Thanks for the quick review @landscapepainter! Ready for another look.

More reviews needed

[landscapepainter]

tested more complicated scenarios and confirmed the log as expected:

1. create 3 of s3 buckets and 2 of gcs buckets. Remove them all externally and remove them with sky.
2. create 3 of r2 buckets, remove two of them externally, and remove them all with sky.

[landscapepainter]

Checking if the bucket was externally removed successfully with assert should be more complete. Perhaps something like,

Suggested change

	subprocess.check_output(cmd)
	out = subprocess.check_output(cmd)
	if isinstance(store_type, storage_lib.StoreType.S3) or isinstance(store_type, storage_lib.StoreType.R2):
	assert 'remove_bucket' not in out.decode('utf-8').lower()
	elif isinstance(store_type, storage_lib.StoreType.GCS):
	assert 'removing' not in out.decode('utf-8').lower()

[romilbhardwaj]

If the removal fails, the awscli and gsutil return a non-zero error code (In which case we'd get a CalledProcessError, which would catch it).

[landscapepainter]

Perhaps leaving the same comment as L1635

[romilbhardwaj]

Answered in the other comment

[landscapepainter]

Adding Same comment as L1635

[romilbhardwaj]

Answered in the other comment

[landscapepainter]

As we have L1026, can we remove this message, applying the same to gcs and r2?

[romilbhardwaj]

Ah, this was intentionally left in as logger.debug (which is not shown to user, unlike logger.info). It should be fine to have verbose logging in debug.

[landscapepainter]

Ready to go!