[Logs] Fix logs download (skypilot-org#1502)

* fix the rsync

* format

* lint

* reorganize

* Add test

* add comment

Author: Michaelvll

sky/utils/command_runner.py

@@ -298,31 +298,34 @@ def rsync(
         # --filter
         rsync_command.append(RSYNC_FILTER_OPTION)
 
-        # --exclude-from
-        resolved_source = pathlib.Path(source).expanduser().resolve()
-        if (resolved_source / GIT_EXCLUDE).exists():
-            # Ensure file exists; otherwise, rsync will error out.
-            rsync_command.append(
-                RSYNC_EXCLUDE_OPTION.format(str(resolved_source / GIT_EXCLUDE)))
-
-        # rsync doesn't support '~' in a quoted target path. need to expand it.
-        full_source_str = str(resolved_source)
-        if resolved_source.is_dir():
-            full_source_str = os.path.join(full_source_str, '')
+        if up:
+            # The source is a local path, so we need to resolve it.
+            # --exclude-from
+            resolved_source = pathlib.Path(source).expanduser().resolve()
+            if (resolved_source / GIT_EXCLUDE).exists():
+                # Ensure file exists; otherwise, rsync will error out.
+                rsync_command.append(
+                    RSYNC_EXCLUDE_OPTION.format(
+                        str(resolved_source / GIT_EXCLUDE)))
 
         ssh_options = ' '.join(
             ssh_options_list(self.ssh_private_key, self.ssh_control_name))
         rsync_command.append(f'-e "ssh {ssh_options}"')
         # To support spaces in the path, we need to quote source and target.
+        # rsync doesn't support '~' in a quoted local path, but it is ok to
+        # have '~' in a quoted remote path.
         if up:
+            full_source_str = str(resolved_source)
+            if resolved_source.is_dir():
+                full_source_str = os.path.join(full_source_str, '')
             rsync_command.extend([
                 f'{full_source_str!r}',
                 f'{self.ssh_user}@{self.ip}:{target!r}',
             ])
         else:
             rsync_command.extend([
-                f'{self.ssh_user}@{self.ip}:{full_source_str!r}',
-                f'{target!r}',
+                f'{self.ssh_user}@{self.ip}:{source!r}',
+                f'{os.path.expanduser(target)!r}',
             ])
         command = ' '.join(rsync_command)
 

tests/test_smoke.py

@@ -138,6 +138,8 @@ def test_minimal():
             f'sky launch -y -c {name} examples/minimal.yaml',
             f'sky logs {name} 2 --status',
             f'sky logs {name} --status | grep "Job 2: SUCCEEDED"',  # Equivalent.
+            # Check the logs downloading
+            f'log_path=$(sky logs {name} 2 --sync-down | tail -n 1 | sed -E "s/^.*Job 2 logs: (.*)\\x1b\\[0m/\\1/g") && echo $log_path && test -f $log_path/run.log',
             # Ensure the raylet process has the correct file descriptor limit.
             f'sky exec {name} "prlimit -n --pid=\$(pgrep -f \'raylet/raylet --raylet_socket_name\') | grep \'"\'1048576 1048576\'"\'"',
             f'sky logs {name} 3 --status',  # Ensure the job succeeded.