Merge lp:~facundo/ubuntuone-client/stable--long-partial-filenames into lp:ubuntuone-client/stable-1-4

[Facundo Batista]

Handle better errors on partial creation. (LP: #657195, LP: #662660)

Handle better errors on partial creation.

This has two sides:

- Leave the node in a better state if *any* error happens when touching disk: in this case, I moved the MD handling before the filesystem handling. So, from now, if something bad happens when creating the partial in the disk, the node will still be in SERVER state, and LR will re-download it.

- Support too long filenames: for this, specific code were added to the filename handling when the file is created: we trim the filename until the filesystem stops giving that error to us.

As the filename may be trimmed according to filesystem errors, it's not fully predictable any more. So, when that happens, it's stored in the metadata object.

Tests included for everything.

[Guillermo Gonzalez (verterok)]

nice, tests pass.

review: Approve

[John O'Brien (jdobrien)]

Looks good...

Should we worry about filename = filename[:-10*trim] resulting in an index out of range if it evaluates to longer than the filename? I guess it's an internal function so perhaps not.

review: Approve

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2010-09-15 19:02:52 +0000
+++ tests/syncdaemon/test_fsm.py	2010-10-25 19:56:06 +0000
@@ -20,10 +20,11 @@
 
 from __future__ import with_statement
 
+import errno
 import os
 import shutil
+import time
 import unittest
-import time
 
 from contrib.testing.testcase import (
     FakeVolumeManager,
@@ -1584,7 +1585,6 @@
         # it has no partial!
         self.fsm.remove_partial("uuid", "share")
 
-
     def test_create_dir_previous(self):
         """Test create .partial for a dir when the dir existed."""
         testdir = os.path.join(self.share_path, "path")
@@ -1697,6 +1697,129 @@
         self.fsm.create_partial("uuid1", "share")
         self.assertTrue(self.fsm.get_by_mdid(mdid).info.is_partial)
 
+    def test_disk_problem(self):
+        """On any disk problem, the node should be left in partial state."""
+        testfile = os.path.join(self.share_path, "path")
+        mdid = self.fsm.create(testfile, "share")
+        self.fsm.set_node_id(testfile, "uuid")
+
+        # ugly problem not handled
+        os.rmdir(self.partials_dir)
+        try:
+            self.fsm.create_partial("uuid", "share")
+        except IOError, e:
+            if e.errno == errno.ENOENT:
+                # expected
+                pass
+        else:
+            raise
+
+        # the node should still be in partial internally
+        # for LR to handle it ok
+        mdobj = self.fsm.get_by_mdid(mdid)
+        self.assertTrue(mdobj.info.is_partial)
+
+    def test_filename_too_long(self):
+        """Handle the filename being too long."""
+        # find a almost too long file
+        repeat = 300
+        while True:
+            testfile = os.path.join(self.share_path, "x"*repeat)
+            try:
+                fh = open(testfile, 'w')
+            except IOError, e:
+                if e.errno == errno.ENAMETOOLONG:
+                    repeat -= 10
+            else:
+                fh.close()
+                os.unlink(testfile)
+                break
+
+        mdid = self.fsm.create(testfile, "share")
+        self.fsm.set_node_id(testfile, "uuid")
+
+        # create partial ok, even knowing that "full partial path" won't fit
+        self.fsm.create_partial("uuid", "share")
+        self.assertTrue(self.fsm.get_by_mdid(mdid).info.is_partial)
+
+        # check the path
+        partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid])
+        self.assertTrue(os.path.exists(partial_path))
+
+    def test_get_partial_path_notrim(self):
+        """Create the partial path."""
+        testfile = os.path.join(self.share_path, "foo")
+        mdid = self.fsm.create(testfile, "share")
+        partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid])
+        partial_name = os.path.basename(partial_path)
+        self.assertEqual(partial_name, mdid + ".u1partial.foo")
+
+    def test_get_partial_path_trim1(self):
+        """Create the partial path trimmed some chars."""
+        longname = "longnamethatistoolong"
+        testfile = os.path.join(self.share_path, longname)
+        mdid = self.fsm.create(testfile, "share")
+        partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid], trim=1)
+        partial_name = os.path.basename(partial_path)
+        self.assertEqual(partial_name, mdid + ".u1partial." + longname[:-10])
+
+    def test_get_partial_path_trim2(self):
+        """Create the partial path trimmed more chars."""
+        longname = "longnamethatistoolong"
+        testfile = os.path.join(self.share_path, longname)
+        mdid = self.fsm.create(testfile, "share")
+        partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid], trim=2)
+        partial_name = os.path.basename(partial_path)
+        self.assertEqual(partial_name, mdid + ".u1partial." + longname[:-20])
+
+    def test_get_partial_path_dontcache_when_notrim(self):
+        """Normal behaviour, no partial path cached."""
+        testfile = os.path.join(self.share_path, "longnamethatistoolong")
+        mdid = self.fsm.create(testfile, "share")
+        self.fsm._get_partial_path(self.fsm.fs[mdid])
+
+        # check
+        mdobj = self.fsm.get_by_mdid(mdid)
+        self.assertFalse(hasattr(mdobj.info, 'partial_path'))
+
+    def test_get_partial_path_caches_when_trim(self):
+        """If trimming is necessary, it will cache the name."""
+        testfile = os.path.join(self.share_path, "longnamethatistoolong")
+        mdid = self.fsm.create(testfile, "share")
+        partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid], trim=1)
+
+        # check
+        mdobj = self.fsm.get_by_mdid(mdid)
+        self.assertEqual(mdobj.info.partial_path, partial_path)
+
+    def test_get_partial_path_cached_normal(self):
+        """Return the cached partial path if there."""
+        testfile = os.path.join(self.share_path, "foo")
+        mdid = self.fsm.create(testfile, "share")
+
+        # fake the cache
+        mdobj = self.fsm.fs[mdid]
+        mdobj['info']['partial_path'] = "bar"
+
+        # check
+        partial_path = self.fsm._get_partial_path(mdobj)
+        partial_name = os.path.basename(partial_path)
+        self.assertEqual(partial_name, "bar")
+
+    def test_get_partial_path_cached_trimming(self):
+        """Do not return the cached partial path if there when trimming."""
+        testfile = os.path.join(self.share_path, "foobarlongone")
+        mdid = self.fsm.create(testfile, "share")
+
+        # fake the cache
+        mdobj = self.fsm.fs[mdid]
+        mdobj['info']['partial_path'] = "bar"
+
+        # check
+        partial_path = self.fsm._get_partial_path(mdobj, trim=1)
+        partial_name = os.path.basename(partial_path)
+        self.assertEqual(partial_name, mdid + ".u1partial.foo")
+
 
 class FileHandlingTests(FSMTestCase):
     """Test the file handling services."""

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2010-09-15 19:02:52 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2010-10-25 19:56:06 +0000
@@ -876,11 +876,19 @@
             raise InconsistencyError(msg)
         return partial_in_md
 
-    def _get_partial_path(self, mdobj):
+    def _get_partial_path(self, mdobj, trim=None):
         """Gets the path of the .partial file for a given mdobj"""
+        if trim is None and "partial_path" in mdobj["info"]:
+            return mdobj["info"]["partial_path"]
+
         path = self.get_abspath(mdobj['share_id'], mdobj['path'])
         partial_path = os.path.join(self.partials_dir, mdobj['mdid'] + '.u1partial')
         dirname, filename = os.path.split(path)
+
+        if trim is not None:
+            filename = filename[:-10*trim]
+            mdobj["info"]["partial_path"] = partial_path + '.' + filename
+
         return partial_path + '.' + filename
 
     def create_partial(self, node_id, share_id):
@@ -896,19 +904,33 @@
         mdobj = self.fs[mdid]
         is_dir = mdobj["is_dir"]
         path = self.get_abspath(mdobj['share_id'], mdobj['path'])
-        partial_path = self._get_partial_path(mdobj)
         with self._enable_share_write(share_id, os.path.dirname(path)):
             # if we don't have the dir yet, create it
             if is_dir and not os.path.exists(path):
                 self.eq.add_to_mute_filter("FS_DIR_CREATE", path)
                 os.mkdir(path)
 
-        # don't alert EQ, partials are in other directory, not watched
-        open(partial_path, "w").close()
         mdobj["info"]["last_partial_created"] = time.time()
         mdobj["info"]["is_partial"] = True
         self.fs[mdid] = mdobj
 
+        # create the partial path, trimming the name until fits
+        # in the filesystem
+        partial_path = self._get_partial_path(mdobj)
+        trim = 0
+        while True:
+            try:
+                # don't alert EQ, partials are in other directory, not watched
+                open(partial_path, "w").close()
+            except IOError, e:
+                if e.errno == errno.ENAMETOOLONG:
+                    trim += 1
+                    partial_path = self._get_partial_path(mdobj, trim=trim)
+                else:
+                    raise
+            else:
+                break
+
     def get_partial_for_writing(self, node_id, share_id):
         """Get a write-only fd to a partial file"""
         mdid = self._idx_node_id[(share_id, node_id)]