Repository: skypilot-org/skypilot
Pull request: [Spot] Disallow `sky down` spot controller when in-progress spot jobs exist
<!-- Describe the changes in this PR -->

Fixes #1666

With this PR the following message will show if `sky down` the spot controller with in-progress spot jobs:
<img width="799" alt="image" src="https://user-images.githubusercontent.com/6753189/216758225-2654732a-ea1d-4800-8b84-1e5fa2927bcc.png">


Tested:
- [x] `sky down sky-spot-controller-<hash>` with in-progress spot jobs

Problem statement:
[Spot] Disallow terminating the spot cluster when there is spot jobs running
Requested by user:

We should ban the termination of the spot controller (not even showing the prompt) when there is spot job running to avoid spot resources leakage.

We should also make the termination of the spot controller harder, by asking the user to type `I understand` etc instead of simply a `y` for terminating the controller.

Hints from issue discussion:

Commit message: address comments

Patch to review:
diff --git a/sky/cli.py b/sky/cli.py
index 10288245c55..8d5c97fabfe 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -2200,7 +2200,7 @@ def down(
                            purge=purge)
 
 
-def _hint_for_down_spot_controller(controller_name: str):
+def _hint_or_raise_for_down_spot_controller(controller_name: str):
     # spot_jobs will be empty when the spot cluster is not running.
     cluster_status, _ = backend_utils.refresh_cluster_status_handle(
         controller_name)
@@ -2208,22 +2208,33 @@ def _hint_for_down_spot_controller(controller_name: str):
         click.echo('Managed spot controller has already been torn down.')
         return
 
+    if cluster_status == global_user_state.ClusterStatus.INIT:
+        with ux_utils.print_exception_no_traceback():
+            raise exceptions.NotSupportedError(
+                'Tearing down the spot controller while it is in INIT state is '
+                'not supported, as we cannot guarantee that all the spot jobs '
+                'are finished. Please wait until the spot controller is UP '
+                f'or fix it with {colorama.Style.BRIGHT}sky start '
+                f'{spot_lib.SPOT_CONTROLLER_NAME}{colorama.Style.RESET_ALL}.')
     msg = (f'{colorama.Fore.YELLOW}WARNING: Tearing down the managed '
            f'spot controller ({cluster_status.value}). Please be '
            f'aware of the following:{colorama.Style.RESET_ALL}'
            '\n * All logs and status information of the spot '
            'jobs (output of `sky spot queue`) will be lost.')
+    click.echo(msg)
     if cluster_status == global_user_state.ClusterStatus.UP:
-        try:
-            spot_jobs = core.spot_queue(refresh=False)
-        except exceptions.ClusterNotUpError:
-            # The spot controller cluster status changed during querying
-            # the spot jobs, use the latest cluster status, so that the
-            # message for INIT and STOPPED states will be correctly
-            # added to the message.
-            cluster_status = backend_utils.refresh_cluster_status_handle(
-                controller_name)
-            spot_jobs = []
+        with backend_utils.safe_console_status(
+                '[bold cyan]Checking spot jobs on the controller[/]'):
+            try:
+                spot_jobs = core.spot_queue(refresh=False)
+            except exceptions.ClusterNotUpError:
+                # The spot controller cluster status changed during querying
+                # the spot jobs, use the latest cluster status, so that the
+                # message for INIT and STOPPED states will be correctly
+                # added to the message.
+                cluster_status = backend_utils.refresh_cluster_status_handle(
+                    controller_name)
+                spot_jobs = []
 
         # Find in-progress spot jobs, and hint users to cancel them.
         non_terminal_jobs = [
@@ -2231,17 +2242,21 @@ def _hint_for_down_spot_controller(controller_name: str):
         ]
         if (cluster_status == global_user_state.ClusterStatus.UP and
                 non_terminal_jobs):
-            msg += ('\n * In-progress spot jobs found, their resources '
-                    'will not be terminated and require manual cleanup:\n')
             job_table = spot_lib.format_job_table(non_terminal_jobs,
                                                   show_all=False)
+            msg = (f'{colorama.Fore.RED}In-progress spot jobs found. '
+                   'To avoid resource leakage, cancel all jobs first: '
+                   f'{colorama.Style.BRIGHT}sky spot cancel -a'
+                   f'{colorama.Style.RESET_ALL}\n')
             # Add prefix to each line to align with the bullet point.
             msg += '\n'.join(
                 ['   ' + line for line in job_table.split('\n') if line != ''])
-    if cluster_status == global_user_state.ClusterStatus.INIT:
-        msg += ('\n * If there are pending/in-progress spot jobs, those '
-                'resources will not be terminated and require manual cleanup.')
-    click.echo(msg)
+            with ux_utils.print_exception_no_traceback():
+                raise exceptions.NotSupportedError(msg)
+        else:
+            click.echo('No in-progress spot jobs found. It should be safe to '
+                       'terminate, but please check the warning above before '
+                       'proceeding.')
 
 
 def _down_or_stop_clusters(
@@ -2316,7 +2331,7 @@ def _down_or_stop_clusters(
                 # TODO(zhwu): We can only have one reserved cluster (spot
                 # controller).
                 assert len(reserved_clusters) == 1, reserved_clusters
-                _hint_for_down_spot_controller(reserved_clusters[0])
+                _hint_or_raise_for_down_spot_controller(reserved_clusters[0])
 
                 if not no_confirm:
                     click.confirm('Proceed?',


Code review comments:
diff_hunk: @@ -2316,7 +2331,7 @@ def _down_or_stop_clusters(
                 # TODO(zhwu): We can only have one reserved cluster (spot
                 # controller).
                 assert len(reserved_clusters) == 1, reserved_clusters
-                _hint_for_down_spot_controller(reserved_clusters[0])
+                _hint_or_raise_for_down_spot_controller(reserved_clusters[0])
 
                 if not no_confirm:
                     click.confirm('Proceed?',
path: sky/cli.py
text: @user1:
One option, for discussion: we can check
```
value = click.prompt('Type 'delete' to proceed', type=str)
if value == 'delete':
  ...
else:
  raise click.Abort() from None
```

@author:
Great point! I changed it to the following. PTAL. : )
<img width="680" alt="image" src="https://user-images.githubusercontent.com/6753189/217997444-eeb2a252-f266-4364-8b5f-970027bdfcda.png">


@user1:
Kind of feel this is a bit too long; was proposing `delete` since it's used in AWS console for certain VPC resources. Wdyt? Feel free to do a poll too.

@author:
Good point! I used the `I understand` because that will remind the user to read the messages above, but I am not feeling strongly about this.  Switched back to `delete`.

diff_hunk: @@ -2200,48 +2200,63 @@ def down(
                            purge=purge)
 
 
-def _hint_for_down_spot_controller(controller_name: str):
+def _hint_or_raise_for_down_spot_controller(controller_name: str):
     # spot_jobs will be empty when the spot cluster is not running.
     cluster_status, _ = backend_utils.refresh_cluster_status_handle(
         controller_name)
     if cluster_status is None:
         click.echo('Managed spot controller has already been torn down.')
         return
 
+    if cluster_status == global_user_state.ClusterStatus.INIT:
+        with ux_utils.print_exception_no_traceback():
+            raise exceptions.NotSupportedError(
+                'Tearing down the spot controller while it is in INIT state is '
+                'not supported, as we cannot guarantee that all the spot jobs '
+                'are finished. Please wait until the spot controller is UP '
+                f'or fix it with {colorama.Style.BRIGHT}sky start '
+                f'{spot_lib.SPOT_CONTROLLER_NAME}{colorama.Style.RESET_ALL}.')
     msg = (f'{colorama.Fore.YELLOW}WARNING: Tearing down the managed '
            f'spot controller ({cluster_status.value}). Please be '
            f'aware of the following:{colorama.Style.RESET_ALL}'
            '\n * All logs and status information of the spot '
            'jobs (output of `sky spot queue`) will be lost.')
+    click.echo(msg)
     if cluster_status == global_user_state.ClusterStatus.UP:
-        try:
-            spot_jobs = core.spot_queue(refresh=False)
-        except exceptions.ClusterNotUpError:
-            # The spot controller cluster status changed during querying
-            # the spot jobs, use the latest cluster status, so that the
-            # message for INIT and STOPPED states will be correctly
-            # added to the message.
-            cluster_status = backend_utils.refresh_cluster_status_handle(
-                controller_name)
-            spot_jobs = []
+        with backend_utils.safe_console_status(
+                '[bold cyan]Checking spot jobs on the controller[/]'):
path: sky/cli.py
text: @user1:
```suggestion
                '[bold cyan]Checking for in-progress spot jobs[/]'):
```