automatically upload a file when it is put in a given local directory

[davidsarah]

During the Tahoe-LAFS summit, I (David-Sarah) implemented a prototype of a dropbox-like uploader: if you write a file into a given directory, it will upload it to a given Tahoe mutable directory (with the same name as its name in the local filesystem).

Its current limitations are:

• it only handles one directory, not including subdirectories
• the behaviour when multiple files are put in the directory (or the same file more than once) has not been well-tested so far
• it doesn't have unit tests or docs (I will write those)
• it must be given the URI of a mutable directory to upload to; it would be more usable to allow that to be a path that may start with an alias
• it uses the ​twisted.internet.inotify API, which depends on Twisted 10.1 and is only supported on platforms with inotify.

[davidsarah]

Prototype implementation of drop-upload from Tahoe-LAFS summit. No tests or docs. (Corrected to include new file.)

[davidsarah]

​http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1435/dependency-updates.darcs.patch updates the dependency for Twisted to 10.1, which ensures that twisted.internet.inotify is available. (Note that it conflicts with drop-upload.darcs.patch​. I'll rebase the latter when I have tests and docs.)

[david-sarah@…]

In 8b4082677477daf1:

Update the dependency on Twisted to >= 10.1. This allows us to simplify some documentation: it's no longer necessary to install pywin32 on Windows, or apply a patch to Twisted in order to use the FTP frontend. fixes #1274, #1438. refs #1429

[zooko]

Here is a review of attachment:drop-upload.darcs.patch​ .

First of all: yay! This feature is neat. :-)

• What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths.
• Why are we catching Exception--was it just because we might have Twisted < 10.1? We should remove any checks for if we have Twisted < 10.1 (except, of course, for the checks we do for all dependencies in src/allmydata/__init__.py​).
• "FIXME: error reporting" sounds like a serious incomplete part, but what exactly should be added? Logging to the Tahoe-LAFS logging system of certain sorts of failures? What could fail here?
• Perhaps the logging in _notify() is too verbose.
• the "dump event to file" part of _notify() is just for debugging and should be removed.
• "FIXME: what happens if we get another event for the same file while it is uploading?" Well, I looked at the code of dirnode.add_file() and Uploader.upload() and I guess it just redundantly uploads it a second time. (See also #952, which is about what happens when you upload the bytewise-identical file more than once at a time through the WAPI.) What should it do? Well, what should the user experience look like? You drag and drop a file into this directory, and then you drag and drop a new version of it one second later, and then what? Maybe the visual display of the directory should have been updated as soon as you dropped the first one to show that it is in the middle of uploading that file. Then, if you drop another version on it, the display should update to show that there is another update scheduled to happen after this one completes. So I guess we need a new ticket for a GUI frontend to this functionality (integrated into some extant GUI like Nautilus, I suppose). But at this layer, I guess what it should do is schedule another upload for as soon as this upload is finished unless another such upload is already scheduled, in which case it should do nothing.
• "FIXME: should all notified events cause an upload?": from ​http://www.kernel.org/doc/man-pages/online/pages/man7/inotify.7.html and ​http://inotify.aiken.cz/?section=inotify&page=faq&lang=en it looks like we should respond to IN_CLOSE_WRITE | IN_CREATE | IN_MOVE_SELF | IN_MOVE_TO
• Also of course the other FIXMEs deserve to be investigated more. :-)

Other than that, it looks good. It is neat how few lines of code are needed to add this functionality! :-)

[davidsarah]

Replying to zooko:

Here is a review of attachment:drop-upload.darcs.patch​ .

First of all: yay! This feature is neat. :-)

Thanks :-)

• What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths.

tahoe.cfg is encoded in UTF-8, but the path wants to be in sys.getfilesystemencoding().

• Why are we catching Exception--was it just because we might have Twisted < 10.1? We should remove any checks for if we have Twisted < 10.1 (except, of course, for the checks we do for all dependencies in src/allmydata/__init__.py​).

If there is any exception here, the node will silently fail to start. (The same is true of exceptions during initialization of the other frontends.) We should fix that in a better way -- it seems relevant to #355 and #1360 for example -- but I wanted to avoid this failure mode while I was debugging.

• "FIXME: error reporting" sounds like a serious incomplete part, but what exactly should be added? Logging to the Tahoe-LAFS logging system of certain sorts of failures? What could fail here?

We should raise an exception with a proper message (and ensure that gets logged) rather than failing an assert.

• Perhaps the logging in _notify() is too verbose.
• the "dump event to file" part of _notify() is just for debugging and should be removed.

Yes, it was just easier to see the events that way while prototyping. Logging to the Twisted log is sufficient, and the message could be shorter.

• "FIXME: what happens if we get another event for the same file while it is uploading?" Well, I looked at the code of dirnode.add_file() and Uploader.upload() and I guess it just redundantly uploads it a second time.

Yes, I think this is fairly harmless, modulo the fact that we should probably be using a more restrictive event mask.

(See also #952, which is about what happens when you upload the bytewise-identical file more than once at a time through the WAPI.) What should it do? Well, what should the user experience look like? You drag and drop a file into this directory, and then you drag and drop a new version of it one second later, and then what? Maybe the visual display of the directory should have been updated as soon as you dropped the first one to show that it is in the middle of uploading that file. Then, if you drop another version on it, the display should update to show that there is another update scheduled to happen after this one completes. So I guess we need a new ticket for a GUI frontend to this functionality (integrated into some extant GUI like Nautilus, I suppose). But at this layer, I guess what it should do is schedule another upload for as soon as this upload is finished unless another such upload is already scheduled, in which case it should do nothing.

Right, I'll open another ticket about suppressing redundant uploads.

• "FIXME: should all notified events cause an upload?": from ​http://www.kernel.org/doc/man-pages/online/pages/man7/inotify.7.html and ​http://inotify.aiken.cz/?section=inotify&page=faq&lang=en it looks like we should respond to IN_CLOSE_WRITE | IN_CREATE | IN_MOVE_SELF | IN_MOVE_TO

That looks about right, although IN_MOVE_SELF should be treated differently, since it's moving the local directory away from its current path. I don't know what that should do -- probably the safest thing is to disable the drop-upload feature entirely until the node is restarted (and say "don't do that" in the docs).

• Also of course the other FIXMEs deserve to be investigated more. :-)

Other than that, it looks good. It is neat how few lines of code are needed to add this functionality! :-)

Yeah, twisted.internet.inotify rocks!

[davidsarah]

Replying to davidsarah:

Replying to zooko:

• What is the "FIXME: Unicode paths"? I don't see what needs to change there to handle non-ASCII paths.

tahoe.cfg is encoded in UTF-8, but the path wants to be in sys.getfilesystemencoding().

Also, in _notify the conversion to a Tahoe Unicode filename is wrong (unicode(filepath.basename()) will use the default ASCII encoding, when it should be converting from sys.getfilesystemencoding()).

[davidsarah]

Drop-upload frontend, with tests (but no documentation). refs #1429

[davidsarah]

The tests could do with covering a few more corner cases and error conditions, but I'm pretty sure there will be enough time for that before the beta.

[warner]

This is pretty cool stuff. I'm hesitant about landing it in Tahoe core, though.. at least in its present form, it feels more like something that wants to be in a plugin, or in an extensions/ sort of directory. If it were a more complete Dropbox-ish replacment, I'd feel better about it: watching all directories under a root, handling modification of existing files not just new ones, and ideally some kind of multiple-client support. (as is, it's more like an automatic backup tool than a sync-a-virtual-directory-across-multiple-machines tool).

Can you envision maintaining its current UI for a couple years? Or do you think you'll look at it in six months and go "oh, that should really look like X instead".

[davidsarah]

Replying to warner:

This is pretty cool stuff. I'm hesitant about landing it in Tahoe core, though.. at least in its present form, it feels more like something that wants to be in a plugin, or in an extensions/ sort of directory.

We don't have a plugin mechanism. In any case, I'm not encouraged by the fate of things that were previously out-of-core, like the FUSE implementations. I'd prefer it to be in the core, tested by default, and available for all users to try out without installing anything else.

If it introduced new dependencies that would be a different matter, but it doesn't (and we'd decided that it was OK to depend on Twisted 10.1, which has other advantages).

If it were a more complete Dropbox-ish replacment, I'd feel better about it: watching all directories under a root, handling modification of existing files not just new ones, and ideally some kind of multiple-client support. (as is, it's more like an automatic backup tool than a sync-a-virtual-directory-across-multiple-machines tool).

I didn't intend it to be a sync-a-virtual-directory-across-multiple-machines tool. It just uploads things that you drop into a particular directory. Perhaps the name 'drop-upload' suggests that it is more similar to Dropbox than it is. We have time before the 1.9 release to rename it, if a better name is suggested.

It already handles modification of existing files (I'll add that to the tests). Watching all directories under a root is #1433, and is a relatively straightforward evolution of the current code.

Can you envision maintaining its current UI for a couple years?

Yes, absolutely. It'll be easy to maintain compatibility with the current UI, that's only a few lines of code and documentation. It also needs to be in core to get sufficient feedback on the UI to improve it.

[davidsarah]

attachment:drop-upload-3.darcs.patch​ fixes some bugs in error paths, and has better test coverage. The tests also pass on Windows now.

[davidsarah]

drop-upload: make counts visible on the statistics page, and disable some debugging. refs #1429

[davidsarah]

There are some lines > 80 characters in drop-upload-docs.darcs.patch​; I'll rewrap those before committing it.

[nejucomo]

I just read over these ticket comments and I'm about to review each patch in sequence. On success I can say: "This code makes sense to me and the tests work and the docs are clear.", but I cannot say: "We should / should not include this in trunk."

I will bug repository writers in IRC about that policy decision after reviewing the ticket, but before I change "review-needed" to "reviewed" to avoid an unintended 1.9 merge.

[nejucomo]

Fix requirement justification comments for Twisted >= 10.1.0

[nejucomo]

There's a comment collision for attachment:drop-upload.darcs.patch​ in _auto_deps.py which causes no change, just more justifications, for the requirement "Twisted>=10.1.0". I've attached attachment:requirements-comment-merge.darcs.patch​ with the merge of the requirements comments.

[nejucomo]

When applying attachment:drop-upload-2.darcs.patch​ there are merge conflicts in ./src/allmydata/client.py, ./src/allmydata/scripts/create_node.py, and ./src/allmydata/test/test_runner.py.

I'm going to punt understanding and resolving these conflicts back to davidsarah and move on to reviewing other patches.

[davidsarah]

attachment:drop-upload-2.darcs.patch​ is obsolete. Use attachment:drop-upload-4.darcs.patch​ .

[davidsarah]

The wording in attachment:requirements-comment-merge.darcs.patch​ isn't quite equivalent, because the inotify requirement is specific to Linux and the FTP server one is cross-platform. I think the wording as attachment:drop-upload-4.darcs.patch​ leaves it is fine.

[nejucomo]

I am resuming this ticket after I realized later patch attachments supercede earlier ones.

[nejucomo]

In the DropUploader constructor there may be a race condition between:

        if not self._local_path.isdir():
            raise AssertionError("The drop-upload local path %r was not an existing directory." % quote_output(local_dir))

-and later-

        self._notifier.watch(self._local_path, mask=mask, callbacks=[self._notify])

What happens if self._local_path is deleted between these lines?

Would it be better to catch and handle the error of self._local_path not existing in the call to .watch ? (Would that call signal the error?)

I do not consider this issue significant enough to require another iteration on this ticket; but a new ticket may be necessary. I haven't thought of any security problems as of yet, and only a rare usability problem.

[davidsarah]

Replying to nejucomo:

In the DropUploader constructor there may be a race condition between:

        if not self._local_path.isdir():
            raise AssertionError("The drop-upload local path %r was not an existing directory." % quote_output(local_dir))

-and later-

        self._notifier.watch(self._local_path, mask=mask, callbacks=[self._notify])

What happens if self._local_path is deleted between these lines?

If the directory doesn't exist, the call to watch seems to succeed but not work as intended (even if the directory is later created). Similarly if the path points to a file rather than a directory. So the isdir() check is necessary for proper error reporting. In general I agree that it is "Better To Ask Forgiveness Than Permission" as opposed to "Looking Before You Leap", but it doesn't seem to be possible to do that here.

There is no security problem if the directory is deleted, that will just cause there to be no further notifications.

[davidsarah]

Reassigning to warner to decide whether this goes in the beta release.

[nejucomo]

I concur that this is reviewed. davidsarah answered the question about the _local_path race condition in IRC: The inotify interface will not indicate if the target directory does not exist.

It still may be possible to remove the race condition by calling isdir() after the call to watch() but this detail does not seem important enough to prevent inclusion.

[davidsarah]

Proposed NEWS:

New Features
''''''''''''

- A "drop-upload" feature has been added, which allows you to upload
  files to a Tahoe-LAFS directory just by writing them to a local
  directory. This feature is experimental and should not be relied on
  to store the only copy of valuable data. It is currently available
  only on Linux. See `<docs/frontends/drop-upload.rst>`_ for
  documentation. (`#1429`_)

Compatibility and Dependencies
''''''''''''''''''''''''''''''

- The Twisted dependency has been raised to version 10.1. This ensures
  that we no longer require pywin32 on Windows even when using older
  versions of Twisted, that the new drop-upload feature has the required
  support from Twisted on Linux, and that it is never necessary to
  patch Twisted in order to use the FTP frontend.
  (`#1274`_, `#1429`_, `#1438`_)

[zooko]

Brian mentioned on IRC that he was uncertain about including this feature in 1.9. His reasoning persuaded me that we might want to be a bit conservative about distributing this feature in a way that makes people think of it as being a fully supported feature of Tahoe-LAFS. It is new and we haven't thought about it that much, and we don't want to take on the burden of backward compatibility for this feature the way we do for all of the supported features in Tahoe-LAFS.

I think maybe we should go ahead and include this feature in Tahoe-LAFS v1.9 but mark it as an experimental feature which we don't necessarily commit to supporting at this time.

Having a new feature in a release seems like a great way to learn about whether we want to promote it to a fully supported feature in a future release.

If this is agreeable to everyone, we should make sure to flag it as experimental in the release announcement and docs.

[davidsarah]

​http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1431/drop-upload-docs-including-windows.darcs.patch is an alternative docs patch if the Windows implementation is included in 1.9.

[davidsarah]

Documentation for drop-upload frontend. refs #1429

[davidsarah]

Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

[david-sarah@…]

In 32a7717205ed824a:

Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

[david-sarah@…]

In 667b086b59ee37d3:

Documentation for drop-upload frontend. refs #1429

[david-sarah@…]

In 08af9cea50c3c3cd:

test_drop_upload.py: fix unused imports. refs #1429

[davidsarah]

drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429

[zooko]

Reviewed attachment:rename-upload-uri-to-dircap.darcs.patch​. There are some issues. I'm attaching patches that fix a couple of them but not all.

Out of curiosity I rerecorded the patch with darcs record; this helped me find issues in the patch; see below. The resulting patches are in the latest attachment​ -- they redo the renaming with darcs replace and would be an acceptable variant to commit to trunk (also a suitably fixed version with hunks instead of darcs replace would be acceptable to me).

issues fixed by the latest attachment​:

• There was a missed rename in Client.init_drop_uploader​ which would cause an exception if the code were executed.
• In init_drop_uploader​ the local variable could be changed from upload_uri to upload_cap, the way this patch changed such names elsewhere. This is changed by the darcs record rename in the attached patches.
• In DropUploader.__init__ it uses FilePath.is_dir as if it will return False when then thing doesn't exist or the thing is a non-directory, but FilePath.is_dir actually raises exception when the thing doesn't exist. The attached patches fix it to use FilePath.exists and change it to report those two cases separately.

issues not fixed:

• The presence of the missed rename also means that the code in Client.init_drop_uploader isn't exercised by unit tests.
• The "was not valid utf-8" and the "could not be represented in your local filesystem" exceptions could be caught and reported separately in DropUploader.__init__ so that the user would know which one happened. Not necessary for this ticket, but could be nice.

[zooko]

Specifically it is the code inside the "if drop-upload is configured" block in Client.init_drop_uploader. The unit tests instantiate a DropUploader instance directly, but don't test what happens when you instantiate a Client instance which is configured to create a DropUploader.

[zooko]

I use the following bash script (executed by emacs) to generate code coverage results for just test_drop_upload:

cd ~/playground/tahoe-lafs/1429-rerecord
rm -rf ./.cover* ./htmlcov*
coverage run --branch --include="`pwd`/src/*" /usr/local/bin/trial allmydata.test.test_drop_upload

[zooko@…]

In 720bc2433b9bd16d:

(The changeset message doesn't reference this ticket)

[zooko@…]

In 5633375d267e5728:

(The changeset message doesn't reference this ticket)

[zooko@…]

In b7683d9b83a23cdd:

(The changeset message doesn't reference this ticket)

[zooko@…]

In 612abca271703508:

drop-upload: report the configured local directory being absent differently from it being a file
refs #1429

[david-sarah@…]

In 369e30b1dfa2b77f:

test_drop_upload.py: fix some grammatical and spelling nits. refs #1429

[david-sarah@…]

In f157b733676b33f5:

test_drop_upload.py: add comment explaining why we don't use FilePath?.setContent. refs #1429

[david-sarah@…]

In 10ee22f50e5bdfd3:

drop-upload: rename 'start' method to 'startService', which is what you're supposed to use to start a Service. refs #1429

[david-sarah@…]

In c102056ac1df1784:

test_client.py: add a test that the drop-uploader is initialized correctly by client.py. Also give the DropUploader? service a name, which is necessary for the test. refs #1429

[david-sarah@…]

In db22fdc20dc93a3e:

Factor out methods dealing with non-ASCII directories and filenames from test_drop_upload.py into common_util.py. refs #1429, #1472

[david-sarah@…]

In ab9eb12f7006322f:

test_client.py: relax a check in test_create_drop_uploader so that it should pass on Python 2.4.x. refs #1429

[david-sarah@…]

In [5148/ticket393-MDMF-2]:

Drop-upload frontend, rerecorded for 1.9 beta (and correcting a minor mistake). Includes some fixes for Windows but not the Windows inotify implementation. fixes #1429

[david-sarah@…]

In [5149/ticket393-MDMF-2]:

Documentation for drop-upload frontend. refs #1429

[david-sarah@…]

In [5150/ticket393-MDMF-2]:

test_drop_upload.py: fix unused imports. refs #1429

[zooko@…]

In [5155/ticket393-MDMF-2]:

drop-upload: unit tests for the configuration options being named "cap" instead of "uri"
This is a subset of a patch that David-Sarah attached to #1429. This is just the unit-tests part of that patch, and uses darcs record instead of hunks to change the names.
refs #1429

[zooko@…]

In [5156/ticket393-MDMF-2]:

drop-upload test for non-existent local dir separately from test for non-directory local dir
A candidate patch for #1429 has a bug when it is using FilePath?.is_dir() to detect whether the configured local dir exists and is a directory. FilePath?.is_dir() raises exception, instead of returning False, if the thing doesn't exist. This test is to make sure that DropUploader?.init raise different exceptions for those two cases.
refs #1429

[zooko@…]

In [5157/ticket393-MDMF-2]:

drop-upload: rename the 'upload.uri' parameter to 'upload.dircap', and a couple of cleanups to error messages. refs #1429
I rerecorded this patch, originally by David-Sarah, to use "darcs replace" instead of editing to do the renames. This uncovered one missed rename in Client.init_drop_uploader. (Which also means that code isn't exercised by the current unit tests.)
refs #1429

[zooko@…]

In [5158/ticket393-MDMF-2]:

drop-upload: report the configured local directory being absent differently from it being a file
refs #1429

[david-sarah@…]

In [5159/ticket393-MDMF-2]:

test_drop_upload.py: fix some grammatical and spelling nits. refs #1429

[david-sarah@…]

In [5160/ticket393-MDMF-2]:

test_drop_upload.py: add comment explaining why we don't use FilePath?.setContent. refs #1429

[david-sarah@…]

In [5161/ticket393-MDMF-2]:

drop-upload: rename 'start' method to 'startService', which is what you're supposed to use to start a Service. refs #1429

[david-sarah@…]

In [5162/ticket393-MDMF-2]:

test_client.py: add a test that the drop-uploader is initialized correctly by client.py. Also give the DropUploader? service a name, which is necessary for the test. refs #1429

[david-sarah@…]

In [5163/ticket393-MDMF-2]:

Factor out methods dealing with non-ASCII directories and filenames from test_drop_upload.py into common_util.py. refs #1429, #1472

[david-sarah@…]

In [5167/ticket393-MDMF-2]:

test_client.py: relax a check in test_create_drop_uploader so that it should pass on Python 2.4.x. refs #1429

[davidsarah]

Although this was auto-closed for the wrong reason (a commit of the original patch on a branch), it is in fact fixed.

[warner]

just to be clear, this made it into 1.9.0 (released yesterday). It's labeled as "experimental", which means we aren't committed to supporting it long-term yet, and it may get pulled out if it misbehaves :).