Repository: skypilot-org/skypilot
Pull request: Fix recursive symlinks when uploading to sky storage 
Closes #814 by using `--no-follow-symlinks` flag for `aws s3 sync`. Sky Storage now ignores all symlinks (like regular file_mounts, I believe).

Tested:
- [x] `./tests/run_smoke_tests.sh test_file_mounts`
- [x] `pytest test_smoke.py::TestStorageWithCredentials`


Problem statement:
[Storage] COPY mode storage bug: infinite loop if source has a `mylink -> .` symlink
My Sky root dir has a symlink
```
 » ll mylink
lrwxr-xr-x@ 1 zongheng  staff  1 Mar  6 09:01 mylink@ -> .
```
When running
```
bash tests/run_smoke_tests.sh  test_spot_storage
```
It seems to get confused and recursively uploads my dir, causing the test to timeout:
```
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/heads/fix-unfulfilled to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/heads/fix-unfulfilled
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/heads/master to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/heads/master
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/heads/onprem to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/heads/onprem
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/heads/spot-logs to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/heads/spot-logs
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/add-spot-ml-exp to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/add-spot-ml-exp
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-bert-qa to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-bert-qa
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-copy-mount-for-spot to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-copy-mount-for-spot
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-down-log to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-down-log
upload: mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-job-status-update-race to s3://sky-test-1652118988/mylink/mylink/mylink/mylink/mylink/.git/refs/remotes/origin/fix-job-status-update-race
^Cmpleted 690.9 MiB/~789.1 MiB (974.6 KiB/s) with ~993 file(s) remaining (calculating...)
```

Hints from issue discussion:
Good catch - seems to be inherited from [aws-cli #3709](https://github.com/aws/aws-cli/issues/3709). We could either:

1. Use `--no-follow-symlinks` [flag](https://docs.aws.amazon.com/cli/latest/reference/s3/sync.html) for `aws s3 sync`. This would ignore all symlinks and not upload even broken links (like we do for `file_mounts`, I think).
2. Implement our own sync logic or call `aws s3 sync` recursively for each directory after iterating through all files and manually resolving symlinks. 

Leaning towards 1. for now, but happy to discuss alternatives

Commit message: fix symlink

Patch to review:
diff --git a/docs/source/reference/storage.rst b/docs/source/reference/storage.rst
index 1c8eda722ba..d68e528aa0b 100644
--- a/docs/source/reference/storage.rst
+++ b/docs/source/reference/storage.rst
@@ -171,9 +171,9 @@ and storage mounting:
 
 .. note::
     Symbolic links are handled differently in :code:`file_mounts` depending on whether Sky Storage is used.
-    For mounts backed by Sky Storage, referenced data for all symbolic links is copied to remote.
-    For mounts not using Sky Storage (e.g., those using rsync) the symbolic links are directly copied.
-    Their targets must be separately mounted or else the symlinks may break.
+    For mounts backed by Sky Storage, symbolic links are not copied to remote.
+    For mounts not using Sky Storage (e.g., those using rsync) the symbolic links are directly copied, not their target data.
+    The targets must be separately mounted or else the symlinks may break.
 
 Creating a shared file system
 -----------------------------
diff --git a/examples/using_file_mounts.yaml b/examples/using_file_mounts.yaml
index edfb08a1eb1..fd99099d93c 100644
--- a/examples/using_file_mounts.yaml
+++ b/examples/using_file_mounts.yaml
@@ -116,3 +116,8 @@ run: |
   # sky.egg-info/ should not exists on remote due to the .gitingore
   cd ~/sky_workdir
   ! ls sky.egg-info/
+
+  # Symlinks should be copied, but they will be broken
+  # Assumes symlink named circle-link in /tmp/workdir (created by test_smoke.py)
+  ls /tmp/workdir/circle-link
+  ! readlink -f /tmp/workdir/circle
diff --git a/sky/cloud_stores.py b/sky/cloud_stores.py
index 08e55135467..215d0eccf12 100644
--- a/sky/cloud_stores.py
+++ b/sky/cloud_stores.py
@@ -70,7 +70,8 @@ def make_sync_dir_command(self, source: str, destination: str) -> str:
         # To increase parallelism, modify max_concurrent_requests in your
         # aws config file (Default path: ~/.aws/config).
         download_via_awscli = f'mkdir -p {destination} && \
-                                aws s3 sync {source} {destination}'
+                                aws s3 sync --no-follow-symlinks ' \
+                              f'{source} {destination}'
 
         all_commands = list(self._GET_AWSCLI)
         all_commands.append(download_via_awscli)
diff --git a/sky/data/storage.py b/sky/data/storage.py
index da44484db45..5f750eaa42b 100644
--- a/sky/data/storage.py
+++ b/sky/data/storage.py
@@ -749,7 +749,8 @@ def sync_local_dir(self) -> None:
         file (Default path: ~/.aws/config).
         """
         source = os.path.abspath(os.path.expanduser(self.source))
-        sync_command = f'aws s3 sync {source} s3://{self.name}/'
+        sync_command = 'aws s3 sync --no-follow-symlinks ' \
+                       f'{source} s3://{self.name}/'
         with backend_utils.safe_console_status(
                 f'[bold cyan]Syncing '
                 f'[green]{self.source} to s3://{self.name}/'):
diff --git a/tests/test_smoke.py b/tests/test_smoke.py
index 8fae2fc0d64..3e28166c3cf 100644
--- a/tests/test_smoke.py
+++ b/tests/test_smoke.py
@@ -158,6 +158,7 @@ def test_file_mounts():
             'touch ~/tmpfile',
             'mkdir -p ~/tmp-workdir',
             'touch ~/tmp-workdir/foo',
+            'ln -f -s ~/tmp-workdir/ ~/tmp-workdir/circle-link',
             f'sky launch -y -c {name} examples/using_file_mounts.yaml',
             f'sky logs {name} 1 --status',  # Ensure the job succeeded.
         ],
@@ -543,6 +544,8 @@ def tmp_mount(self, tmp_path):
         tmp_dir.mkdir()
         tmp_file = tmp_dir / 'tmp-file'
         tmp_file.write_text('test')
+        circle_link = tmp_dir / 'circle-link'
+        circle_link.symlink_to(tmp_dir, target_is_directory=True)
         yield str(tmp_dir)
 
     @pytest.fixture
@@ -625,6 +628,14 @@ def test_upload_to_existing_bucket(self, tmp_awscli_bucket, tmp_mount):
             'File not found in bucket - output was : {}'.format(out.decode
                                                                 ('utf-8'))
 
+        # Check symlinks - symlinks don't get copied by sky storage
+        assert pathlib.Path(tmp_mount + '/circle-link').is_symlink(), \
+            'circle-link was not found in the upload source - ' \
+            'are the test fixtures correct?'
+        assert 'circle-link' not in out.decode('utf-8'), \
+            'Symlink found in bucket - ls output was : {}'.format(out.decode
+                                                                  ('utf-8'))
+
         # Run sky storage ls to check if storage object exists in the output.
         # It should not exist because the bucket was created externally.
         out = subprocess.check_output(['sky', 'storage', 'ls'])


Code review comments:
diff_hunk: @@ -625,6 +628,14 @@ def test_upload_to_existing_bucket(self, tmp_awscli_bucket, tmp_mount):
             'File not found in bucket - output was : {}'.format(out.decode
                                                                 ('utf-8'))
 
+        # Check symlinks - symlinks don't get copied by sky storage
+        assert pathlib.Path(tmp_mount + '/circle-link').is_symlink(), \
path: tests/test_smoke.py
text: @user1:
nit: `(pathlib.Path(tmp_mount) / 'circle-link')`

diff_hunk: @@ -116,3 +116,8 @@ run: |
   # sky.egg-info/ should not exists on remote due to the .gitingore
   cd ~/sky_workdir
   ! ls sky.egg-info/
+
+  # Symlinks should be copied, but they will be broken
+  # Assumes symlink named circle-link in /tmp/workdir (created by test_smoke.py)
+  ls /tmp/workdir/circle-link
+  ! readlink -f /tmp/workdir/circle
path: examples/using_file_mounts.yaml
text: @user1:
typo? `circle` -> `circle-link`?