Merge lp:~verterok/ubuntuone-client/fix-776386 into lp:ubuntuone-client

[Guillermo Gonzalez]

Fix Tritcask._find_data_files method to handle the case of broken/corrupted data files (when opening the file).

This branch fix Tritcask._find_data_files method to handle the case of broken/corrupted data files (when opening the file).
It basically rename any broken "immutable" data file doing: s/".inactive"/".broken"/

[Facundo Batista (facundo)]

Nice!

review: Approve

=== modified file 'tests/syncdaemon/test_tritcask.py'
--- tests/syncdaemon/test_tritcask.py	2011-08-30 17:16:49 +0000
+++ tests/syncdaemon/test_tritcask.py	2011-09-15 19:58:33 +0000
@@ -38,9 +38,11 @@
     HINT,
     INACTIVE,
     DEAD,
+    BROKEN,
     BadCrc,
     BadHeader,
     DataFile,
+    ImmutableDataFile,
     TempDataFile,
     DeadDataFile,
     HintFile,
@@ -869,6 +871,52 @@
                                               key=attrgetter('filename'))]
         self.assertEqual(files, filenames)
 
+    def test_find_data_files_immutable_open_error(self):
+        """Test the _find_data_files method failing to open a file."""
+        data_file = DataFile(self.base_dir)
+        data_file.write(0, 'foo_0', 'bar_0')
+        immutable_file = data_file.make_immutable()
+        immutable_file.close()
+        # patch the open call and make it fail
+        def fail_open(filename, *a):
+            """Always fail."""
+            raise IOError("I'm a broken file.")
+        self.patch(ImmutableDataFile, '_open', fail_open)
+        db = Tritcask(self.base_dir)
+        self.addCleanup(db.shutdown)
+        files = [fn.filename for fn in sorted(db._immutable.values(),
+                                              key=attrgetter('filename'))]
+        self.assertNotIn(immutable_file.filename, files)
+        # check the logs
+        msg = ("Failed to open %s, renaming it to: %s - error: %s" % \
+               (immutable_file.filename,
+                immutable_file.filename.replace(INACTIVE, BROKEN),
+                IOError("I'm a broken file")))
+        self.assertTrue(self.memento.check_warning(msg))
+
+    def test_find_data_files_live_open_error(self):
+        """Test the _find_data_files method failing to open a file."""
+        data_file = DataFile(self.base_dir)
+        data_file.write(0, 'foo_0', 'bar_0')
+        data_file.close()
+        # patch the open call and make it fail
+        orig_open = DataFile._open
+        def fail_open(filename, *a):
+            """Always fail only once."""
+            self.patch(DataFile, '_open', orig_open)
+            raise IOError("I'm a broken file.")
+        self.patch(DataFile, '_open', fail_open)
+        db = Tritcask(self.base_dir)
+        self.addCleanup(db.shutdown)
+        self.assertNotEqual(data_file.filename, db.live_file.filename)
+        # check the logs
+        self.memento.debug = True
+        msg = ("Failed to open %s, renaming it to: %s - error: %s" % \
+               (data_file.filename,
+                data_file.filename.replace(LIVE, BROKEN),
+                IOError("I'm a broken file")))
+        self.assertTrue(self.memento.check_warning(msg))
+
     def test_build_keydir_on_init(self):
         """Test _build_keydir method."""
         db = Tritcask(self.base_dir)

=== modified file 'ubuntuone/syncdaemon/tritcask.py'
--- ubuntuone/syncdaemon/tritcask.py	2011-08-23 15:48:54 +0000
+++ ubuntuone/syncdaemon/tritcask.py	2011-09-15 19:58:33 +0000
@@ -55,6 +55,7 @@
 INACTIVE = '.inactive'
 HINT = '.hint'
 DEAD = '.dead'
+BROKEN = '.broken'
 
 VERSION = 'v1'
 FILE_SUFFIX = '.tritcask-%s.data' % VERSION
@@ -73,6 +74,7 @@
 class BadHeader(Exception):
     """A Exception for Bad header value."""
 
+
 TritcaskEntry = namedtuple('TritcaskEntry', ['crc32', 'tstamp', 'key_sz',
                          'value_sz', 'row_type', 'key', 'value', 'value_pos'])
 
@@ -172,12 +174,12 @@
         """
         self.has_bad_crc = False
         self.has_bad_data = False
+        self.fd = None
         if filename is None:
             filename = self._get_next_file_id() + LIVE + FILE_SUFFIX
         self.filename = os.path.join(base_path, filename)
         self.file_id = _get_file_id(filename)
         self.hint_filename = os.path.join(base_path, filename + HINT)
-        self.fd = None
         self._open()
 
     @property
@@ -681,6 +683,7 @@
         """Collect the files we need to work with."""
         logger.debug("lookingup data files")
         dead_files = 0
+        broken_files = 0
         # becuase listdir appends / at the end of the path if there is not
         # os.path.sep at the end we are going to add it, otherwhise on windows
         # we will have wrong paths formed
@@ -689,21 +692,37 @@
             base_path += os.path.sep
         files = os.listdir(self.base_path)
         for filename in files:
+            # first check for hint and dead
             if is_hint(filename):
                 continue
-            if is_live(filename):
-                self.live_file = DataFile(self.base_path, filename)
-            elif is_immutable(filename):
-                # it's an immutable file
-                data_file = ImmutableDataFile(self.base_path, filename)
-                self._immutable[data_file.file_id] = data_file
             elif is_dead(filename):
                 # a dead file...let's remove it
                 dead_files += 1
                 DeadDataFile(self.base_path, filename).delete()
+                continue
+            # if it's a live or immutable file try to open it, but if it's
+            # "broken", just rename it and continue
+            try:
+                if is_live(filename):
+                    self.live_file = DataFile(self.base_path, filename)
+                elif is_immutable(filename):
+                    # it's an immutable file
+                    data_file = ImmutableDataFile(self.base_path, filename)
+                    self._immutable[data_file.file_id] = data_file
+            except IOError, e:
+                # oops, failed to open the file..discard it
+                broken_files += 1
+                orig = os.path.join(self.base_path, filename)
+                # get the kind of the file so we can rename it
+                kind = LIVE if is_live(filename) else INACTIVE
+                dest = orig.replace(kind, BROKEN)
+                logger.warning("Failed to open %s, renaming it to: %s - "
+                               "error: %s", orig, dest, e)
+                # rename it to "broken"
+                os.rename(orig, dest)
         # immutable files + live
-        logger.info("found %s data files and %s dead files",
-                    len(self._immutable)+1, dead_files)
+        logger.info("found %s data files, %s dead and %s broken files",
+                    len(self._immutable)+1, dead_files, broken_files)
 
     def rotate(self, create_file=True):
         """Rotate the live file only if it already exsits."""