Repository: skypilot-org/skypilot
Pull request: [UX] Reset autostop timer whenever autostop config changes on remote machine
<!-- Describe the changes in this PR -->

Fixes #1511

<!-- 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):

- [ ] Code formatting: `bash format.sh`
- [ ] Any manual or new tests for this PR (please specify below)
- [ ] All smoke tests: `pytest tests/test_smoke.py` 
- [x] Relevant individual smoke tests: `pytest tests/test_smoke.py::test_fill_in_the_name` 
  - [x] `pytest tests/test_smoke.py::test_autostop`
  - [x] `pytest tests/test_smoke.py::test_autodown`
- [ ] Backward compatibility tests: `bash tests/backward_comaptibility_tests.sh`


Problem statement:
[Autostop] Need to reset the timer for every execution stage
Our user observed that the spot controller is autostopped, during the submission of a spot job (during setting up the controller). It is possible because we only reset the autostop timer, when the controller process is submitted to the ray job.

One way to mitigate it is to reset the autostop timer at the beginning of every execution stage. cc @concretevitamin

Hints from issue discussion:
This issue is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days.
This issue was closed because it has been stalled for 10 days with no activity.
This issue is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days.
This issue was closed because it has been stalled for 10 days with no activity.

Commit message: update smoke test

Patch to review:
diff --git a/sky/cli.py b/sky/cli.py
index 6feb97690fa..e0069a671ab 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -2559,17 +2559,12 @@ def autostop(
 
     - The cluster has restarted.
 
-    - An autostop is set when there is no active setting. (Namely, either
-      there's never any autostop setting set, or the previous autostop setting
-      was canceled.) This is useful for restarting the autostop timer.
+    - A new autostop idle time is set.
 
-    Example: say a cluster without any autostop set has been idle for 1 hour,
-    then an autostop of 30 minutes is set. The cluster will not be immediately
-    autostopped. Instead, the idleness timer only starts counting after the
-    autostop setting was set.
-
-    When multiple autostop settings are specified for the same cluster, the
-    last setting takes precedence.
+    Example: say a cluster with autostop set to be 2 hours has been idle for 1
+    hour, then autostop is reset to 30 minutes. The cluster will not be
+    immediately autostopped. Instead, the idleness timer only restart counting
+    after autostop setting was reset.
 
     Typical usage:
 
@@ -2581,9 +2576,8 @@ def autostop(
         # Cancel autostop for a specific cluster.
         sky autostop cluster_name --cancel
         \b
-        # Since autostop was canceled in the last command, idleness will
-        # restart counting after this command.
-        sky autostop cluster_name -i 60
+        # Autodown this cluster after 60 minutes of idleness.
+        sky autostop cluster_name -i 60 --down
     """
     if cancel and idle_minutes is not None:
         raise click.UsageError(
diff --git a/sky/skylet/autostop_lib.py b/sky/skylet/autostop_lib.py
index 91c98f89da1..abc887b3d21 100644
--- a/sky/skylet/autostop_lib.py
+++ b/sky/skylet/autostop_lib.py
@@ -54,13 +54,11 @@ def get_autostop_config() -> AutostopConfig:
 def set_autostop(idle_minutes: int, backend: Optional[str], down: bool) -> None:
     boot_time = psutil.boot_time()
     autostop_config = AutostopConfig(idle_minutes, boot_time, backend, down)
-    prev_autostop_config = get_autostop_config()
     configs.set_config(_AUTOSTOP_CONFIG_KEY, pickle.dumps(autostop_config))
     logger.debug(f'set_autostop(): idle_minutes {idle_minutes}, down {down}.')
-    if (prev_autostop_config.autostop_idle_minutes < 0 or
-            prev_autostop_config.boot_time != psutil.boot_time()):
-        # Either autostop never set, or has been canceled. Reset timer.
-        set_last_active_time_to_now()
+    # Reset timer whenever the autostop is set on the machine, i.e. the idle
+    # time will be counted from now.
+    set_last_active_time_to_now()
 
 
 def set_autostopping_started() -> None:
diff --git a/sky/skylet/constants.py b/sky/skylet/constants.py
index fa284022329..0e76ecb71be 100644
--- a/sky/skylet/constants.py
+++ b/sky/skylet/constants.py
@@ -41,7 +41,7 @@
 # cluster yaml is updated.
 #
 # TODO(zongheng,zhanghao): make the upgrading of skylet automatic?
-SKYLET_VERSION = '7'
+SKYLET_VERSION = '8'
 # The version of the lib files that skylet/jobs use. Whenever there is an API
 # change for the job_lib or log_lib, we need to bump this version, so that the
 # user can be notified to update their SkyPilot version on the remote cluster.
diff --git a/tests/test_smoke.py b/tests/test_smoke.py
index 6206f50a8df..bd2db80ecda 100644
--- a/tests/test_smoke.py
+++ b/tests/test_smoke.py
@@ -1713,7 +1713,7 @@ def test_autostop(generic_cloud: str):
             f'sky status | grep {name} | grep "1m"',
 
             # Ensure the cluster is not stopped early.
-            'sleep 20',
+            'sleep 40',
             f's=$(sky status {name} --refresh); echo "$s"; echo; echo; echo "$s"  | grep {name} | grep UP',
 
             # Ensure the cluster is STOPPED.
@@ -1728,12 +1728,11 @@ def test_autostop(generic_cloud: str):
             f'sky exec {name} tests/test_yamls/minimal.yaml',
             f'sky logs {name} 2 --status',
 
-            # Test restarting the idleness timer via cancel + reset:
+            # Test restarting the idleness timer via reset:
             f'sky autostop -y {name} -i 1',  # Idleness starts counting.
-            'sleep 30',  # Almost reached the threshold.
-            f'sky autostop -y {name} --cancel',
+            'sleep 40',  # Almost reached the threshold.
             f'sky autostop -y {name} -i 1',  # Should restart the timer.
-            'sleep 30',
+            'sleep 40',
             f's=$(sky status {name} --refresh); echo "$s"; echo; echo; echo "$s" | grep {name} | grep UP',
             f'sleep {autostop_timeout}',
             f's=$(sky status {name} --refresh); echo "$s"; echo; echo; echo "$s"  | grep {name} | grep STOPPED',
@@ -1772,7 +1771,7 @@ def test_autodown(generic_cloud: str):
             # Ensure autostop is set.
             f'sky status | grep {name} | grep "1m (down)"',
             # Ensure the cluster is not terminated early.
-            'sleep 30',
+            'sleep 40',
             f's=$(sky status {name} --refresh); echo "$s"; echo; echo; echo "$s"  | grep {name} | grep UP',
             # Ensure the cluster is terminated.
             f'sleep {autodown_timeout}',


Code review comments:
diff_hunk: @@ -2559,17 +2559,12 @@ def autostop(
 
     - The cluster has restarted.
 
-    - An autostop is set when there is no active setting. (Namely, either
-      there's never any autostop setting set, or the previous autostop setting
-      was canceled.) This is useful for restarting the autostop timer.
+    - A new autostop idle time is set.
path: sky/cli.py
text: @user1:
"new": What if there was an existing N minutes, and users submit an autostop command with N again?

@author:
Removed "new" to be more clear. Thanks!

diff_hunk: @@ -2559,17 +2559,12 @@ def autostop(
 
     - The cluster has restarted.
 
-    - An autostop is set when there is no active setting. (Namely, either
-      there's never any autostop setting set, or the previous autostop setting
-      was canceled.) This is useful for restarting the autostop timer.
+    - A new autostop idle time is set.
 
-    Example: say a cluster without any autostop set has been idle for 1 hour,
-    then an autostop of 30 minutes is set. The cluster will not be immediately
-    autostopped. Instead, the idleness timer only starts counting after the
-    autostop setting was set.
-
-    When multiple autostop settings are specified for the same cluster, the
-    last setting takes precedence.
+    Example: say a cluster with autostop set to be 2 hours has been idle for 1
+    hour, then autostop is reset to 30 minutes. The cluster will not be
+    immediately autostopped. Instead, the idleness timer only restart counting
+    after autostop setting was reset.
path: sky/cli.py
text: @user1:
```suggestion
    Example: say a cluster with autostop set to 2 hours has been idle for 1
    hour, then autostop is reset to 30 minutes. The cluster will not be
    immediately autostopped. Instead, the idleness timer restarts counting
    when the second autostop setting of 30 minutes was submitted.
```

diff_hunk: @@ -2559,17 +2559,12 @@ def autostop(
 
     - The cluster has restarted.
 
-    - An autostop is set when there is no active setting. (Namely, either
-      there's never any autostop setting set, or the previous autostop setting
-      was canceled.) This is useful for restarting the autostop timer.
+    - A new autostop idle time is set.
 
-    Example: say a cluster without any autostop set has been idle for 1 hour,
path: sky/cli.py
text: @user1:
Keep/update this scenario?

@author:
The scenario shoud be similar as the new scenario we have. Do you think it is needed to keep this?

@user1:
I think it'd help the users/readers. Even though the technical impl is the same, it's "setting autostop for the first time" vs. "resetting autostop" scenarios.