-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
dvc: add sanity checks for hard/symlinks #3088
Conversation
7839ccb
to
6fce869
Compare
dvc/system.py
Outdated
@@ -28,6 +35,8 @@ def hardlink(source, link_name): | |||
except OSError as exc: | |||
raise DvcException("failed to link") from exc | |||
|
|||
_verify_link(link_name, System.is_hardlink, "hardlink") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit worried about a potential performance hit, so might just add these checks to dvc version
. Or maybe to cache type caching mechanism that we have, so we could only run it once. Will take a closer look soon.
dvc/system.py
Outdated
@@ -36,6 +45,8 @@ def symlink(source, link_name): | |||
except OSError as exc: | |||
raise DvcException("failed to symlink") from exc | |||
|
|||
_verify_link(link_name, System.is_symlink, "symlink") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't seen any weird symlinks yet, but tbh I won't be surprised...
Hi @efiop, I just tested this PR on my setup and I think it has discovered a latent bug.
So repo and cache are both on the network drive (SMB). The user cannot create symlinks. The network protocol doesn't support creating reflinks. The network drive, although capable of creating hardlinks, cannot tell the difference between an ordinary file and a hardlink (as seen with #3080). So we expect that only the copy type will be used.
First problem: DVC (actually sqlite3) doesn't like having its local config on a network drive. Let's try a workaround to make it less obvious and try again.
Note two things:
Note that I can workaround the issue by setting the cache type to be
|
Or maybe the problem is just that on link validation failure the link needs to be deleted (remember that this FS can create hardlinks but not see them). |
@rxxg Thanks for trying it out! Correct, current patch doesn't delete the link, it was just created as a POC. I'm working on an updated version right now, that will actually delete the hardlink/symlink if it doesn't pass verification. Thanks for the heads up! π |
6ecb01e
to
dc344f0
Compare
|
0e29a83
to
6b1f185
Compare
@rxxg Oh, thanks for trying it out! Pushed a new version that should handle that more gracefully. Please give it a try π |
|
@rxxg Very interesting. It allows us to create the file and doesn't allow to stat it. Very odd. Something is very broken in your setup, I'm not sure I understand the reasons... |
@rxxg Maybe there is something special about the mounting parameters that you use? |
Sorry, no idea. The strange thing is that the file can be easily deleted by explorer or |
@rxxg Got it. But |
Fine in my limited testing. |
@rxxg Got it. I think we could leave it as is, until new problems arise or someone else runs into the same issues, at which point we will need to do an in-depth investigation. |
Fixes #3080
β Have you followed the guidelines in the Contributing to DVC list?
π Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
β Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. π