Merge lp:~facundo/ubuntuone-client/fix-svfilenew-conflict into lp:ubuntuone-client

Proposed by Facundo Batista on 2011-04-04
Status: Merged
Approved by: Facundo Batista on 2011-04-06
Approved revision: 926
Merged at revision: 938
Proposed branch: lp:~facundo/ubuntuone-client/fix-svfilenew-conflict
Merge into: lp:ubuntuone-client
Diff against target: 293 lines (+136/-43)
4 files modified
contrib/dump_metadata.py (+4/-3)
tests/syncdaemon/test_sync.py (+89/-0)
ubuntuone/syncdaemon/sync.py (+34/-5)
ubuntuone/syncdaemon/u1fsfsm.py (+9/-35)
To merge this branch: bzr merge lp:~facundo/ubuntuone-client/fix-svfilenew-conflict
Reviewer Review Type Date Requested Status
Guillermo Gonzalez Approve on 2011-04-06
Lucio Torre (community) 2011-04-04 Approve on 2011-04-04
Review via email: mp+56232@code.launchpad.net

Commit message

Support SV_FILE_NEW when having a node for that path (LP: #711389)

Description of the change

Support SV_FILE_NEW when having a node for that path

This is because _handle_SV_FILE_NEW is called when there's no match by node_id, but then it searches the node by path, and sometimes there is a node with that path.

Three cases arise:

- if the node still has no node_id: it's just waiting for it, so we go and set it.

- if the node has a node_id but other one: it's a case where the file was overwritten by a move operation; just delete the file and will get the server one.

- if the node has the same node_id: we broke something, because we shouldn't have called _handle_SV_FILE_NEW at all.

Tests included for everything.

To post a comment you must log in.
Lucio Torre (lucio.torre) :
review: Approve
Guillermo Gonzalez (verterok) wrote :

+1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'contrib/dump_metadata.py'
2--- contrib/dump_metadata.py 2011-02-21 17:15:15 +0000
3+++ contrib/dump_metadata.py 2011-04-04 17:52:08 +0000
4@@ -97,10 +97,11 @@
5
6 print "\nShowing trash:"
7 something = False
8- for (vol_id, node_id), (mdid, parent_id, path) in fsm.trash.iteritems():
9+ for (vol_id, node_id), (mdid, parent_id, path, is_dir) in \
10+ fsm.trash.iteritems():
11 something = True
12- print " mdid=%r volume_id=%r node_id=%r parent_id=%r path=%r" % (
13- mdid, share_id, node_id, parent_id, path)
14+ print (" mdid=%r volume_id=%r node_id=%r parent_id=%r path=%r "
15+ "is_dir=%r" % (mdid, share_id, node_id, parent_id, path, is_dir))
16 if not something:
17 print " (empty)"
18
19
20=== modified file 'tests/syncdaemon/test_sync.py'
21--- tests/syncdaemon/test_sync.py 2011-03-31 00:36:23 +0000
22+++ tests/syncdaemon/test_sync.py 2011-04-04 17:52:08 +0000
23@@ -255,6 +255,7 @@
24 BaseSync.setUp(self)
25 self.sync = Sync(main=self.main)
26 self.fsm = self.main.fs
27+ self.handler.setLevel(logging.DEBUG)
28
29 def test_deleting_open_files_is_no_cause_for_despair(self):
30 """test_deleting_open_files_is_no_cause_for_despair."""
31@@ -621,6 +622,76 @@
32 'new_parent_id', 'new_name', 'error')
33 self.assertEqual(called[1:], ['AQ_MOVE_ERROR', {}, '', 'node_id'])
34
35+ def test_SV_FILE_NEW_no_node(self):
36+ """Handle SV_FILE_NEW not having a node."""
37+ # fake method
38+ called = []
39+ orig = SyncStateMachineRunner.new_file
40+ self.patch(SyncStateMachineRunner, 'new_file',
41+ lambda *a: called.extend(a) or orig(*a))
42+
43+ # create the parent
44+ parentpath = os.path.join(self.root, 'somepath')
45+ self.fsm.create(parentpath, '', node_id='parent_id')
46+
47+ # call and check
48+ r = self.sync._handle_SV_FILE_NEW('', 'node_id', 'parent_id', 'name')
49+ self.assertEqual(called[1:], ['SV_FILE_NEW', {}, '', 'node_id',
50+ 'parent_id', 'name'])
51+ self.assertEqual(r.share_id, '')
52+ self.assertEqual(r.node_id, 'node_id')
53+ self.assertEqual(r.path, 'somepath/name')
54+
55+ def test_SV_FILE_NEW_node_no_id(self):
56+ """Handle SV_FILE_NEW having a node without node_id."""
57+ # fake method
58+ called = []
59+ orig = SyncStateMachineRunner.new_server_file_having_local
60+ self.patch(SyncStateMachineRunner, 'new_server_file_having_local',
61+ lambda *a: called.extend(a) or orig(*a))
62+
63+ # create the node and its parent, to match per path
64+ parentpath = os.path.join(self.root, 'somepath')
65+ self.fsm.create(parentpath, '', node_id='parent_id')
66+ childpath = os.path.join(parentpath, 'name')
67+ self.fsm.create(childpath, '')
68+
69+ # call and check
70+ self.sync._handle_SV_FILE_NEW('', 'node_id', 'parent_id', 'name')
71+ self.assertEqual(called[1:], ['SV_FILE_NEW', {}, '', 'node_id',
72+ 'parent_id', 'name'])
73+
74+ def test_SV_FILE_NEW_node_same_id(self):
75+ """Handle SV_FILE_NEW having a node with the same node_id."""
76+ parentpath = os.path.join(self.root, 'somepath')
77+ self.fsm.create(parentpath, '', node_id='parent_id')
78+ childpath = os.path.join(parentpath, 'name')
79+ self.fsm.create(childpath, '', node_id='node_id')
80+ r = self.assertRaises(ValueError, self.sync._handle_SV_FILE_NEW,
81+ '', 'node_id', 'parent_id', 'name')
82+ self.assertTrue("same node_id in handle_SV_FILE_NEW" in str(r))
83+
84+ def test_SV_FILE_NEW_node_different_id(self):
85+ """Handle SV_FILE_NEW having a node with different node_id."""
86+ # create the node and its parent, to match per path
87+ parentpath = os.path.join(self.root, 'somepath')
88+ self.fsm.create(parentpath, '', node_id='parent_id')
89+ childpath = os.path.join(parentpath, 'name')
90+ self.fsm.create(childpath, '', node_id='other_id')
91+
92+ # fake method
93+ called = []
94+ orig = self.fsm.delete_file
95+ self.patch(self.fsm, 'delete_file',
96+ lambda path: called.append(path) or orig(path))
97+
98+ # call and check
99+ self.sync._handle_SV_FILE_NEW('', 'node_id', 'parent_id', 'name')
100+ self.assertEqual(called, [childpath])
101+ self.handler.debug = True
102+ self.assertTrue(self.handler.check_debug("Wanted to apply SV_FILE_NEW",
103+ "found it with other id"))
104+
105
106 class SyncStateMachineRunnerTestCase(BaseSync):
107 """Tests for the SyncStateMachineRunner."""
108@@ -1115,6 +1186,24 @@
109 # check the create and the make_dir
110 self.assertEqual(called, [(new_path, '', 'node_id', True), 'mdid'])
111
112+ def test_new_new_server_file_having_local(self):
113+ """Set the node_id to the node."""
114+ # create context
115+ somepath = os.path.join(self.root, 'foo')
116+ self.fsm.create(somepath, '')
117+ key = FSKey(self.main.fs, path=somepath)
118+ ssmr = SyncStateMachineRunner(fsm=self.fsm, main=self.main,
119+ key=key, logger=None)
120+
121+ # log the call to fsm
122+ called = []
123+ self.fsm.set_node_id = lambda *a: called.append(a)
124+
125+ # call the tested method and check
126+ ssmr.new_server_file_having_local('event', {}, '', 'node_id',
127+ 'parent_id', 'name')
128+ self.assertEqual(called, [(somepath, 'node_id')])
129+
130
131 class FakedState(object):
132 """A faked state."""
133
134=== modified file 'ubuntuone/syncdaemon/sync.py'
135--- ubuntuone/syncdaemon/sync.py 2011-03-31 00:36:23 +0000
136+++ ubuntuone/syncdaemon/sync.py 2011-04-04 17:52:08 +0000
137@@ -545,6 +545,11 @@
138 self.m.action_q.uuid_map.set(marker, new_id)
139 self.m.fs.set_node_id(self.key['path'], new_id)
140
141+ def new_server_file_having_local(self, event, parms, share_id, node_id,
142+ parent_id, name):
143+ """Got new file from server, we have it local but without id yet."""
144+ self.m.fs.set_node_id(self.key['path'], node_id)
145+
146 def new_local_dir(self, event, parms, path):
147 """a new local dir was created"""
148 parent_path = os.path.dirname(path)
149@@ -838,9 +843,31 @@
150 key = FSKey(self.m.fs, path=path)
151 log = FileLogger(self.logger, key)
152 ssmr = SyncStateMachineRunner(self.fsm, self.m, key, log)
153+
154+ # check if the node found by path has other node_id (which will
155+ # cause a conflict); note that we don't get the node_id from the
156+ # 'key' as it lies to us with the marker
157+ try:
158+ mdid = key.get_mdid()
159+ except KeyError:
160+ pass # no md at all, didn't find any node by the path
161+ else:
162+ mdobj = self.m.fs.get_by_mdid(mdid)
163+ if mdobj.node_id is not None:
164+ if mdobj.node_id == node_id:
165+ raise ValueError("Found same node_id in handle_SV_FILE_NEW"
166+ " (node_id=%s path=%r)" % (node_id, path))
167+ # have metadata with *other* node_id
168+ log.debug("Wanted to apply SV_FILE_NEW with node_id %s to node"
169+ "with path %r, but found it with other id: %s",
170+ node_id, path, mdobj.node_id)
171+ key.delete_file()
172+ return None
173+
174 ssmr.on_event("SV_FILE_NEW", {}, share_id, node_id, parent_id, name)
175 self.m.event_q.push('SV_FILE_NEW', volume_id=share_id,
176 node_id=node_id, parent_id=parent_id, name=name)
177+ return self.m.fs.get_by_node_id(share_id, node_id)
178
179 def _handle_SV_DIR_NEW(self, share_id, node_id, parent_id, name):
180 """on SV_DIR_NEW"""
181@@ -852,6 +879,7 @@
182 ssmr.on_event("SV_DIR_NEW", {}, share_id, node_id, parent_id, name)
183 self.m.event_q.push('SV_DIR_NEW', volume_id=share_id,
184 node_id=node_id, parent_id=parent_id, name=name)
185+ return self.m.fs.get_by_node_id(share_id, node_id)
186
187 def _handle_SV_FILE_DELETED(self, share_id, node_id, is_dir):
188 """on SV_FILE_DELETED. Not called by EQ anymore."""
189@@ -1106,13 +1134,14 @@
190 continue
191
192 # node not there, we must create it
193+ args = (dt.share_id, dt.node_id, dt.parent_id, dt_name)
194 if is_dir:
195- self._handle_SV_DIR_NEW(dt.share_id, dt.node_id,
196- dt.parent_id, dt_name)
197+ node = self._handle_SV_DIR_NEW(*args)
198 else:
199- self._handle_SV_FILE_NEW(dt.share_id, dt.node_id,
200- dt.parent_id, dt_name)
201- node = self.m.fs.get_by_node_id(dt.share_id, dt.node_id)
202+ node = self._handle_SV_FILE_NEW(*args)
203+ if node is None:
204+ # the node was not created!
205+ continue
206
207 # if the delta is older than the node, skip!
208 if node.generation > dt.generation:
209
210=== modified file 'ubuntuone/syncdaemon/u1fsfsm.ods'
211Binary files ubuntuone/syncdaemon/u1fsfsm.ods 2011-03-08 20:25:00 +0000 and ubuntuone/syncdaemon/u1fsfsm.ods 2011-04-04 17:52:08 +0000 differ
212=== modified file 'ubuntuone/syncdaemon/u1fsfsm.py'
213--- ubuntuone/syncdaemon/u1fsfsm.py 2010-09-07 20:49:38 +0000
214+++ ubuntuone/syncdaemon/u1fsfsm.py 2011-04-04 17:52:08 +0000
215@@ -300,32 +300,6 @@
216 'STATE_OUT': {u'changed': u'=',
217 u'has_metadata': u'=',
218 u'is_directory': u'='}}],
219- u'AQ_DOWNLOAD_CANCELLED': [{'ACTION': u'md.remove_partial(uuid);',
220- 'ACTION_FUNC': u'nothing',
221- 'COMMENTS': u'Download cancelled, remove the partial file',
222- 'PARAMETERS': {u'hash_eq_local_hash': u'*',
223- u'hash_eq_server_hash': u'*',
224- u'not_authorized': u'NA',
225- u'not_available': u'NA'},
226- 'STATE': {u'changed': u'*',
227- u'has_metadata': u'T',
228- u'is_directory': u'*'},
229- 'STATE_OUT': {u'changed': u'=',
230- u'has_metadata': u'=',
231- u'is_directory': u'='}},
232- {'ACTION': u'pass',
233- 'ACTION_FUNC': u'nothing',
234- 'COMMENTS': u'',
235- 'PARAMETERS': {u'hash_eq_local_hash': u'*',
236- u'hash_eq_server_hash': u'*',
237- u'not_authorized': u'NA',
238- u'not_available': u'NA'},
239- 'STATE': {u'changed': u'*',
240- u'has_metadata': u'F',
241- u'is_directory': u'*'},
242- 'STATE_OUT': {u'changed': u'=',
243- u'has_metadata': u'=',
244- u'is_directory': u'='}}],
245 u'AQ_DOWNLOAD_DOES_NOT_EXIST': [{'ACTION': u'',
246 'ACTION_FUNC': u'delete_file',
247 'COMMENTS': u"Directory doesn't exist anymore, remove it",
248@@ -1015,8 +989,8 @@
249 'STATE_OUT': {u'changed': u'=',
250 u'has_metadata': u'=',
251 u'is_directory': u'='}},
252- {'ACTION': u'md.set(mdid, server_uuid=server_uuid)\nPANIC',
253- 'ACTION_FUNC': u'DESPAIR',
254+ {'ACTION': u"Node got node_id with a SV_FILE_NEW and now it's uploading something",
255+ 'ACTION_FUNC': u'nothing',
256 'COMMENTS': u'',
257 'PARAMETERS': {u'hash_eq_local_hash': u'NA',
258 u'hash_eq_server_hash': u'NA',
259@@ -1025,8 +999,8 @@
260 'STATE': {u'changed': u'SERVER',
261 u'has_metadata': u'T',
262 u'is_directory': u'F'},
263- 'STATE_OUT': {u'changed': u'NONE',
264- u'has_metadata': u'T',
265+ 'STATE_OUT': {u'changed': u'=',
266+ u'has_metadata': u'=',
267 u'is_directory': u'='}},
268 {'ACTION': u'aq.uuid_map.set(marker, new_id)',
269 'ACTION_FUNC': u'release_marker_ok',
270@@ -2211,8 +2185,8 @@
271 'STATE_OUT': {u'changed': u'NONE',
272 u'has_metadata': u'T',
273 u'is_directory': u'F'}},
274- {'ACTION': u'CONFLICT',
275- 'ACTION_FUNC': u'new_file_on_server_with_local',
276+ {'ACTION': u"Didn't find the node by node_id, but found it by path",
277+ 'ACTION_FUNC': u'new_server_file_having_local',
278 'COMMENTS': u'',
279 'PARAMETERS': {u'hash_eq_local_hash': u'NA',
280 u'hash_eq_server_hash': u'NA',
281@@ -2221,9 +2195,9 @@
282 'STATE': {u'changed': u'*',
283 u'has_metadata': u'T',
284 u'is_directory': u'*'},
285- 'STATE_OUT': {u'changed': u'NONE',
286- u'has_metadata': u'T',
287- u'is_directory': u'F'}}],
288+ 'STATE_OUT': {u'changed': u'=',
289+ u'has_metadata': u'=',
290+ u'is_directory': u'='}}],
291 u'SV_HASH_NEW': [{'ACTION': u'NA',
292 'ACTION_FUNC': u'',
293 'COMMENTS': u'',

Subscribers

People subscribed via source and target branches