Skip to content

sftp: make move across drives possible #2709

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.

Sign up for GitHub

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

Merged
merged 4 commits into from Nov 2, 2019
Merged

sftp: make move across drives possible #2709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab1ead8
sftp: make move across drives possible
Nov 1, 2019
283f848
sftp: make move an atomic operation
Nov 1, 2019
8f70000
sftp: move -> fallback to copy when sftp.rename fails
Nov 1, 2019
16164e6
sftp: move -> use atomic copying
Nov 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion dvc/remote/ssh/connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,27 @@ def download(self, src, dest, no_progress_bar=False, progress_title=None):
self.sftp.get(src, dest, callback=pbar.update_to)

def move(self, src, dst):
"""Rename src to dst, if it is not possible (in case src and dst are
on different filesystems) and actual physical copying of data is
happening.
"""
self.makedirs(posixpath.dirname(dst))
self.sftp.rename(src, dst)

try:
self.sftp.rename(src, dst)
Copy link
Contributor

@shcheklein shcheklein Nov 1, 2019

Choose a reason for hiding this comment

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

are you 100% sure that rename is never copying data? just to double-check

Copy link
Author

@ghost ghost Nov 1, 2019

Choose a reason for hiding this comment

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

If I understand the man page correctly, POSIX's rename is atomic: http://man7.org/linux/man-pages/man2/rename.2.html

But it turns out SFTP doesn't follow POSIX standard, there's an specific call sftp.posix_rename for that, it is implemented as an OpenSSH extension (widely used) so I guess it is fine to use it :)

Copy link
Contributor

@shcheklein shcheklein Nov 1, 2019

Choose a reason for hiding this comment

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

i was more asking about it failing if you try to rename from one mounted point to another - is is part of the standard as it is for regular posix rename

re atomicity - yeah, it looks like sftp rename is not atomic, but in this sense our own version with tmp file would not be better, so it's probably safe to use it 👍

except OSError:
self._atomic_copy(src, dst)

self.remove(src)

def _atomic_copy(self, src, dst):
tmp = tmp_fname(dst)

try:
self.copy(src, tmp)
self.sftp.rename(tmp, dst)
finally:
self.remove(tmp)

def upload(self, src, dest, no_progress_bar=False, progress_title=None):
self.makedirs(posixpath.dirname(dest))
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/remote/ssh/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,9 @@ def test_hardlink(repo_dir, ssh):
def test_copy(repo_dir, ssh):
ssh.copy("foo", "link")
assert filecmp.cmp("foo", "link")


def test_move(repo_dir, ssh):
ssh.move("foo", "copy")
assert os.path.exists("copy")
assert not os.path.exists("foo")