OpenStack Compute (nova)

Merge lp:~vishvananda/nova/fix-volumes into lp:~hudson-openstack/nova/trunk

  1. fix-volumes
  2. Merge into trunk
Proposed by Vish Ishaya
Status: Merged
Approved by: Eric Day
Approved revision: 202
Merge reported by: OpenStack Infra
Merged at revision: not available
Proposed branch: lp:~vishvananda/nova/fix-volumes
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 212 lines (+39/-35)
3 files modified
nova/endpoint/cloud.py (+5/-6)
nova/tests/volume_unittest.py (+10/-10)
nova/volume/service.py (+24/-19)
To merge this branch: bzr merge lp:~vishvananda/nova/fix-volumes
Reviewer Review Type Date Requested Status
Eric Day (community) Approve
Jesse Andrews (community) Approve
Review via email: mp+31445@code.launchpad.net

Commit message

Fixes various concurrency issues in volume worker.

Description of the change

Create volume was randomly erroring due to commands running concurrently and some commands printing to stderr for no reason. This patch fixes the inline callbacks to actually yield properly and simplifies the processing of the value returned to cloud.py.

To post a comment you must log in.
lp:~vishvananda/nova/fix-volumes updated
202. By Vish Ishaya

Fix Tests

Revision history for this message
Jesse Andrews (anotherjesse) wrote :

lgtm

review: Approve
Revision history for this message
Eric Day (eday) wrote :

So, this looks like it has almost the same set of changes as your fix-exceptions branch, but they don't share a commit history for those changes. Is this one no longer valid? This will probably cause a conflict with your other branch once merged.

Revision history for this message
Vish Ishaya (vishvananda) wrote :

hmm ifix eceptions is bqased on fix volumes. I forgot to put the depends on
in the other branch. The other fixes are actually just a couple of lines

On Aug 4, 2010 10:07 PM, "Eric Day" <email address hidden> wrote:

So, this looks like it has almost the same set of changes as your
fix-exceptions branch, but they don't share a commit history for those
changes. Is this one no longer valid? This will probably cause a conflict
with your other branch once merged.

--
https://code.launchpad.net/~vishvananda/nova/fix-volumes/+merge/31445
You are the owner of lp:~v...

Revision history for this message
Eric Day (eday) wrote :

Ahh, I see. They were inside a merge, not at the top level.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/endpoint/cloud.py'
2--- nova/endpoint/cloud.py 2010-07-29 03:23:51 +0000
3+++ nova/endpoint/cloud.py 2010-07-31 02:35:50 +0000
4@@ -295,17 +295,16 @@
5 return v
6
7 @rbac.allow('projectmanager', 'sysadmin')
8+ @defer.inlineCallbacks
9 def create_volume(self, context, size, **kwargs):
10 # TODO(vish): refactor this to create the volume object here and tell service to create it
11- res = rpc.call(FLAGS.volume_topic, {"method": "create_volume",
12+ result = yield rpc.call(FLAGS.volume_topic, {"method": "create_volume",
13 "args" : {"size": size,
14 "user_id": context.user.id,
15 "project_id": context.project.id}})
16- def _format_result(result):
17- volume = self._get_volume(context, result['result'])
18- return {'volumeSet': [self.format_volume(context, volume)]}
19- res.addCallback(_format_result)
20- return res
21+ # NOTE(vish): rpc returned value is in the result key in the dictionary
22+ volume = self._get_volume(context, result['result'])
23+ defer.returnValue({'volumeSet': [self.format_volume(context, volume)]})
24
25 def _get_address(self, context, public_ip):
26 # FIXME(vish) this should move into network.py
27
28=== modified file 'nova/tests/volume_unittest.py'
29--- nova/tests/volume_unittest.py 2010-07-28 23:11:02 +0000
30+++ nova/tests/volume_unittest.py 2010-07-31 02:35:50 +0000
31@@ -42,15 +42,14 @@
32 vol_size = '0'
33 user_id = 'fake'
34 project_id = 'fake'
35- volume_id = self.volume.create_volume(vol_size, user_id, project_id)
36+ volume_id = yield self.volume.create_volume(vol_size, user_id, project_id)
37 # TODO(termie): get_volume returns differently than create_volume
38 self.assertEqual(volume_id,
39 volume_service.get_volume(volume_id)['volume_id'])
40
41 rv = self.volume.delete_volume(volume_id)
42- self.assertRaises(exception.Error,
43- volume_service.get_volume,
44- volume_id)
45+ self.assertFailure(volume_service.get_volume(volume_id),
46+ exception.Error)
47
48 def test_too_big_volume(self):
49 vol_size = '1001'
50@@ -68,13 +67,14 @@
51 total_slots = FLAGS.slots_per_shelf * num_shelves
52 vols = []
53 for i in xrange(total_slots):
54- vid = self.volume.create_volume(vol_size, user_id, project_id)
55+ vid = yield self.volume.create_volume(vol_size, user_id, project_id)
56 vols.append(vid)
57- self.assertRaises(volume_service.NoMoreVolumes,
58- self.volume.create_volume,
59- vol_size, user_id, project_id)
60+ self.assertFailure(self.volume.create_volume(vol_size,
61+ user_id,
62+ project_id),
63+ volume_service.NoMoreVolumes)
64 for id in vols:
65- self.volume.delete_volume(id)
66+ yield self.volume.delete_volume(id)
67
68 def test_run_attach_detach_volume(self):
69 # Create one volume and one compute to test with
70@@ -83,7 +83,7 @@
71 user_id = "fake"
72 project_id = 'fake'
73 mountpoint = "/dev/sdf"
74- volume_id = self.volume.create_volume(vol_size, user_id, project_id)
75+ volume_id = yield self.volume.create_volume(vol_size, user_id, project_id)
76
77 volume_obj = volume_service.get_volume(volume_id)
78 volume_obj.start_attach(instance_id, mountpoint)
79
80=== modified file 'nova/volume/service.py'
81--- nova/volume/service.py 2010-07-27 00:14:28 +0000
82+++ nova/volume/service.py 2010-07-31 02:35:50 +0000
83@@ -103,6 +103,7 @@
84 except Exception, err:
85 pass
86
87+ @defer.inlineCallbacks
88 @validate.rangetest(size=(0, 1000))
89 def create_volume(self, size, user_id, project_id):
90 """
91@@ -111,11 +112,12 @@
92 Volume at this point has size, owner, and zone.
93 """
94 logging.debug("Creating volume of size: %s" % (size))
95- vol = self.volume_class.create(size, user_id, project_id)
96+ vol = yield self.volume_class.create(size, user_id, project_id)
97 datastore.Redis.instance().sadd('volumes', vol['volume_id'])
98 datastore.Redis.instance().sadd('volumes:%s' % (FLAGS.storage_name), vol['volume_id'])
99- self._restart_exports()
100- return vol['volume_id']
101+ logging.debug("restarting exports")
102+ yield self._restart_exports()
103+ defer.returnValue(vol['volume_id'])
104
105 def by_node(self, node_id):
106 """ returns a list of volumes for a node """
107@@ -128,6 +130,7 @@
108 for volume_id in datastore.Redis.instance().smembers('volumes'):
109 yield self.volume_class(volume_id=volume_id)
110
111+ @defer.inlineCallbacks
112 def delete_volume(self, volume_id):
113 logging.debug("Deleting volume with id of: %s" % (volume_id))
114 vol = get_volume(volume_id)
115@@ -135,19 +138,18 @@
116 raise exception.Error("Volume is still attached")
117 if vol['node_name'] != FLAGS.storage_name:
118 raise exception.Error("Volume is not local to this node")
119- vol.destroy()
120+ yield vol.destroy()
121 datastore.Redis.instance().srem('volumes', vol['volume_id'])
122 datastore.Redis.instance().srem('volumes:%s' % (FLAGS.storage_name), vol['volume_id'])
123- return True
124+ defer.returnValue(True)
125
126 @defer.inlineCallbacks
127 def _restart_exports(self):
128 if FLAGS.fake_storage:
129 return
130- yield process.simple_execute(
131- "sudo vblade-persist auto all")
132- yield process.simple_execute(
133- "sudo vblade-persist start all")
134+ yield process.simple_execute("sudo vblade-persist auto all")
135+ # NOTE(vish): this command sometimes sends output to stderr for warnings
136+ yield process.simple_execute("sudo vblade-persist start all", error_ok=1)
137
138 @defer.inlineCallbacks
139 def _init_volume_group(self):
140@@ -173,6 +175,7 @@
141 return {"volume_id": self.volume_id}
142
143 @classmethod
144+ @defer.inlineCallbacks
145 def create(cls, size, user_id, project_id):
146 volume_id = utils.generate_uid('vol')
147 vol = cls(volume_id)
148@@ -188,13 +191,12 @@
149 vol['attach_status'] = "detached" # attaching | attached | detaching | detached
150 vol['delete_on_termination'] = 'False'
151 vol.save()
152- vol.create_lv()
153- vol._setup_export()
154+ yield vol._create_lv()
155+ yield vol._setup_export()
156 # TODO(joshua) - We need to trigger a fanout message for aoe-discover on all the nodes
157- # TODO(joshua
158 vol['status'] = "available"
159 vol.save()
160- return vol
161+ defer.returnValue(vol)
162
163 def start_attach(self, instance_id, mountpoint):
164 """ """
165@@ -223,16 +225,18 @@
166 self['attach_status'] = "detached"
167 self.save()
168
169+ @defer.inlineCallbacks
170 def destroy(self):
171 try:
172- self._remove_export()
173- except:
174+ yield self._remove_export()
175+ except Exception as ex:
176+ logging.debug("Ingnoring failure to remove export %s" % ex)
177 pass
178- self._delete_lv()
179+ yield self._delete_lv()
180 super(Volume, self).destroy()
181
182 @defer.inlineCallbacks
183- def create_lv(self):
184+ def _create_lv(self):
185 if str(self['size']) == '0':
186 sizestr = '100M'
187 else:
188@@ -248,13 +252,14 @@
189 "sudo lvremove -f %s/%s" % (FLAGS.volume_group,
190 self['volume_id']))
191
192+ @defer.inlineCallbacks
193 def _setup_export(self):
194 (shelf_id, blade_id) = get_next_aoe_numbers()
195 self['aoe_device'] = "e%s.%s" % (shelf_id, blade_id)
196 self['shelf_id'] = shelf_id
197 self['blade_id'] = blade_id
198 self.save()
199- self._exec_export()
200+ yield self._exec_export()
201
202 @defer.inlineCallbacks
203 def _exec_export(self):
204@@ -277,7 +282,7 @@
205
206
207 class FakeVolume(Volume):
208- def create_lv(self):
209+ def _create_lv(self):
210 pass
211
212 def _exec_export(self):