Merge lp:~mandel/ubuntuone-client/eq-add-ancestors-watches into lp:ubuntuone-client

[Manuel de la Peña]

Fixes lp:838111

The code moves the addition of the watches for the UDF ancestor to the FilesystemMonitor which is a platform dependent class. The new API is exposed through the eq so that the local rescan can use it. This branch also provides the implementation of Windows which simply ignores the addition of the watches.

Fixes lp:838111

The code moves the addition of the watches for the UDF ancestor to the FilesystemMonitor which is a platform dependent class. The new API is exposed through the eq so that the local rescan can use it. This branch also provides the implementation of Windows which simply ignores the addition of the watches.

This is a new MP so that the code is not blocked du to team holidays. Previous MP can be found here: https://code.launchpad.net/~mandel/ubuntuone-client/eq-add-ancestors-watches/+merge/73775

[Roberto Alsina (ralsina)]

+1

review: Approve

[Alejandro J. Cura (alecu)]

Looks fine!

review: Approve

=== modified file 'tests/platform/linux/test_filesystem_notifications.py'
--- tests/platform/linux/test_filesystem_notifications.py	2011-08-25 17:31:02 +0000
+++ tests/platform/linux/test_filesystem_notifications.py	2011-09-05 13:52:23 +0000
@@ -37,6 +37,17 @@
 # pylint: disable=W0212
 
 
+class FakeVolume(object):
+    """A fake volume."""
+
+    def __init__(self, path, ancestors):
+        """Create a new instance."""
+        super(FakeVolume, self).__init__()
+        self.volume_id = path
+        self.path = path
+        self.ancestors = ancestors
+
+
 class ShutdownTestCase(testcase.BaseTwistedTestCase):
     """Test the monitor shutdown."""
 
@@ -288,6 +299,41 @@
         self.assertIn(self.root_dir, self.monitor._general_watchs)
 
     @defer.inlineCallbacks
+    def test_add_watches_to_udf_ancestors(self):
+        """Test that the ancestor watches are added."""
+        ancestors = ['~', '~/Picture', '~/Pictures/Work']
+        path = 'test_path'
+        volume = FakeVolume(path, ancestors)
+        self.patch(filesystem_notifications, 'access', lambda path: True)
+        self.patch(self.monitor, '_is_udf_ancestor', lambda path: True)
+        # lets add the watches, ensure that we do return true and that the new
+        # watches are indeed present.
+        added = yield self.monitor.add_watches_to_udf_ancestors(volume)
+        self.assertTrue(added, 'Watches should have been added.')
+        for path in ancestors:
+            self.assertTrue(self.log_handler.check_debug(
+                "Adding ancestors inotify watch", path))
+            self.assertIn(path, self.monitor._ancestors_watchs)
+
+    @defer.inlineCallbacks
+    def test_add_watches_to_udf_ancestors_reverted(self):
+        """Test that the ancestor watches are reverted."""
+        ancestors = ['~', '~/Picture', '~/Pictures/Work']
+        path = 'test_path'
+        volume = FakeVolume(path, ancestors)
+        self.patch(filesystem_notifications, 'access',
+                   lambda path: path != ancestors[2])
+        self.patch(self.monitor, '_is_udf_ancestor', lambda path: True)
+        # lets ensure that we did not added any of the watches.
+        added = yield self.monitor.add_watches_to_udf_ancestors(volume)
+        self.assertFalse(added, 'Watches should NOT have been added.')
+        for path in ancestors:
+            if path != ancestors[2]:
+                self.assertTrue(self.log_handler.check_debug(
+                    "Adding ancestors inotify watch", path))
+            self.assertNotIn(path, self.monitor._ancestors_watchs)
+
+    @defer.inlineCallbacks
     def test_add_watch_on_udf_ancestor(self):
         """Test that ancestors watchs can be added."""
         # create the udf and add the watch

=== modified file 'tests/platform/windows/test_filesystem_notifications.py'
--- tests/platform/windows/test_filesystem_notifications.py	2011-08-17 19:24:52 +0000
+++ tests/platform/windows/test_filesystem_notifications.py	2011-09-05 13:52:23 +0000
@@ -1315,3 +1315,23 @@
 
         self.assertTrue(d1.called, "Should already be called.")
         self.assertTrue(d2.called, "Should already be called.")
+
+    @defer.inlineCallbacks
+    def test_add_watches_to_udf_ancestors(self):
+        """Test that the ancestor watches are not added."""
+
+        class FakeVolume(object):
+            """A fake UDF."""
+
+            def __init__(self, ancestors):
+                """Create a new instance."""
+                self.ancestors = ancestors
+
+        ancestors = ['~', '~\\Pictures', '~\\Pictures\\Home',]
+        volume = FakeVolume(ancestors)
+        monitor = FilesystemMonitor(None, None)
+        added = yield monitor.add_watches_to_udf_ancestors(volume)
+        self.assertTrue(added, 'We should always return true.')
+        # lets ensure that we never added the watches
+        self.assertEqual(0, len(monitor._watch_manager._wdm.values()),
+                         'No watches should have been added.')

=== modified file 'tests/syncdaemon/test_eq_inotify.py'
--- tests/syncdaemon/test_eq_inotify.py	2011-08-25 18:27:25 +0000
+++ tests/syncdaemon/test_eq_inotify.py	2011-09-05 13:52:23 +0000
@@ -1,6 +1,7 @@
 # tests.syncdaemon.test_eq_inotify
 #
-# Author: Facundo Batista <[e-mail]>
+# Authors: Facundo Batista <[e-mail]>
+#          Manuel de la Pena <[e-mail]>
 #
 # Copyright 2009-2011 Canonical Ltd.
 #
@@ -45,7 +46,7 @@
     set_file_readwrite,
     set_dir_readwrite,
 )
-from ubuntuone.syncdaemon import volume_manager
+from ubuntuone.syncdaemon import event_queue, volume_manager
 
 # our logging level
 TRACE = logging.getLevelName('TRACE')
@@ -83,6 +84,41 @@
         self.assertEqual(called, [method_arg])
         self.assertEqual(res, method_resp)
 
+    def test_add_watches_to_udf_ancestors(self):
+        """Test that ancestors watches can be added."""
+        called = []
+        method_resp = True
+        method_arg = object()
+
+        def add_watches_to_udf_ancestors(path):
+            """Fake it."""
+            called.append(path)
+            return defer.succeed(method_resp)
+
+        self.patch(self.eq.monitor, 'add_watches_to_udf_ancestors',
+                   add_watches_to_udf_ancestors)
+        res = yield self.eq.add_watches_to_udf_ancestors(method_arg)
+        self.assertEqual(called, [method_arg])
+        self.assertEqual(res, method_resp)
+
+    def test_add_watches_to_udf_ancestors_no_access(self):
+        """Test that ancestors watches are not added."""
+        called = []
+        method_resp = True
+        method_arg = object()
+
+        def add_watches_to_udf_ancestors(path):
+            """Fake it."""
+            called.append(path)
+            return defer.succeed(method_resp)
+
+        self.patch(event_queue, 'access', lambda path: False)
+        self.pathc(self.eq.monitor, 'add_watches_to_udf_ancestors',
+                   add_watches_to_udf_ancestors)
+        added = yield self.eq.add_watches_to_udf_ancestors(method_arg)
+        self.assertFalse(added, 'Watches should have not been added.')
+        self.assertEqual(0, len(called))
+
     def test_rm_watch(self):
         """Test that watchs can be removed."""
         called = []

=== modified file 'tests/syncdaemon/test_localrescan.py'
--- tests/syncdaemon/test_localrescan.py	2011-08-30 18:04:23 +0000
+++ tests/syncdaemon/test_localrescan.py	2011-09-05 13:52:23 +0000
@@ -1,5 +1,6 @@
 # Author: Facundo Batista <[e-mail]>
 # Author: Natalia Bidart <[e-mail]>
+# Author: Manuel de la Pena <[e-mail]>
 #
 # Copyright 2009-2011 Canonical Ltd.
 #
@@ -76,6 +77,10 @@
     add_watch = freeze_rollback = is_frozen = _fake
     rm_watch = freeze_begin = add_to_mute_filter = _fake
 
+    def add_watches_to_udf_ancestors(self, volume):
+        """Fake ancestors addition."""
+        return defer.succeed(True)
+
     def freeze_commit(self, events):
         """just store events"""
         self.pushed.extend(events)
@@ -2237,7 +2242,17 @@
                 self.watches.append(path)
                 return defer.succeed(True)
 
+        @defer.inlineCallbacks
+        def fake_add_watches_to_udf_ancestors(volume):
+            """Fake the addition of the ancestors watches."""
+            for ancestor in volume.ancestors:
+                self._logger.debug("Adding watch to UDF's %r", ancestor)
+                yield fake_add(ancestor)
+            defer.returnValue(True)
+
         self.patch(self.eq, 'add_watch', fake_add)
+        self.patch(self.eq, 'add_watches_to_udf_ancestors',
+                   fake_add_watches_to_udf_ancestors)
 
         self.lr = local_rescan.LocalRescan(self.vm, self.fsm, self.eq, self.aq)
 

=== modified file 'ubuntuone/platform/linux/filesystem_notifications.py'
--- ubuntuone/platform/linux/filesystem_notifications.py	2011-08-25 17:31:02 +0000
+++ ubuntuone/platform/linux/filesystem_notifications.py	2011-09-05 13:52:23 +0000
@@ -1,6 +1,7 @@
 # ubuntuone.syncdaemon.event_queue - Event queuing
 #
-# Author: Facundo Batista <[e-mail]>
+# Authors: Facundo Batista <[e-mail]>
+#          Manuel de la Pena <[e-mail]>
 #
 # Copyright 2009-2011 Canonical Ltd.
 #
@@ -23,6 +24,7 @@
 
 import pyinotify
 from twisted.internet import abstract, reactor, error, defer
+from ubuntuone.platform.linux.os_helper import access
 
 
 # translates quickly the event and it's is_dir state to our standard events
@@ -472,6 +474,32 @@
         w_dict[dirpath] = result[dirpath]
         return defer.succeed(True)
 
+    @defer.inlineCallbacks
+    def add_watches_to_udf_ancestors(self, volume):
+        """Add a inotify watch to volume's ancestors if it's an UDF."""
+        added_watches = []
+
+        def revert_watches():
+            """Restore the just added watches and unsubscribe volume."""
+            for path in added_watches:
+                self.eq.rm_watch(path)
+
+        for ancestor in volume.ancestors:
+            # check that ancestor is still there
+            if not access(ancestor):
+                self.log.info("Tree broken at path: %r", volume.path)
+                revert_watches()
+                defer.returnValue(False)
+
+            self.log.debug("Adding watch to UDF's ancestor %r", ancestor)
+            really_added = yield self.eq.add_watch(ancestor)
+            # only note it for the revert if the watch was not there before
+            if really_added:
+                added_watches.append(ancestor)
+
+        # all is ok
+        defer.returnValue(True)
+
     def inotify_watch_fix(self, pathfrom, pathto):
         """Fix the path in inotify structures."""
         if pathfrom in self._general_watchs:

=== modified file 'ubuntuone/platform/windows/filesystem_notifications.py'
--- ubuntuone/platform/windows/filesystem_notifications.py	2011-08-15 20:00:36 +0000
+++ ubuntuone/platform/windows/filesystem_notifications.py	2011-09-05 13:52:23 +0000
@@ -734,6 +734,14 @@
         return self._watch_manager.add_watch(dirpath,
                              FILESYSTEM_MONITOR_MASK, auto_add=True)
 
+    def add_watches_to_udf_ancestors(self, volume):
+        """Add a inotify watch to volume's ancestors if it's an UDF."""
+        # On windows there is no need to add the watches to the ancestors
+        # so we will always return true. The reason for this is that the
+        # handles that we open stop the user from renaming the ancestors of
+        # the UDF, for a user to do that he has to unsync the udf first
+        return defer.succeed(True)
+
     def is_frozen(self):
         """Checks if there's something frozen."""
         return self._processor.frozen_path is not None

=== modified file 'ubuntuone/syncdaemon/event_queue.py'
--- ubuntuone/syncdaemon/event_queue.py	2011-08-17 13:11:06 +0000
+++ ubuntuone/syncdaemon/event_queue.py	2011-09-05 13:52:23 +0000
@@ -1,6 +1,7 @@
 # ubuntuone.syncdaemon.event_queue - Event queuing
 #
-# Author: Facundo Batista <[e-mail]>
+# Authors: Facundo Batista <[e-mail]>
+#          Manuel de la Pena <[e-mail]>
 #
 # Copyright 2009-2011 Canonical Ltd.
 #
@@ -23,7 +24,7 @@
 
 from twisted.internet import defer
 
-from ubuntuone.platform import FilesystemMonitor
+from ubuntuone.platform import access, FilesystemMonitor
 
 # these are our internal events, what is inserted into the whole system
 EVENTS = {
@@ -222,6 +223,18 @@
         """Add watch to a dir."""
         return self.monitor.add_watch(dirpath)
 
+    def add_watches_to_udf_ancestors(self, volume):
+        """Add a inotify watch to volume's ancestors if it's an UDF."""
+        # This is a platform dependent operation since there are cases in
+        # which the watches do not have to be added (On windows we do not
+        # have to add them since we have an opened handle.)
+        # finally, check that UDF is ok in disk
+        if not access(volume.path):
+            # if we cannot access the UDF lets return false and do
+            # nothing
+            return defer.succeed(False)
+        return self.monitor.add_watches_to_udf_ancestors(volume)
+
     def unsubscribe(self, obj):
         """Remove the callback object from the listener queue.
 

=== modified file 'ubuntuone/syncdaemon/local_rescan.py'
--- ubuntuone/syncdaemon/local_rescan.py	2011-08-22 14:20:30 +0000
+++ ubuntuone/syncdaemon/local_rescan.py	2011-09-05 13:52:23 +0000
@@ -259,40 +259,6 @@
             self._process_next_queue(None)
         return self._previous_deferred
 
-    @defer.inlineCallbacks
-    def _add_watches_to_udf_ancestors(self, volume):
-        """Add a inotify watch to volume's ancestors if it's an UDF."""
-        added_watches = []
-
-        def revert_watches():
-            """Restore the just added watches and unsubscribe volume."""
-            for path in added_watches:
-                self.eq.rm_watch(path)
-            m = "Unsubscribing UDF %r because not in disk: %r"
-            log_info(m, volume.volume_id, volume.path)
-            self.vm.unsubscribe_udf(volume.volume_id)
-
-        for ancestor in volume.ancestors:
-            # check that ancestor is still there
-            if not access(ancestor):
-                log_info("Tree broken at path: %r", volume.path)
-                revert_watches()
-                defer.returnValue(False)
-
-            log_debug("Adding watch to UDF's ancestor %r", ancestor)
-            really_added = yield self.eq.add_watch(ancestor)
-            # only note it for the revert if the watch was not there before
-            if really_added:
-                added_watches.append(ancestor)
-
-        # finally, check that UDF is ok in disk
-        if not access(volume.path):
-            revert_watches()
-            defer.returnValue(False)
-
-        # all is ok
-        defer.returnValue(True)
-
     def _process_next_queue(self, _):
         """Process the next item in the queue, if any."""
         log_debug("process next in queue (len %d)", len(self._queue))
@@ -312,8 +278,13 @@
                 # add watches to UDF ancestors and check UDF is ok
                 volume = scan_info[0]
                 if isinstance(volume, volume_manager.UDF):
-                    udf_ok = yield self._add_watches_to_udf_ancestors(volume)
+                    udf_ok = yield self.eq.add_watches_to_udf_ancestors(volume)
                     if not udf_ok:
+                        # we need to ensure that the udf is not subscribed
+                        # when an error happens while adding the parent watches.
+                        m = "Unsubscribing UDF %r because not in disk: %r"
+                        log_info(m, volume.volume_id, volume.path)
+                        self.vm.unsubscribe_udf(volume.volume_id)
                         self._process_next_queue(None)
                         return