Repository: skypilot-org/skypilot
Pull request: Added sky down --purge
Added `sky down --purge` which allows a user to remove a cluster from Sky ignoring any errors that the cloud provider throws (i.e ResourceNotFound). Useful for when the user manually plays around with resources or sky launch is called but terminated before any resources are created.

Problem statement:
UX: fail to `sky down` a cluster that was manually terminated in console
Daniel reported:

I accidentally terminated an instance in the EC2 web console that corresponded to a STOPPED cluster (I thought it was one of the rogue instances). Now I can't remove the cluster from the sky interface:
```
$ sky down vasp
Terminating 1 cluster: vasp. Proceed? [Y/n]: y
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] Failed to terminate vasp.
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] **** STDOUT ****
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] 
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] **** STDERR ****
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] 
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] An error occurred (InvalidParameterCombination) when calling the TerminateInstances operation: No instances specified
E 03-22 18:40:18 cloud_vm_ray_backend.py:2049] 
Terminating cluster vasp...failed. Please check the logs and try again.
```
should I just manually delete the corresponding row from the state.db? (also maybe a more helpful error message is warranted)

----
One possible option is to add `sky down --purge` to forcefully remove an entry on the user's behalf.

Hints from issue discussion:

Commit message: added sky down --purge

Patch to review:
diff --git a/.pylintrc b/.pylintrc
index 5ae679e73ff..24472ae6f58 100644
--- a/.pylintrc
+++ b/.pylintrc
@@ -210,7 +210,7 @@ const-rgx=^(_?[A-Z][A-Z0-9_]*|__[a-z0-9_]+__|_?[a-z][a-z0-9_]*)$
 attr-rgx=^_{0,2}[a-z][a-z0-9_]*$
 
 # Regular expression matching correct argument names
-argument-rgx=^[a-z][a-z0-9_]*$
+argument-rgx=^[a-z_][a-z0-9_]*$
 
 # Regular expression matching correct class attribute names
 class-attribute-rgx=^(_?[A-Z][A-Z0-9_]*|__[a-z0-9_]+__|_?[a-z][a-z0-9_]*)$
diff --git a/sky/backends/backend.py b/sky/backends/backend.py
index e873a9b28b8..ec71ce43c21 100644
--- a/sky/backends/backend.py
+++ b/sky/backends/backend.py
@@ -58,7 +58,11 @@ def post_execute(self, handle: ResourceHandle, teardown: bool) -> None:
     def teardown_ephemeral_storage(self, task: Task) -> None:
         raise NotImplementedError
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
+    def teardown(self,
+                 handle: ResourceHandle,
+                 terminate: bool,
+                 purge: bool = False,
+                 _force: bool = False) -> bool:
         raise NotImplementedError
 
     def register_info(self, **kwargs) -> None:
diff --git a/sky/backends/cloud_vm_ray_backend.py b/sky/backends/cloud_vm_ray_backend.py
index 53f88faaf44..814eed15941 100644
--- a/sky/backends/cloud_vm_ray_backend.py
+++ b/sky/backends/cloud_vm_ray_backend.py
@@ -1928,6 +1928,7 @@ def teardown_ephemeral_storage(self, task: Task) -> None:
     def teardown(self,
                  handle: ResourceHandle,
                  terminate: bool,
+                 purge: bool = False,
                  _force: bool = False) -> bool:
         cluster_name = handle.cluster_name
         lock_path = os.path.expanduser(_LOCK_FILENAME.format(cluster_name))
@@ -1936,14 +1937,14 @@ def teardown(self,
             # Should only be forced when teardown is called within a
             # locked section of the code (i.e teardown when not enough
             # resources can be provisioned)
-            return self._teardown(handle, terminate)
+            return self._teardown(handle, terminate, purge)
 
         try:
             # TODO(mraheja): remove pylint disabling when filelock
             # version updated
             # pylint: disable=abstract-class-instantiated
             with filelock.FileLock(lock_path, _FILELOCK_TIMEOUT_SECONDS):
-                success = self._teardown(handle, terminate)
+                success = self._teardown(handle, terminate, purge)
             if success and terminate:
                 os.remove(lock_path)
             return success
@@ -1952,7 +1953,8 @@ def teardown(self,
                          'Check to see if it is still being launched.')
         return False
 
-    def _teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
+    def _teardown(self, handle: ResourceHandle, terminate: bool,
+                  purge: bool) -> bool:
         log_path = os.path.join(os.path.expanduser(self.log_dir),
                                 'teardown.log')
         log_abs_path = os.path.abspath(log_path)
@@ -2052,9 +2054,11 @@ def _teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
                                  f'{tpu_stderr}{colorama.Style.RESET_ALL}')
                     return False
 
-        if returncode != 0:
+        if not purge and returncode != 0:
             logger.error(
-                f'{colorama.Fore.RED}Failed to terminate {cluster_name}.\n'
+                f'{colorama.Fore.RED}Failed to terminate {cluster_name}. '
+                f'If you want to ignore this error and remove the cluster '
+                f'from Sky anyways, use sky down --purge.\n'
                 f'**** STDOUT ****\n'
                 f'{stdout}\n'
                 f'**** STDERR ****\n'
diff --git a/sky/backends/local_docker_backend.py b/sky/backends/local_docker_backend.py
index 7fc275accac..3ac49cb7dee 100644
--- a/sky/backends/local_docker_backend.py
+++ b/sky/backends/local_docker_backend.py
@@ -345,7 +345,11 @@ def post_execute(self, handle: ResourceHandle, teardown: bool) -> None:
             f'task run command, run {style.BRIGHT}docker run -it '
             f'{container.image.tags[0]} /bin/bash{style.RESET_ALL}')
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
+    def teardown(self,
+                 handle: ResourceHandle,
+                 terminate: bool,
+                 purge: bool = False,
+                 _force: bool = False) -> bool:
         """Teardown kills the container."""
         if not terminate:
             logger.warning(
diff --git a/sky/cli.py b/sky/cli.py
index cd07af5e59c..fc1576d517f 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -1202,10 +1202,17 @@ def _filter(name, all_clusters):
               default=False,
               required=False,
               help='Skip confirmation prompt.')
+@click.option('--purge',
+              '-p',
+              is_flag=True,
+              default=False,
+              required=False,
+              help='Ignore cloud provider errors.')
 def down(
     clusters: Tuple[str],
     all: Optional[bool],  # pylint: disable=redefined-builtin
     yes: bool,
+    purge: bool,
 ):
     """Tear down cluster(s).
 
@@ -1243,11 +1250,15 @@ def down(
     _terminate_or_stop_clusters(clusters,
                                 apply_to_all=all,
                                 terminate=True,
-                                no_confirm=yes)
+                                no_confirm=yes,
+                                purge=purge)
 
 
-def _terminate_or_stop_clusters(names: Tuple[str], apply_to_all: Optional[bool],
-                                terminate: bool, no_confirm: bool) -> None:
+def _terminate_or_stop_clusters(names: Tuple[str],
+                                apply_to_all: Optional[bool],
+                                terminate: bool,
+                                no_confirm: bool,
+                                purge: bool = False) -> None:
     """Terminates or stops a cluster (or all clusters)."""
     command = 'down' if terminate else 'stop'
     if not names and apply_to_all is None:
@@ -1278,6 +1289,11 @@ def _terminate_or_stop_clusters(names: Tuple[str], apply_to_all: Optional[bool],
         return
 
     if not no_confirm:
+        if purge:
+            click.secho(
+                'WARNING: Purge will likely require you to manually delete '
+                'resources from your cloud provider.',
+                fg='yellow')
         teardown_verb = 'Terminating' if terminate else 'Stopping'
         cluster_str = 'clusters' if len(to_down) > 1 else 'cluster'
         cluster_list = ', '.join([r['name'] for r in to_down])
@@ -1302,7 +1318,7 @@ def _terminate_or_stop_clusters(names: Tuple[str], apply_to_all: Optional[bool],
             click.echo('  To terminate the cluster, run: ', nl=False)
             click.secho(f'sky down {name}', bold=True)
             continue
-        success = backend.teardown(handle, terminate=terminate)
+        success = backend.teardown(handle, terminate=terminate, purge=purge)
         operation = 'Terminating' if terminate else 'Stopping'
         if success:
             click.secho(f'{operation} cluster {name}...done.', fg='green')


Code review comments:
diff_hunk: @@ -2052,9 +2054,11 @@ def _teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
                                  f'{tpu_stderr}{colorama.Style.RESET_ALL}')
                     return False
 
-        if returncode != 0:
+        if not purge and returncode != 0:
             logger.error(
-                f'{colorama.Fore.RED}Failed to terminate {cluster_name}.\n'
+                f'{colorama.Fore.RED}Failed to terminate {cluster_name}. '
+                f'If you want to ignore this error and remove the cluster '
+                f'from Sky anyways, use sky down --purge.\n'
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
1. `{colorama.Style.RESET_ALL}` at the end.
2. Use backticks to surround `sky down --purge`?
3. "..and remove the cluster from Sky's status table, use..."

@author:
does another `{colorama.Style.RESET_ALL}` need to be added or is the one after the STERR good enough?

@user1:
I believe the latter is good; best to test it out?

diff_hunk: @@ -345,7 +345,11 @@ def post_execute(self, handle: ResourceHandle, teardown: bool) -> None:
             f'task run command, run {style.BRIGHT}docker run -it '
             f'{container.image.tags[0]} /bin/bash{style.RESET_ALL}')
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
+    def teardown(self,
+                 handle: ResourceHandle,
+                 terminate: bool,
+                 purge: bool = False,
+                 _force: bool = False) -> bool:
path: sky/backends/local_docker_backend.py
text: @user1:
```
del purge, _force  # Unused.
```
before L354?  See: https://google.github.io/styleguide/pyguide.html#214-decision

@author:
if we have to do this, should we delete it in `backend.py` too where it doesn't use any of the variables but raises an exception?

@user1:
In that place, since a NotImplementedError is raised, it is very clear it's not being used.  The intent of the above "del ... # Unused" pattern is to use it in an actual implemented function - in case anything in the body uses the vars.

diff_hunk: @@ -1928,6 +1928,7 @@ def teardown_ephemeral_storage(self, task: Task) -> None:
     def teardown(self,
                  handle: ResourceHandle,
                  terminate: bool,
+                 purge: bool = False,
                  _force: bool = False) -> bool:
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
What was the rationale of naming it `_force` rather than `force`?

@author:
i wanted to specify it as something internal instead of something that could be invoked by `sky down --force` but maybe that's not necessary

@user1:
Ah, I see, I missed this in the previous PR.  How about:
- Change https://github.com/sky-proj/sky/blob/885f9712343c3833166c951ab500669f058ca7c2/sky/backends/cloud_vm_ray_backend.py#L889-L891 the only caller for `force=True`, to directly call `._teardown(..., force=True)`.  We could add a pylint disable to allow calling an internal method.  Or, you can call it `CloudVmRayBackend().teardown_no_lock()` or the like.
- This way, we can remove the `force` arg from the public interface, `Backend.teardown()`. Agreed it is an internal detail that shouldn't be exposed.
- `force` should be renamed to `ignore_lock` or `no_locking` etc., to reflect the intent better.

diff_hunk: @@ -58,7 +58,11 @@ def post_execute(self, handle: ResourceHandle, teardown: bool) -> None:
     def teardown_ephemeral_storage(self, task: Task) -> None:
         raise NotImplementedError
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
+    def teardown(self,
+                 handle: ResourceHandle,
+                 terminate: bool,
+                 purge: bool = False,
+                 _force: bool = False) -> bool:
path: sky/backends/backend.py
text: @user1:
https://google.github.io/styleguide/pyguide.html#316-naming
`function_parameter_name` -> `force`?

diff_hunk: @@ -1278,6 +1289,11 @@ def _terminate_or_stop_clusters(names: Tuple[str], apply_to_all: Optional[bool],
         return
 
     if not no_confirm:
+        if purge:
+            click.secho(
+                'WARNING: Purge will likely require you to manually delete '
path: sky/cli.py
text: @user1:
The user may experiment by typing `sky down --purge clus` where clus is up and well.  In this case we will actually down the cluster, and it's surprising to see this warning.

```
» sky down --purge xxx2
WARNING: Purge will likely require you to manually delete resources from your cloud provider.
Terminating 1 cluster: xxx2. Proceed? [Y/n]:
Terminating cluster xxx2...done.
```

~~Not feeling strongly about keeping or removing this warning, though.  If keeping, "Purge" -> "--purge"?~~

Actually, how about removing this warning?  For the normal case like above, the user is left wondering if the cluster is actually down'd or not.

@author:
how about sending a warning only if there is a non-zero exit code while purging?