Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recursive symlinks when uploading to sky storage #882

Merged
merged 6 commits into from
Jun 6, 2022

Conversation

romilbhardwaj
Copy link
Collaborator

@romilbhardwaj romilbhardwaj commented Jun 2, 2022

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:

  • ./tests/run_smoke_tests.sh test_file_mounts
  • pytest test_smoke.py::TestStorageWithCredentials

@romilbhardwaj romilbhardwaj changed the title Fix symlinks Fix recursive symlinks when uploading to sky storage Jun 2, 2022
@concretevitamin
Copy link
Member

Thanks!

Sky Storage now ignores all symlinks (like regular file_mounts, I believe).

By ignore, do you mean we now copy symlinks as-is, without following? Please also verify regular file_mounts do this (preferably in a smoke test, using_file_mounts!).

Relatedly: can we also update all relevant docs to reflect this change?

@michaelzhiluo
Copy link
Collaborator

For regular filemounts, I think there was a prior PR #431 that does not upload symlinks. Think this is just a nit pertaining to Sky Storage.

@suquark suquark force-pushed the storage_symlink_loop_fix branch from e2e7f46 to 40a0f3a Compare June 5, 2022 11:09
@romilbhardwaj
Copy link
Collaborator Author

Thanks!

Sky Storage now ignores all symlinks (like regular file_mounts, I believe).

By ignore, do you mean we now copy symlinks as-is, without following? Please also verify regular file_mounts do this (preferably in a smoke test, using_file_mounts!).

Relatedly: can we also update all relevant docs to reflect this change?

Sorry I should have clarified:

  • With Sky Storage - symlinks are not copied.
  • With file_mounts - symlinks are copied, but the links are broken. Their targets need to be added as a separate file_mount and the links must be fixed by the user.

I have added test cases for both using_file_mounts and TestStorageWithCredentials in smoke tests. Docs also updated.

(Commits will be pushed after the repo compression is done)

Copy link
Member

@concretevitamin concretevitamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @romilbhardwaj - some comments.

sky/cloud_stores.py Show resolved Hide resolved
sky/data/storage.py Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
examples/using_file_mounts.yaml Outdated Show resolved Hide resolved
tests/test_smoke.py Outdated Show resolved Hide resolved
@romilbhardwaj romilbhardwaj merged commit 52b9609 into master Jun 6, 2022
@romilbhardwaj romilbhardwaj deleted the storage_symlink_loop_fix branch June 6, 2022 19:58
suquark pushed a commit that referenced this pull request Jun 7, 2022
* Fix symlinks

* Add tests

* fix symlink

* address comments

* lint

* fix
suquark pushed a commit that referenced this pull request Jun 7, 2022
* Fix symlinks

* Add tests

* fix symlink

* address comments

* lint

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Storage] COPY mode storage bug: infinite loop if source has a mylink -> . symlink
3 participants