From ab1ead881ba8dc8d8f940329f786706a7191b5aa Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Fri, 1 Nov 2019 12:40:16 -0600 Subject: [PATCH 1/4] sftp: make move across drives possible Fix #2704 --- dvc/remote/ssh/connection.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dvc/remote/ssh/connection.py b/dvc/remote/ssh/connection.py index dcbb1727cc..657a059ec3 100644 --- a/dvc/remote/ssh/connection.py +++ b/dvc/remote/ssh/connection.py @@ -185,8 +185,17 @@ 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): + """Use `SFTP RENAME` operation when possible, otherwise, + fallback to copy and remove, since the former one doesn't + work across drives. + """ self.makedirs(posixpath.dirname(dst)) - self.sftp.rename(src, dst) + + try: + self.sftp.rename(src, dst) + except OSError: + self.copy(src, dst) + self.remove(src) def upload(self, src, dest, no_progress_bar=False, progress_title=None): self.makedirs(posixpath.dirname(dest)) From 283f84882209e67d0c989ab02825a7dec5ab2843 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Fri, 1 Nov 2019 13:17:22 -0600 Subject: [PATCH 2/4] sftp: make move an atomic operation --- dvc/remote/ssh/connection.py | 17 +++++++++-------- tests/unit/remote/ssh/test_connection.py | 6 ++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/dvc/remote/ssh/connection.py b/dvc/remote/ssh/connection.py index 657a059ec3..f8a38a0eb8 100644 --- a/dvc/remote/ssh/connection.py +++ b/dvc/remote/ssh/connection.py @@ -185,17 +185,18 @@ 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): - """Use `SFTP RENAME` operation when possible, otherwise, - fallback to copy and remove, since the former one doesn't - work across drives. + """Atomically move src to dst. + + Moving is performed in two stages to make the whole operation atomic in + case src and dst are on different filesystems and actual physical + copying of data is happening. """ self.makedirs(posixpath.dirname(dst)) - try: - self.sftp.rename(src, dst) - except OSError: - self.copy(src, dst) - self.remove(src) + tmp = tmp_fname(dst) + self.copy(src, tmp) + self.sftp.rename(tmp, dst) + self.remove(src) def upload(self, src, dest, no_progress_bar=False, progress_title=None): self.makedirs(posixpath.dirname(dest)) diff --git a/tests/unit/remote/ssh/test_connection.py b/tests/unit/remote/ssh/test_connection.py index f0c12ec219..6b1300ce30 100644 --- a/tests/unit/remote/ssh/test_connection.py +++ b/tests/unit/remote/ssh/test_connection.py @@ -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") From 8f7000056b43c8221f96378f2c5c63ef9217d320 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Fri, 1 Nov 2019 13:26:03 -0600 Subject: [PATCH 3/4] sftp: move -> fallback to copy when sftp.rename fails --- dvc/remote/ssh/connection.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dvc/remote/ssh/connection.py b/dvc/remote/ssh/connection.py index f8a38a0eb8..fa2e8a5ee9 100644 --- a/dvc/remote/ssh/connection.py +++ b/dvc/remote/ssh/connection.py @@ -194,7 +194,12 @@ def move(self, src, dst): self.makedirs(posixpath.dirname(dst)) tmp = tmp_fname(dst) - self.copy(src, tmp) + + try: + self.sftp.rename(src, tmp) + except OSError: + self.copy(src, tmp) + self.sftp.rename(tmp, dst) self.remove(src) From 16164e65ad1f036aca2dc3c408e177af98f8c635 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Fri, 1 Nov 2019 16:56:55 -0600 Subject: [PATCH 4/4] sftp: move -> use atomic copying --- dvc/remote/ssh/connection.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/dvc/remote/ssh/connection.py b/dvc/remote/ssh/connection.py index fa2e8a5ee9..b2da0b67ac 100644 --- a/dvc/remote/ssh/connection.py +++ b/dvc/remote/ssh/connection.py @@ -185,24 +185,28 @@ 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): - """Atomically move src to dst. - - Moving is performed in two stages to make the whole operation atomic in - case src and dst are on different filesystems and actual physical - copying of data is happening. + """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)) - tmp = tmp_fname(dst) - try: - self.sftp.rename(src, tmp) + self.sftp.rename(src, dst) except OSError: - self.copy(src, tmp) + self._atomic_copy(src, dst) - self.sftp.rename(tmp, 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)) tmp_file = tmp_fname(dest)