sftp: make move across drives possible #2709
Conversation
ghost
mentioned this pull request
shcheklein
left a comment
shcheklein
left a comment
There was a problem hiding this comment.
a few comments + can we add a test for this please?
ghost
dismissed
shcheklein’s stale review
thanks for the review, @shcheklein, instead of fallback to an atomic operation, now always moves in two steps
|
Not sure how to mock across drives operations, tho 🤔 |
|
I think that I should return to the previous implementation, fall back only when |
dvc/remote/ssh/connection.py
Outdated
| self.makedirs(posixpath.dirname(dst)) | ||
| self.sftp.rename(src, dst) | ||
|
|
||
| tmp = tmp_fname(dst) |
There was a problem hiding this comment.
is there something like with tmp ... to make sure we cleanup the stuff if there is some failure in the middle
There was a problem hiding this comment.
@shcheklein , we can do finally on the try/except block 👍
There was a problem hiding this comment.
yeah, I was just thinking that there might be something like with tmp already - since it makes a lot of sense and simplifies + deduplicates the code ... but finally works fine either
There was a problem hiding this comment.
@shcheklein , but what's the point of doing a two step operation if we are always removing the tmp? Isn't it useful to rollback the src when deleted or at least leave the tmp?
There was a problem hiding this comment.
not sure I got the question, @MrOutis . My only point is that if something fails in the middle we don't want to stuck in some weird state (thus atomicity), and we don't want to leave some garbage after us that won't be controlled by anyone and can consume a lot of space (thus with tmp ... or finally or whatnot). with smt is just a way of achieving the second requirement
| self.sftp.rename(src, dst) | ||
|
|
||
| try: | ||
| self.sftp.rename(src, dst) |
There was a problem hiding this comment.
are you 100% sure that rename is never copying data? just to double-check
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 👍
2 participants
Fix #2704