Merge lp:~facundo/ubuntuone-client/lr-dont-delete-partial-nopartialfile into lp:ubuntuone-client

Proposed by Facundo Batista on 2010-10-19
Status: Merged
Approved by: dobey on 2010-10-22
Approved revision: 736
Merged at revision: 738
Proposed branch: lp:~facundo/ubuntuone-client/lr-dont-delete-partial-nopartialfile
Merge into: lp:ubuntuone-client
Diff against target: 216 lines (+151/-6)
2 files modified
tests/syncdaemon/test_fsm.py (+125/-2)
ubuntuone/syncdaemon/filesystem_manager.py (+26/-4)
To merge this branch: bzr merge lp:~facundo/ubuntuone-client/lr-dont-delete-partial-nopartialfile
Reviewer Review Type Date Requested Status
Lucio Torre (community) Approve on 2010-10-22
Guillermo Gonzalez 2010-10-19 Approve on 2010-10-20
Review via email: mp+38834@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Guillermo Gonzalez (verterok) wrote :

looks good, tests pass.

review: Approve
736. By Facundo Batista on 2010-10-20

Make the filename length platform agnostic in the test

Lucio Torre (lucio.torre) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tests/syncdaemon/test_fsm.py'
2--- tests/syncdaemon/test_fsm.py 2010-09-15 19:02:52 +0000
3+++ tests/syncdaemon/test_fsm.py 2010-10-20 20:57:45 +0000
4@@ -20,10 +20,11 @@
5
6 from __future__ import with_statement
7
8+import errno
9 import os
10 import shutil
11+import time
12 import unittest
13-import time
14
15 from contrib.testing.testcase import (
16 FakeVolumeManager,
17@@ -1584,7 +1585,6 @@
18 # it has no partial!
19 self.fsm.remove_partial("uuid", "share")
20
21-
22 def test_create_dir_previous(self):
23 """Test create .partial for a dir when the dir existed."""
24 testdir = os.path.join(self.share_path, "path")
25@@ -1697,6 +1697,129 @@
26 self.fsm.create_partial("uuid1", "share")
27 self.assertTrue(self.fsm.get_by_mdid(mdid).info.is_partial)
28
29+ def test_disk_problem(self):
30+ """On any disk problem, the node should be left in partial state."""
31+ testfile = os.path.join(self.share_path, "path")
32+ mdid = self.fsm.create(testfile, "share")
33+ self.fsm.set_node_id(testfile, "uuid")
34+
35+ # ugly problem not handled
36+ os.rmdir(self.partials_dir)
37+ try:
38+ self.fsm.create_partial("uuid", "share")
39+ except IOError, e:
40+ if e.errno == errno.ENOENT:
41+ # expected
42+ pass
43+ else:
44+ raise
45+
46+ # the node should still be in partial internally
47+ # for LR to handle it ok
48+ mdobj = self.fsm.get_by_mdid(mdid)
49+ self.assertTrue(mdobj.info.is_partial)
50+
51+ def test_filename_too_long(self):
52+ """Handle the filename being too long."""
53+ # find a almost too long file
54+ repeat = 300
55+ while True:
56+ testfile = os.path.join(self.share_path, "x"*repeat)
57+ try:
58+ fh = open(testfile, 'w')
59+ except IOError, e:
60+ if e.errno == errno.ENAMETOOLONG:
61+ repeat -= 10
62+ else:
63+ fh.close()
64+ os.unlink(testfile)
65+ break
66+
67+ mdid = self.fsm.create(testfile, "share")
68+ self.fsm.set_node_id(testfile, "uuid")
69+
70+ # create partial ok, even knowing that "full partial path" won't fit
71+ self.fsm.create_partial("uuid", "share")
72+ self.assertTrue(self.fsm.get_by_mdid(mdid).info.is_partial)
73+
74+ # check the path
75+ partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid])
76+ self.assertTrue(os.path.exists(partial_path))
77+
78+ def test_get_partial_path_notrim(self):
79+ """Create the partial path."""
80+ testfile = os.path.join(self.share_path, "foo")
81+ mdid = self.fsm.create(testfile, "share")
82+ partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid])
83+ partial_name = os.path.basename(partial_path)
84+ self.assertEqual(partial_name, mdid + ".u1partial.foo")
85+
86+ def test_get_partial_path_trim1(self):
87+ """Create the partial path trimmed some chars."""
88+ longname = "longnamethatistoolong"
89+ testfile = os.path.join(self.share_path, longname)
90+ mdid = self.fsm.create(testfile, "share")
91+ partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid], trim=1)
92+ partial_name = os.path.basename(partial_path)
93+ self.assertEqual(partial_name, mdid + ".u1partial." + longname[:-10])
94+
95+ def test_get_partial_path_trim2(self):
96+ """Create the partial path trimmed more chars."""
97+ longname = "longnamethatistoolong"
98+ testfile = os.path.join(self.share_path, longname)
99+ mdid = self.fsm.create(testfile, "share")
100+ partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid], trim=2)
101+ partial_name = os.path.basename(partial_path)
102+ self.assertEqual(partial_name, mdid + ".u1partial." + longname[:-20])
103+
104+ def test_get_partial_path_dontcache_when_notrim(self):
105+ """Normal behaviour, no partial path cached."""
106+ testfile = os.path.join(self.share_path, "longnamethatistoolong")
107+ mdid = self.fsm.create(testfile, "share")
108+ self.fsm._get_partial_path(self.fsm.fs[mdid])
109+
110+ # check
111+ mdobj = self.fsm.get_by_mdid(mdid)
112+ self.assertFalse(hasattr(mdobj.info, 'partial_path'))
113+
114+ def test_get_partial_path_caches_when_trim(self):
115+ """If trimming is necessary, it will cache the name."""
116+ testfile = os.path.join(self.share_path, "longnamethatistoolong")
117+ mdid = self.fsm.create(testfile, "share")
118+ partial_path = self.fsm._get_partial_path(self.fsm.fs[mdid], trim=1)
119+
120+ # check
121+ mdobj = self.fsm.get_by_mdid(mdid)
122+ self.assertEqual(mdobj.info.partial_path, partial_path)
123+
124+ def test_get_partial_path_cached_normal(self):
125+ """Return the cached partial path if there."""
126+ testfile = os.path.join(self.share_path, "foo")
127+ mdid = self.fsm.create(testfile, "share")
128+
129+ # fake the cache
130+ mdobj = self.fsm.fs[mdid]
131+ mdobj['info']['partial_path'] = "bar"
132+
133+ # check
134+ partial_path = self.fsm._get_partial_path(mdobj)
135+ partial_name = os.path.basename(partial_path)
136+ self.assertEqual(partial_name, "bar")
137+
138+ def test_get_partial_path_cached_trimming(self):
139+ """Do not return the cached partial path if there when trimming."""
140+ testfile = os.path.join(self.share_path, "foobarlongone")
141+ mdid = self.fsm.create(testfile, "share")
142+
143+ # fake the cache
144+ mdobj = self.fsm.fs[mdid]
145+ mdobj['info']['partial_path'] = "bar"
146+
147+ # check
148+ partial_path = self.fsm._get_partial_path(mdobj, trim=1)
149+ partial_name = os.path.basename(partial_path)
150+ self.assertEqual(partial_name, mdid + ".u1partial.foo")
151+
152
153 class FileHandlingTests(FSMTestCase):
154 """Test the file handling services."""
155
156=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
157--- ubuntuone/syncdaemon/filesystem_manager.py 2010-09-15 19:02:52 +0000
158+++ ubuntuone/syncdaemon/filesystem_manager.py 2010-10-20 20:57:45 +0000
159@@ -876,11 +876,19 @@
160 raise InconsistencyError(msg)
161 return partial_in_md
162
163- def _get_partial_path(self, mdobj):
164+ def _get_partial_path(self, mdobj, trim=None):
165 """Gets the path of the .partial file for a given mdobj"""
166+ if trim is None and "partial_path" in mdobj["info"]:
167+ return mdobj["info"]["partial_path"]
168+
169 path = self.get_abspath(mdobj['share_id'], mdobj['path'])
170 partial_path = os.path.join(self.partials_dir, mdobj['mdid'] + '.u1partial')
171 dirname, filename = os.path.split(path)
172+
173+ if trim is not None:
174+ filename = filename[:-10*trim]
175+ mdobj["info"]["partial_path"] = partial_path + '.' + filename
176+
177 return partial_path + '.' + filename
178
179 def create_partial(self, node_id, share_id):
180@@ -896,19 +904,33 @@
181 mdobj = self.fs[mdid]
182 is_dir = mdobj["is_dir"]
183 path = self.get_abspath(mdobj['share_id'], mdobj['path'])
184- partial_path = self._get_partial_path(mdobj)
185 with self._enable_share_write(share_id, os.path.dirname(path)):
186 # if we don't have the dir yet, create it
187 if is_dir and not os.path.exists(path):
188 self.eq.add_to_mute_filter("FS_DIR_CREATE", path)
189 os.mkdir(path)
190
191- # don't alert EQ, partials are in other directory, not watched
192- open(partial_path, "w").close()
193 mdobj["info"]["last_partial_created"] = time.time()
194 mdobj["info"]["is_partial"] = True
195 self.fs[mdid] = mdobj
196
197+ # create the partial path, trimming the name until fits
198+ # in the filesystem
199+ partial_path = self._get_partial_path(mdobj)
200+ trim = 0
201+ while True:
202+ try:
203+ # don't alert EQ, partials are in other directory, not watched
204+ open(partial_path, "w").close()
205+ except IOError, e:
206+ if e.errno == errno.ENAMETOOLONG:
207+ trim += 1
208+ partial_path = self._get_partial_path(mdobj, trim=trim)
209+ else:
210+ raise
211+ else:
212+ break
213+
214 def get_partial_for_writing(self, node_id, share_id):
215 """Get a write-only fd to a partial file"""
216 mdid = self._idx_node_id[(share_id, node_id)]

Subscribers

People subscribed via source and target branches