Repository: skypilot-org/skypilot
Pull request: [UX] Better logging message for operators on the clusters terminated manually in the cloud
<!-- Describe the changes in this PR -->

Fixes #2298.

Note this only works for the spot clusters or the clusters with autostop/down setup, as the status refresh will not be triggered for the operators on the normal clusters.

Previously:
```
sky logs test-warning-spot
ValueError: Cluster 'test-warning-spot' does not exist.
```

Now:
```
sky logs test-warning-spot
ValueError: Cluster 'test-warning-spot' was preempted or manually terminated in console.
```

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [x] Code formatting: `bash format.sh`
- [x] Any manual or new tests for this PR (please specify below)
  -  `sky launch -c test-warning-spot --cpus 2 --cloud gcp --use-spot`
  - manually terminate the cluster in the console
  - `sky logs test-warning-spot`
- [ ] 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: `bash tests/backward_comaptibility_tests.sh`


Problem statement:
Ops with auto-refresh should warn about clusters terminated on the cloud
Currently, clusters terminated on the cloud (either manually, or preempted, or auto-downed) are only warned if they are caught in `status -r`, where we print "Cluster terminated on the cloud: xxx".

However, for the preemption case, user has run into the following:
- a preemption happened
- ran `sky logs <clus> 1` which prints
```
» sky logs dbg2 1
ValueError: Cluster 'dbg2' does not exist.
```
which is very confusing to the user. This happened because `sky logs` auto-refresh first without warning any cluster being terminated on the cloud. 
- After this, running `status` would no longer show the cluster, which makes the user wonder exactly what happened.

I reproduced the above with both `sky logs` and `sky queue`, and we should look at all ops with such auto-refresh. Repro:
```
sky launch --cpus 2+ --use-spot  --cloud aws -i60 --down -c dbg

- 'dbg' exists in status table, is UP
- manually terminate in console

» sky queue dbg
Fetching and parsing job queue...
ValueError: Cluster 'dbg' does not exist.
```

Hints from issue discussion:

Commit message: Fix message

Patch to review:
diff --git a/sky/backends/backend_utils.py b/sky/backends/backend_utils.py
index 4e734f7e0fb..c711aec3374 100644
--- a/sky/backends/backend_utils.py
+++ b/sky/backends/backend_utils.py
@@ -2359,10 +2359,15 @@ def check_cluster_available(
         exceptions.CloudUserIdentityError: if we fail to get the current user
           identity.
     """
+    record = global_user_state.get_cluster_from_name(cluster_name)
     if dryrun:
-        record = global_user_state.get_cluster_from_name(cluster_name)
         assert record is not None, cluster_name
         return record['handle']
+
+    previous_cluster_status = None
+    if record is not None:
+        previous_cluster_status = record['status']
+
     try:
         cluster_status, handle = refresh_cluster_status_handle(cluster_name)
     except exceptions.ClusterStatusFetchingError as e:
@@ -2381,7 +2386,6 @@ def check_cluster_available(
             f'Failed to refresh the status for cluster {cluster_name!r}. It is '
             f'not fatal, but {operation} might hang if the cluster is not up.\n'
             f'Detailed reason: {e}')
-        record = global_user_state.get_cluster_from_name(cluster_name)
         if record is None:
             cluster_status, handle = None, None
         else:
@@ -2390,10 +2394,15 @@ def check_cluster_available(
     bright = colorama.Style.BRIGHT
     reset = colorama.Style.RESET_ALL
     if handle is None:
+        if previous_cluster_status is None:
+            error_msg = f'Cluster {cluster_name!r} does not exist.'
+        else:
+            error_msg = (f'Cluster {cluster_name!r} was preempted or manually '
+                         'terminated in console.')
+
         with ux_utils.print_exception_no_traceback():
-            raise ValueError(
-                f'{colorama.Fore.YELLOW}Cluster {cluster_name!r} does not '
-                f'exist.{reset}')
+            raise ValueError(f'{colorama.Fore.YELLOW}{error_msg}{reset}')
+    assert cluster_status is not None, 'handle is not None but status is None'
     backend = get_backend_from_handle(handle)
     if check_cloud_vm_ray_backend and not isinstance(
             backend, backends.CloudVmRayBackend):


Code review comments:
diff_hunk: @@ -2390,10 +2394,15 @@ def check_cluster_available(
     bright = colorama.Style.BRIGHT
     reset = colorama.Style.RESET_ALL
     if handle is None:
+        if previous_cluster_status is None:
+            error_msg = f'Cluster {cluster_name!r} does not exist.'
+        else:
+            error_msg = (f'Cluster {cluster_name!r} was preempted or manually '
path: sky/backends/backend_utils.py
text: @user1:
Is it possible to make this more fine-grained:
- check if the cluster is a spot cluster, and if so += "was preempted" 
- check if the cluster had autodown specified, and if so += "was autodowned"
- += "or manually terminated"

E.g., "Cluster X not found on the cloud provider. It was either A, B, or C".

@author:
Done. Thanks! The current message looks like this:
```
sky logs sky-2600-zhwu                         ✭ ✱
ValueError: Cluster 'sky-2600-zhwu' not found on the cloud provider. It was either preempted, autodowned, or manually terminated in console.
```