Merge lp:~verterok/ubuntuone-client/kill-the-zombie-share into lp:ubuntuone-client

[Guillermo Gonzalez]

[r=jblount, r=statik] delete share/d metadata if the share was deleted/canceled (this is done when a new share list is received)

[Guillermo Gonzalez (verterok)]

This branch adds some housekeeping code to the SHARE_LIST handler in VolumeManager, in order to delete dead/orphaned shares.

[Guillermo Gonzalez (verterok)]

This is the first branch of two in order to fix Bug #383760

[Elliot Murphy (statik)]

review: Approve

[Joshua Blount (jblount)]

review: Approve

=== modified file 'canonical/ubuntuone/storage/syncdaemon/filesystem_manager.py'
--- canonical/ubuntuone/storage/syncdaemon/filesystem_manager.py	2009-05-19 14:11:44 +0000
+++ canonical/ubuntuone/storage/syncdaemon/filesystem_manager.py	2009-06-18 01:31:04 +0000
@@ -544,7 +544,7 @@
                     # it that, :(. We handle this here because it's possible
                     # and correct that the path is not there anymore
                     m = "Error %s when trying to remove the watch on %r"
-                    log_warning(m, path)
+                    log_warning(m, e, path)
 
             self.delete_metadata(p)
         mdobj["info"]["last_conflicted"] = time.time()

=== modified file 'canonical/ubuntuone/storage/syncdaemon/volume_manager.py'
--- canonical/ubuntuone/storage/syncdaemon/volume_manager.py	2009-05-12 13:36:05 +0000
+++ canonical/ubuntuone/storage/syncdaemon/volume_manager.py	2009-06-18 14:47:31 +0000
@@ -23,6 +23,7 @@
 import shutil
 import stat
 from contextlib import contextmanager
+from itertools import ifilter
 
 from canonical.ubuntuone.storage.syncdaemon.marker import MDMarker
 from canonical.ubuntuone.storage.syncdaemon import file_shelf
@@ -175,18 +176,21 @@
 
     def handle_AQ_SHARES_LIST(self, shares_list):
         """ handle AQ_SHARES_LIST event """
-        for share in shares_list.shares:
-            share_id = getattr(share, 'id', getattr(share, 'share_id', None))
-            self.log.debug('new share %r: id=%s, name=%r',
-                                        share.direction, share_id, share.name)
-
+        self.log.debug('handling shares list: ')
         self.list_shares_retries = 0
+        shares = []
+        shared = []
         for a_share in shares_list.shares:
+            share_id = getattr(a_share, 'id',
+                               getattr(a_share, 'share_id', None))
+            self.log.debug('share %r: id=%s, name=%r', a_share.direction,
+                           share_id, a_share.name)
             if a_share.direction == "to_me":
                 dir_name = self._build_dirname(a_share.name,
                                                a_share.other_visible_name)
                 path = os.path.join(self.m.shares_dir, dir_name)
                 share = Share.from_response(a_share, path)
+                shares.append(share.id)
                 self.add_share(share)
             elif a_share.direction == "from_me":
                 try:
@@ -199,8 +203,21 @@
                             " but don't have the node_id in the metadata yet")
                     path = None
                 share = Share.from_response(a_share, path)
+                shared.append(share.id)
                 self.add_shared(share)
 
+        # housekeeping of the shares and shared shelf's each time we get the
+        # list of shares
+        self.log.debug('deleting dead shares')
+        for share in ifilter(lambda item: item and item not in shares,
+                             self.shares):
+            self.log.debug('deleting share: id=%s', share)
+            self.share_deleted(share)
+        for share in ifilter(lambda item: item and item not in shared,
+                             self.shared):
+            self.log.debug('deleting shared: id=%s', share)
+            del self.shared[share]
+
     def _build_dirname(self, share_name, visible_name):
         '''Builds the root path using the share information.'''
         dir_name = share_name + u' from ' + visible_name
@@ -210,7 +227,6 @@
         dir_name = dir_name.encode("utf8")
         return dir_name
 
-
     def handle_AQ_LIST_SHARES_ERROR(self, error):
         """ handle AQ_LIST_SHARES_ERROR event """
         # just call list_shares again, until we reach the retry limit
@@ -268,6 +284,8 @@
         """ Add a share to the share list, and creates the fs mdobj. """
         self.log.info('Adding new share with id: %s - path: %r',
                       share.id, share.path)
+        if share.id in self.shares:
+            del self.shares[share.id]
         self.shares[share.id] = share
         if share.accepted:
             self._create_fsm_object(share)
@@ -294,8 +312,6 @@
 
     def share_deleted(self, share_id):
         """ process the share deleted event. """
-        # XXX: verterok: shares deletion is not fully implemented,
-        # the files aren't deleted from disk
         self.log.debug("Share (id: %s) deleted. ", share_id)
         share = self.shares.get(share_id, None)
         if share is None:
@@ -343,9 +359,21 @@
                                                           server_hash=None,))
 
     def _delete_fsm_object(self, share):
-        """ Deletes the share from fsm. """
-        #XXX: not implemented yet, this ;ll delegate most of the work in fsm
-        pass
+        """ Deletes the share and it files/folders metadata from fsm. """
+        #XXX: partially implemented, this should be moved into fsm?.
+        # should delete all the files in the share?
+        if share.can_write():
+            try:
+                self.m.event_q.inotify_rm_watch(share.path)
+            except (ValueError, RuntimeError, TypeError), e:
+                # pyinotify has an ugly error management, if we can call
+                # it that, :(. We handle this here because it's possible
+                # and correct that the path is not there anymore
+                self.log.warning("Error %s when trying to remove the watch"
+                                 " on %r", e, share.path)
+        # delete all the metadata but dont touch the files/folders
+        for path, is_dir in self.m.fs.get_paths_starting_with(share.path):
+            self.m.fs.delete_metadata(path)
 
     def create_share(self, path, username, name, access_level):
         """ create a share for the specified path, username, name """

=== modified file 'tests/syncdaemon/test_vm.py'
--- tests/syncdaemon/test_vm.py	2009-05-12 17:34:36 +0000
+++ tests/syncdaemon/test_vm.py	2009-06-18 01:31:04 +0000
@@ -322,6 +322,45 @@
         self.vm.handle_SV_SHARE_ANSWERED('share_id', 'Yes')
         self.assertNotIn('share_id', self.vm.shared)
 
+    def test_delete_share(self):
+        share_response = ShareResponse.from_params('share_id', 'to_me',
+                                                   'fake_share_uuid',
+                                                   'fake_share', 'username',
+                                                   'visible_username', 'yes',
+                                                   'View')
+        share_response_1 = ShareResponse.from_params('share_id_1', 'to_me',
+                                                   'fake_share_uuid_1',
+                                                   'fake_share_1', 'username',
+                                                   'visible_username', 'yes',
+                                                   'View')
+        # initialize the the root
+        self.vm.on_server_root('root_uuid')
+        response = ListShares(None)
+        response.shares = [share_response, share_response_1]
+        self.vm.handle_AQ_SHARES_LIST(response)
+        self.assertEquals(3, len(self.vm.shares)) # the new shares and root
+        # check that the share is in the shares dict
+        self.assertTrue('share_id' in self.vm.shares)
+        self.assertTrue('share_id_1' in self.vm.shares)
+        share = self.vm.shares['share_id']
+        self.assertEquals('fake_share', share.name)
+        self.assertEquals('fake_share_uuid', share.subtree)
+        share = self.vm.shares['share_id_1']
+        self.assertEquals('fake_share_1', share.name)
+        self.assertEquals('fake_share_uuid_1', share.subtree)
+        # inject a new ListShares response
+        new_response = ListShares(None)
+        new_response.shares = [share_response]
+        self.vm.handle_AQ_SHARES_LIST(new_response)
+        # check that the missing share was removed
+        self.assertTrue('share_id' in self.vm.shares)
+        self.assertFalse('share_id_1' in self.vm.shares)
+        share = self.vm.shares['share_id']
+        self.assertEquals('fake_share', share.name)
+        self.assertEquals('fake_share_uuid', share.subtree)
+        self.assertEquals(None, self.vm.shares.get('share_id_1'))
+
+
 class VolumeManagerUnicodeTests(BaseVolumeManagerTests):
     """Tests for Volume Manager unicode capabilities."""