Repository: skypilot-org/skypilot
Pull request: Make rsync more resilient
<!-- Describe the changes in this PR -->

Closes #1538.
- `rsync` related errors are normally related to transient network issues, so this adds "to fix" information for the user in error messages
- This also adds retrying to the `rsync` command to reduce the frequency of failures due to transient network issues

<!-- 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):

- [ ] Any manual or new tests for this PR (please specify below)
- [ ] 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:
UX: print "to fix" suggestions for rsync related errors 
Currently rsync related errors (e.g., transient network issues; we should look for repros) are not helpful because they don't display "how to fix" suggestions. 

In general, we should strive for including in each error message some "to fix" hints.

Examples -- we should audit all these places:
```
...
Failed to run command before rsync <my bucket> -> ~/sky_workdir.

...
Failed to rsync up: <script> -> ~/.sky/spot_tasks/<yaml>
```

In the above, a simple hint to add could be, "Ensure network is stable, then retry the command/API."

----

Orthoghnoally -- We may also want to add retries for these rsync's.

Here's an example of running the `test_logs` smoke test and encountering likely transient rsync errors:
```
...
+ sky logs test-logs-2b289124-d0 3 4 --sync-down
D 01-04 05:57:08 skypilot_config.py:85] Using config path: /Users/zongheng/.sky/config.yaml
D 01-04 05:57:08 skypilot_config.py:88] Config loaded: None
Syncing down logs to local...
I 01-04 05:57:10 cloud_vm_ray_backend.py:2683] Job 3 logs: /Users/zongheng/sky_logs/sky-2023-01-04-05-56-49-216418
I 01-04 05:57:10 cloud_vm_ray_backend.py:2683] Job 4 logs: /Users/zongheng/sky_logs/sky-2023-01-04-05-56-57-808346
sky.exceptions.CommandError: Command rsync -Pavz --filter='dir-merge,- .gitignore' -e "ssh -i ~/.ssh/sky-key -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes -o ExitOnForwardFailure=yes -o ServerAliveInterval=5 -o ServerAliveCountMax=3 -o ConnectTimeout=30s -o ForwardAgent=yes -o ControlMaster=auto -o ControlPath=/tmp/skypilot_ssh_zongheng/828a340d70/%C -o ControlPersist=300s" ubuntu@3.86.32.116:'~/sky_logs/sky-2023-01-04-05-56-49-216418/*' '/Users/zongheng/sky_logs/sky-2023-01-04-05-56-49-216418' failed with return code 255.
Failed to rsync down: ~/sky_logs/sky-2023-01-04-05-56-49-216418/* -> /Users/zongheng/sky_logs/sky-2023-01-04-05-56-49-216418
```
Or, `autodown` failed rsync up
```
...
I 01-04 06:20:19 cloud_vm_ray_backend.py:1185] Successfully provisioned or found existing VMs.
I 01-04 06:20:25 cloud_vm_ray_backend.py:2347] Running setup on 2 nodes.
...
sky.exceptions.CommandError: Command rsync -Pavz --filter='dir-merge,- .gitignore' -e "ssh -i ~/.ssh/sky-key -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o IdentitiesOnly=yes -o ExitOnForwardFailure=yes -o ServerAliveInterval=5 -o ServerAliveCountMax=3 -o ConnectTimeout=30s -o ForwardAgent=yes" '/private/var/folders/8f/56gzvwkd3n3293xjlrztr6600000gp/T/sky_setup_zqkd25am' ubuntu@54.89.178.24:'/tmp/sky_setup_zqkd25am' failed with return code 255.
Failed to rsync up: /var/folders/8f/56gzvwkd3n3293xjlrztr6600000gp/T/sky_setup_zqkd25am -> /tmp/sky_setup_zqkd25am
```

Hints from issue discussion:
A user met this error and didn't know it's a transient network connection issue. I'll draft a PR for this.

Commit message: Fix formatting

Patch to review:
diff --git a/sky/backends/backend_utils.py b/sky/backends/backend_utils.py
index 5ef5b7b2903..500f4641baf 100644
--- a/sky/backends/backend_utils.py
+++ b/sky/backends/backend_utils.py
@@ -1186,7 +1186,8 @@ def _sync_node(runner: 'command_runner.SSHCommandRunner') -> None:
             subprocess_utils.handle_returncode(
                 rc,
                 cmd, ('Failed to run command before rsync '
-                      f'{origin_source} -> {target}.'),
+                      f'{origin_source} -> {target}.'
+                      'Ensure that the network is stable, then retry.'),
                 stderr=stdout + stderr)
 
         if run_rsync:
diff --git a/sky/utils/command_runner.py b/sky/utils/command_runner.py
index 410946d04a7..b7668966190 100644
--- a/sky/utils/command_runner.py
+++ b/sky/utils/command_runner.py
@@ -292,6 +292,7 @@ def rsync(
         # Advanced options.
         log_path: str = os.devnull,
         stream_logs: bool = True,
+        retry_cnt: int = 1,
     ) -> None:
         """Uses 'rsync' to sync 'source' to 'target'.
 
@@ -356,11 +357,20 @@ def rsync(
                                                      stream_logs=stream_logs,
                                                      shell=True,
                                                      require_outputs=True)
+        while returncode != 0 and retry_cnt > 0:
+            returncode, _, stderr = log_lib.run_with_log(
+                command,
+                log_path=log_path,
+                stream_logs=stream_logs,
+                shell=True,
+                require_outputs=True)
+            retry_cnt -= 1
 
         direction = 'up' if up else 'down'
-        subprocess_utils.handle_returncode(
-            returncode,
-            command,
-            f'Failed to rsync {direction}: {source} -> {target}',
-            stderr=stderr,
-            stream_logs=stream_logs)
+        error_msg = (f'Failed to rsync {direction}: {source} -> {target}.'
+                     'Ensure that the network is stable, then retry.')
+        subprocess_utils.handle_returncode(returncode,
+                                           command,
+                                           error_msg,
+                                           stderr=stderr,
+                                           stream_logs=stream_logs)


Code review comments:
diff_hunk: @@ -292,6 +292,7 @@ def rsync(
         # Advanced options.
         log_path: str = os.devnull,
         stream_logs: bool = True,
+        retry_cnt: int = 1,
path: sky/utils/command_runner.py
text: @user1:
Add to Args below?

@author:
Thanks for catching this!

diff_hunk: @@ -356,11 +357,20 @@ def rsync(
                                                      stream_logs=stream_logs,
                                                      shell=True,
                                                      require_outputs=True)
+        while returncode != 0 and retry_cnt > 0:
+            returncode, _, stderr = log_lib.run_with_log(
+                command,
+                log_path=log_path,
+                stream_logs=stream_logs,
+                shell=True,
+                require_outputs=True)
path: sky/utils/command_runner.py
text: @user1:
nit: May want to fold in L355-359 to inside the loop to reduce duplication

diff_hunk: @@ -356,11 +357,20 @@ def rsync(
                                                      stream_logs=stream_logs,
                                                      shell=True,
                                                      require_outputs=True)
+        while returncode != 0 and retry_cnt > 0:
+            returncode, _, stderr = log_lib.run_with_log(
+                command,
+                log_path=log_path,
+                stream_logs=stream_logs,
+                shell=True,
+                require_outputs=True)
+            retry_cnt -= 1
 
         direction = 'up' if up else 'down'
-        subprocess_utils.handle_returncode(
-            returncode,
-            command,
-            f'Failed to rsync {direction}: {source} -> {target}',
-            stderr=stderr,
-            stream_logs=stream_logs)
+        error_msg = (f'Failed to rsync {direction}: {source} -> {target}.'
path: sky/utils/command_runner.py
text: @user1:
```suggestion
        error_msg = (f'Failed to rsync {direction}: {source} -> {target}. '
```

diff_hunk: @@ -356,11 +357,20 @@ def rsync(
                                                      stream_logs=stream_logs,
                                                      shell=True,
                                                      require_outputs=True)
+        while returncode != 0 and retry_cnt > 0:
path: sky/utils/command_runner.py
text: @user1:
Wait a random amount first? Or use the Backoff class.

diff_hunk: @@ -1186,7 +1186,8 @@ def _sync_node(runner: 'command_runner.SSHCommandRunner') -> None:
             subprocess_utils.handle_returncode(
                 rc,
                 cmd, ('Failed to run command before rsync '
-                      f'{origin_source} -> {target}.'),
+                      f'{origin_source} -> {target}.'
path: sky/backends/backend_utils.py
text: @user1:
```suggestion
                      f'{origin_source} -> {target}. '
```