Repository: skypilot-org/skypilot
Pull request: Created and handled teardown success bool
Added a success variable to teardown (`sky down`) which has the following benefits:

1. Makes it more clear when `sky down` has failed from a user perspective by surfacing it as a red result

```
(sky-full) mraheja@latte:~/Sky/sky$ sky down sky-cpunode-mraheja
Terminating 1 cluster: sky-cpunode-mraheja. Proceed? [Y/n]: y
E 03-15 02:11:49 cloud_vm_ray_backend.py:1743] Cluster sky-cpunode-mraheja is locked by /home/mraheja/.sky/.sky-cpunode-mraheja.lock. Check to see if it is still being launched.
Terminating cluster sky-cpunode-mraheja...failed.
```

2. Prevents deletion of YAML file if `sky down` fails by early termination of `_teardown()` #570 
3. Similarly, prevents deletion of lock file if `sky down` fails
4. Similarly, prevents removal of cluster from cluster table if `sky down` fails





Problem statement:
Do not delete the ray yaml file if `sky down` fails
When I tried to `sky down` a GCP cluster, I was on a wrong conda environment that does not have `googleapiclient` installed. Though it print out the error messages, after I switched back to the correct environment and `sky down` again, it told me the ray yaml file does not exist. We may not remove the yaml file before the entry is removed from the cluster table.

Hints from issue discussion:
+1, encountered this too.  Best to remove the yaml only after the actual down/stop returns success (exit code == 0).  cc @mraheja

Commit message: Created and handled teardown success bool

Patch to review:
diff --git a/sky/backends/cloud_vm_ray_backend.py b/sky/backends/cloud_vm_ray_backend.py
index 193016ecfaf..a521e8720ed 100644
--- a/sky/backends/cloud_vm_ray_backend.py
+++ b/sky/backends/cloud_vm_ray_backend.py
@@ -1727,7 +1727,7 @@ def teardown_ephemeral_storage(self, task: Task) -> None:
                 if not storage.persistent:
                     storage.delete()
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
+    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
         cluster_name = handle.cluster_name
         lock_path = os.path.expanduser(_LOCK_FILENAME.format(cluster_name))
         try:
@@ -1735,14 +1735,16 @@ def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
             # version updated
             # pylint: disable=abstract-class-instantiated
             with filelock.FileLock(lock_path, 10):
-                self._teardown(handle, terminate)
-            if terminate:
+                success = self._teardown(handle, terminate)
+            if success and terminate:
                 os.remove(lock_path)
+            return success
         except filelock.Timeout:
-            logger.error(f'Cluster {cluster_name} is locked by {lock_path}. \
-                    Check to see if it is still being launched.')
+            logger.error(f'Cluster {cluster_name} is locked by {lock_path}. ' \
+                'Check to see if it is still being launched.')
+            return False
 
-    def _teardown(self, handle: ResourceHandle, terminate: bool) -> None:
+    def _teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
         log_path = os.path.join(os.path.expanduser(self.log_dir),
                                 'teardown.log')
         log_abs_path = os.path.abspath(log_path)
@@ -1840,6 +1842,7 @@ def _teardown(self, handle: ResourceHandle, terminate: bool) -> None:
                                  f'{tpu_stdout}\n'
                                  f'**** STDERR ****\n'
                                  f'{tpu_stderr}{colorama.Style.RESET_ALL}')
+                    return False
 
         if returncode != 0:
             logger.error(
@@ -1848,6 +1851,7 @@ def _teardown(self, handle: ResourceHandle, terminate: bool) -> None:
                 f'{stdout}\n'
                 f'**** STDERR ****\n'
                 f'{stderr}{colorama.Style.RESET_ALL}')
+            return False
 
         auth_config = backend_utils.read_yaml(handle.cluster_yaml)['auth']
         _remove_cluster_from_ssh_config(handle.head_ip, auth_config)
diff --git a/sky/cli.py b/sky/cli.py
index e9fa9a02359..3e0618acd85 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -1203,9 +1203,12 @@ 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
-        backend.teardown(handle, terminate=terminate)
+        success = backend.teardown(handle, terminate=terminate)
         if terminate:
-            click.secho(f'Terminating cluster {name}...done.', fg='green')
+            if success:
+                click.secho(f'Terminating cluster {name}...done.', fg='green')
+            else:
+                click.secho(f'Terminating cluster {name}...failed.', fg='red')
         else:
             click.secho(f'Stopping cluster {name}...done.', fg='green')
             click.echo('  To restart the cluster, run: ', nl=False)


Code review comments:
diff_hunk: @@ -1203,9 +1203,12 @@ 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
-        backend.teardown(handle, terminate=terminate)
+        success = backend.teardown(handle, terminate=terminate)
         if terminate:
-            click.secho(f'Terminating cluster {name}...done.', fg='green')
+            if success:
+                click.secho(f'Terminating cluster {name}...done.', fg='green')
+            else:
+                click.secho(f'Terminating cluster {name}...failed.', fg='red')
path: sky/cli.py
text: @user1:
L1211: Suggest a fix.  "...failed. Please check the logs and try again."

diff_hunk: @@ -1727,22 +1727,24 @@ def teardown_ephemeral_storage(self, task: Task) -> None:
                 if not storage.persistent:
                     storage.delete()
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
+    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
         cluster_name = handle.cluster_name
         lock_path = os.path.expanduser(_LOCK_FILENAME.format(cluster_name))
         try:
             # TODO(mraheja): remove pylint disabling when filelock
             # version updated
             # pylint: disable=abstract-class-instantiated
             with filelock.FileLock(lock_path, 10):
-                self._teardown(handle, terminate)
-            if terminate:
+                success = self._teardown(handle, terminate)
+            if success and terminate:
                 os.remove(lock_path)
+            return success
         except filelock.Timeout:
-            logger.error(f'Cluster {cluster_name} is locked by {lock_path}. \
-                    Check to see if it is still being launched.')
+            logger.error(f'Cluster {cluster_name} is locked by {lock_path}. ' \
+                'Check to see if it is still being launched.')
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
Nit: no need for the ending `\`.  Use implicit join: https://google.github.io/styleguide/pyguide.html#32-line-length

diff_hunk: @@ -1727,22 +1727,24 @@ def teardown_ephemeral_storage(self, task: Task) -> None:
                 if not storage.persistent:
                     storage.delete()
 
-    def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
+    def teardown(self, handle: ResourceHandle, terminate: bool) -> bool:
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
As this is an interface change, change the respective return types in  `backend.Backend` and `LocalDockerBackend` too.

diff_hunk: @@ -1203,9 +1203,12 @@ 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
-        backend.teardown(handle, terminate=terminate)
+        success = backend.teardown(handle, terminate=terminate)
         if terminate:
-            click.secho(f'Terminating cluster {name}...done.', fg='green')
+            if success:
+                click.secho(f'Terminating cluster {name}...done.', fg='green')
+            else:
+                click.secho(f'Terminating cluster {name}...failed.', fg='red')
path: sky/cli.py
text: @user1:
The following stopping does not have output changes. How about after L1206 we have `status_str = 'done' if success else 'failed'`, and here (also L1213) `f'... cluster {name}...{status_str}'`.