[Storage] Continue storage deletion in `sky storage delete` when some fail #4454

andylizf · 2024-12-10T21:45:02Z

Make sky storage delete continue deleting remaining storages when some fail, instead of aborting the entire operation. Failed deletions are reported individually.

We Implemented this feature via adding a continue_on_error parameter in run_in_parallel

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

andylizf · 2024-12-10T21:45:55Z

@cblmemo PTAL, thanks!

andylizf · 2024-12-10T22:55:39Z

I inserted five new records into the 'global_user_state' database: three records containing the previous storage handle value, and two records with null handle values.

andyl@DESKTOP-7FP6SMO ~/skypilot (fix-storage-recover) [1]> sky storage dele
te -a
Deleting 6 storages: skypilot-workdir-andyl-74d418df, test-storage-1, test-storage-2, test-storage-3, test-storage-4, test-storage-5. Proceed? [Y/n]: 
Error deleting storage test-storage-3: In global_user_state, storage name 'test-storage-3' does not match handle.storage_name 'skypilot-workdir-andyl-577894a6'
Error deleting storage test-storage-2: In global_user_state, storage name 'test-storage-2' does not match handle.storage_name 'skypilot-workdir-andyl-577894a6'
Error deleting storage test-storage-4: Storage name 'test-storage-4' not found.
Error deleting storage test-storage-1: In global_user_state, storage name 'test-storage-1' does not match handle.storage_name 'skypilot-workdir-andyl-577894a6'
Error deleting storage test-storage-5: Storage name 'test-storage-5' not found.
Deleted GCS bucket skypilot-workdir-andyl-74d418df.

cblmemo

Thanks for the prompt fix @andylizf ! left some discussions.

sky/cli.py

sky/global_user_state.py

cblmemo · 2024-12-11T17:41:37Z

sky/utils/subprocess_utils.py

    """
-    # Reference: https://stackoverflow.com/questions/25790279/python-multiprocessing-early-termination # pylint: disable=line-too-long


why not removing this reference

You mean why remove the reference? I think it's trivial now, and the reference doesn't add much value to the code.

cblmemo · 2024-12-11T17:42:55Z

sky/utils/subprocess_utils.py

 def run_in_parallel(func: Callable,
                    args: Iterable[Any],
-                    num_threads: Optional[int] = None) -> List[Any]:
+                    num_threads: Optional[int] = None,
+                    continue_on_error: bool = False) -> List[Any]:


adding this seems change the return value type from Value to Union[Value, Exception], which looks a little bit strange to me. Why do we need this anyway? aren't we already add the try-except in the func?

You are right. I thought about not adding a function to catch and log these exceptions, but instead handle all exceptions in run_in_parallel. Anyway, it made sense to have this function have an argument like continue_on_error.
But then I realized it violated the abstraction concept and would make it harder to print custom logs. So I ended up wrapping the function and logging it.

Co-authored-by: Tian Xia <[email protected]>

cblmemo

Thanks @andylizf ! Mostly looks good to me. After those nits it should be ready to go!

sky/cli.py

cblmemo · 2024-12-19T16:34:27Z

sky/cli.py

+        except (exceptions.StorageBucketDeleteError, PermissionError) as e:
+            click.secho(f'Error deleting storage {name}: {e}', fg='red')
+        except ValueError as e:
+            click.secho(f'Error deleting storage {name}: {e}', fg='red')


Suggested change

except (exceptions.StorageBucketDeleteError, PermissionError) as e:

click.secho(f'Error deleting storage {name}: {e}', fg='red')

except ValueError as e:

click.secho(f'Error deleting storage {name}: {e}', fg='red')

except (exceptions.StorageBucketDeleteError, PermissionError, ValueError) as e:

click.secho(f'Error deleting storage {name}: {e}', fg='red')

can we merge them?

Actually, seems like here is one exception that is not being caught. After a second thought, for simplicity, we can probably just do broad except here..

(skypilot) (base) ➜ skypilot git:(remove_token) ✗ sky storage delete -a Deleting 15 storages: skypilot-workdir-windsey-afa32d67, skypilot-workdir-windsey-81d2115f, skypilot-workdir-windsey-b8fd0382, skypilot-workdir-windsey-499178bb, skypilot-workdir-windsey-dd64cc00, skypilot-workdir-windsey-d04e17a8, skypilot-workdir-windsey-43925bf5, skypilot-workdir-windsey-3375424a, skypilot-workdir-windsey-5e824f17, skypilot-workdir-windsey-db755382, skypilot-workdir-windsey-25270ec6, skypilot-workdir-windsey-1cb4689a, skypilot-workdir-windsey-f40df433, skypilot-workdir-windsey-2ef8a0bf, skypilot-workdir-windsey-58878e24. Proceed? [Y/n]: No backing stores found. Deleting storage. botocore.exceptions.ClientError: An error occurred (403) when calling the HeadBucket operation: Forbidden The above exception was the direct cause of the following exception: sky.exceptions.StorageBucketGetError: Failed to access existing bucket 'skypilot-workdir-windsey-afa32d67'. This is likely because it is a private bucket you do not have access to. To fix: 1. If you are trying to create a new bucket: use a different name. 2. If you are trying to connect to an existing bucket: make sure your cloud credentials have access to it. To debug, consider running `aws s3 ls skypilot-workdir-windsey-afa32d67`.

Yeah... I agree

cblmemo · 2024-12-19T16:35:52Z

sky/cli.py

+        except Exception as e:  # pylint: disable=broad-except
+            with ux_utils.print_exception_no_traceback():
+                raise e
+


Suggested change

except Exception as e: # pylint: disable=broad-except

with ux_utils.print_exception_no_traceback():

raise e

It seems like we dont need it? We can simply not except it. And any exceptions raised by sky.storage_delete should already have the surpress traceback context?

Why they should have the traceback context supressed?

Cuz this is a user-facing API and we want to keep the error message simple. The content in the error message should be self-explanatory.

Co-authored-by: Tian Xia <[email protected]>

cblmemo

Thanks @andylizf ! LGTM. Please lmk when you pass the test and it should be ready to go!

feat: support storage deletion recovery

b9e81d1

fix: check handle None later and add storage name consistency check

9f87404

cblmemo reviewed Dec 11, 2024

View reviewed changes

andylizf and others added 5 commits December 11, 2024 11:47

Apply suggestions from code review

4fd28bb

Co-authored-by: Tian Xia <[email protected]>

refactor: use storage_name=* to replace a trivial function

36ca3a3

refactor: fine-grained catch

8b7b075

revert: run_in_parallel continue_on_errors

fa6037f

chore: remove TODO

1d67da7

cblmemo reviewed Dec 19, 2024

View reviewed changes

cblmemo changed the title ~~Continue storage deletion when some fail~~ [Storage] Continue storage deletion in sky storage delete when some fail Dec 19, 2024

andylizf and others added 2 commits December 19, 2024 22:05

style: comment on why *

cee19ea

Co-authored-by: Tian Xia <[email protected]>

revert: use broad-except

47d01fa

cblmemo approved these changes Dec 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Storage] Continue storage deletion in `sky storage delete` when some fail #4454

[Storage] Continue storage deletion in `sky storage delete` when some fail #4454

andylizf commented Dec 10, 2024

andylizf commented Dec 10, 2024

andylizf commented Dec 10, 2024 •

edited

Loading

cblmemo left a comment

cblmemo Dec 11, 2024

andylizf Dec 11, 2024

cblmemo Dec 11, 2024

andylizf Dec 11, 2024

cblmemo left a comment

cblmemo Dec 19, 2024

cblmemo Dec 19, 2024

andylizf Dec 20, 2024

cblmemo Dec 19, 2024

andylizf Dec 20, 2024

cblmemo Dec 21, 2024

cblmemo left a comment

		"""
		# Reference: https://stackoverflow.com/questions/25790279/python-multiprocessing-early-termination # pylint: disable=line-too-long

	except Exception as e: # pylint: disable=broad-except
	with ux_utils.print_exception_no_traceback():
	raise e

[Storage] Continue storage deletion in sky storage delete when some fail #4454

Are you sure you want to change the base?

[Storage] Continue storage deletion in sky storage delete when some fail #4454

Conversation

andylizf commented Dec 10, 2024

andylizf commented Dec 10, 2024

andylizf commented Dec 10, 2024 • edited Loading

cblmemo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblmemo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblmemo left a comment

Choose a reason for hiding this comment

[Storage] Continue storage deletion in `sky storage delete` when some fail #4454

[Storage] Continue storage deletion in `sky storage delete` when some fail #4454

andylizf commented Dec 10, 2024 •

edited

Loading