1 From fd7e9dd59ffa346bed5a11e5312f4bb1bf114ab4 Mon Sep 17 00:00:00 2001
2 From: Dmitry Borodaenko <angdraug@gmail.com>
3 Date: Wed, 27 Nov 2013 14:33:00 -0800
4 Subject: [PATCH] Do not clone non-raw images in rbd backend
6 RBD backend only supports booting from images in raw format. A volume
7 that was cloned from an image in any other format is not bootable. The
8 RBD driver will consider non-raw images to be uncloneable to trigger
9 automatic conversion to raw format.
11 Includes conversion of the corresponding unit test to use mock (instead
12 of mox) and expanded comments and error messages from patchset #58893 by
15 Change-Id: I5725d2f7576bc1b3e9b874ba944ad17d33a6e2cb
19 cinder/tests/test_gpfs.py | 6 +-
20 cinder/tests/test_netapp_nfs.py | 12 +-
21 cinder/tests/test_rbd.py | 168 +++++++++++++++++---------
22 cinder/tests/test_volume.py | 2 +-
23 cinder/volume/driver.py | 7 +-
24 cinder/volume/drivers/gpfs.py | 2 +-
25 cinder/volume/drivers/lvm.py | 2 +-
26 cinder/volume/drivers/netapp/nfs.py | 2 +-
27 cinder/volume/drivers/rbd.py | 14 ++-
28 cinder/volume/drivers/scality.py | 2 +-
29 cinder/volume/flows/create_volume/__init__.py | 2 +-
30 11 files changed, 144 insertions(+), 75 deletions(-)
32 diff --git a/cinder/tests/test_gpfs.py b/cinder/tests/test_gpfs.py
33 index 4fdb788..1f47c6b 100644
34 --- a/cinder/tests/test_gpfs.py
35 +++ b/cinder/tests/test_gpfs.py
36 @@ -288,7 +288,8 @@ class GPFSDriverTestCase(test.TestCase):
37 CONF.gpfs_images_share_mode = 'copy_on_write'
38 self.driver.clone_image(volume,
44 self.assertTrue(os.path.exists(volumepath))
45 self.volume.delete_volume(self.context, volume['id'])
46 @@ -309,7 +310,8 @@ class GPFSDriverTestCase(test.TestCase):
47 CONF.gpfs_images_share_mode = 'copy'
48 self.driver.clone_image(volume,
54 self.assertTrue(os.path.exists(volumepath))
55 self.volume.delete_volume(self.context, volume['id'])
56 diff --git a/cinder/tests/test_netapp_nfs.py b/cinder/tests/test_netapp_nfs.py
57 index 950efc8..042280e 100644
58 --- a/cinder/tests/test_netapp_nfs.py
59 +++ b/cinder/tests/test_netapp_nfs.py
60 @@ -469,7 +469,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase):
61 drv._post_clone_image(volume)
64 - drv. clone_image(volume, ('image_location', None), 'image_id')
65 + drv.clone_image(volume, ('image_location', None), 'image_id', {})
68 def get_img_info(self, format):
69 @@ -493,7 +493,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase):
72 (prop, cloned) = drv. clone_image(
73 - volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id')
74 + volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id', {})
76 if not cloned and not prop['provider_location']:
78 @@ -529,7 +529,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase):
82 - volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id')
83 + volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id', {})
86 def test_clone_image_cloneableshare_notraw(self):
87 @@ -566,7 +566,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase):
91 - volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id')
92 + volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id', {})
95 def test_clone_image_file_not_discovered(self):
96 @@ -605,7 +605,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase):
99 vol_dict, result = drv. clone_image(
100 - volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id')
101 + volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id', {})
103 self.assertFalse(result)
104 self.assertFalse(vol_dict['bootable'])
105 @@ -652,7 +652,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase):
108 vol_dict, result = drv. clone_image(
109 - volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id')
110 + volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id', {})
112 self.assertFalse(result)
113 self.assertFalse(vol_dict['bootable'])
114 diff --git a/cinder/tests/test_rbd.py b/cinder/tests/test_rbd.py
115 index 60de09b..054bbb5 100644
116 --- a/cinder/tests/test_rbd.py
117 +++ b/cinder/tests/test_rbd.py
118 @@ -34,6 +34,7 @@ from cinder.tests.test_volume import DriverTestCase
119 from cinder import units
120 from cinder.volume import configuration as conf
121 import cinder.volume.drivers.rbd as driver
122 +from cinder.volume.flows import create_volume
125 LOG = logging.getLogger(__name__)
126 @@ -247,7 +248,8 @@ class RBDTestCase(test.TestCase):
127 self.assertRaises(exception.ImageUnacceptable,
128 self.driver._parse_location,
130 - self.assertFalse(self.driver._is_cloneable(loc))
132 + self.driver._is_cloneable(loc, {'disk_format': 'raw'}))
134 def test_cloneable(self):
135 self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc')
136 @@ -264,12 +266,14 @@ class RBDTestCase(test.TestCase):
140 - self.assertTrue(self.driver._is_cloneable(location))
142 + self.driver._is_cloneable(location, {'disk_format': 'raw'}))
144 def test_uncloneable_different_fsid(self):
145 self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc')
146 location = 'rbd://def/pool/image/snap'
147 - self.assertFalse(self.driver._is_cloneable(location))
149 + self.driver._is_cloneable(location, {'disk_format': 'raw'}))
151 def test_uncloneable_unreadable(self):
152 self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc')
153 @@ -284,7 +288,16 @@ class RBDTestCase(test.TestCase):
157 - self.assertFalse(self.driver._is_cloneable(location))
159 + self.driver._is_cloneable(location, {'disk_format': 'raw'}))
161 + def test_uncloneable_bad_format(self):
162 + self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc')
163 + location = 'rbd://abc/pool/image/snap'
164 + formats = ['qcow2', 'vmdk', 'vdi']
167 + self.driver._is_cloneable(location, {'disk_format': f}))
169 def _copy_image(self):
170 @contextlib.contextmanager
171 @@ -504,26 +517,37 @@ class ManagedRBDTestCase(DriverTestCase):
172 super(ManagedRBDTestCase, self).setUp()
173 fake_image.stub_out_image_service(self.stubs)
174 self.volume.driver.set_initialized()
177 - def _clone_volume_from_image(self, expected_status,
179 + def _create_volume_from_image(self, expected_status, raw=False,
180 + clone_error=False):
181 """Try to clone a volume from an image, and check the status
184 + NOTE: if clone_error is True we force the image type to raw otherwise
185 + clone_image is not called
187 - def fake_clone_image(volume, image_location, image_id):
188 - return {'provider_location': None}, True
189 + def mock_clone_image(volume, image_location, image_id, image_meta):
190 + self.called.append('clone_image')
192 + raise exception.CinderException()
194 + return {'provider_location': None}, True
196 - def fake_clone_error(volume, image_location, image_id):
197 - raise exception.CinderException()
201 - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True)
203 - self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_image)
204 + # See tests.image.fake for image types.
206 + expected_calls = ['clone_image']
207 + image_id = '155d900f-4e14-4e4c-a73d-069cbf4541e6'
209 - self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_error)
210 + expected_calls = ['clone_image', 'create_volume',
211 + 'copy_image_to_volume']
212 + image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77'
214 - image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77'
217 # creating volume testdata
218 db.volume_create(self.context,
220 @@ -533,58 +557,88 @@ class ManagedRBDTestCase(DriverTestCase):
221 'status': 'creating',
222 'instance_uuid': None,
226 - self.volume.create_volume(self.context,
230 - self.assertRaises(exception.CinderException,
231 - self.volume.create_volume,
236 - volume = db.volume_get(self.context, volume_id)
237 - self.assertEqual(volume['status'], expected_status)
240 - db.volume_destroy(self.context, volume_id)
242 + with mock.patch.object(self.volume.driver, 'create_volume') as \
243 + mock_create_volume:
244 + with mock.patch.object(self.volume.driver, 'clone_image',
246 + with mock.patch.object(create_volume.CreateVolumeFromSpecTask,
247 + '_copy_image_to_volume') as \
248 + mock_copy_image_to_volume:
249 + self.volume.driver._is_cloneable = mock.Mock()
250 + self.volume.driver._is_cloneable.return_value = True
253 + if not clone_error:
254 + self.volume.create_volume(self.context,
258 + self.assertRaises(exception.CinderException,
259 + self.volume.create_volume,
264 + volume = db.volume_get(self.context, volume_id)
265 + self.assertEqual(volume['status'], expected_status)
268 + db.volume_destroy(self.context, volume_id)
271 + self.assertEquals(self.called, ['clone_image'])
273 + mock_create_volume.assert_called()
274 + mock_copy_image_to_volume.assert_called()
276 def test_create_vol_from_image_status_available(self):
277 - """Verify that before cloning, an image is in the available state."""
278 - self._clone_volume_from_image('available', True)
279 + """Clone raw image then verify volume is in available state."""
280 + self._create_volume_from_image('available', raw=True)
282 + def test_create_vol_from_non_raw_image_status_available(self):
283 + """Clone non-raw image then verify volume is in available state."""
284 + self._create_volume_from_image('available', raw=False)
286 def test_create_vol_from_image_status_error(self):
287 - """Verify that before cloning, an image is in the available state."""
288 - self._clone_volume_from_image('error', False)
289 + """Clone raw image with failure then verify volume is in error
292 + self._create_volume_from_image('error', raw=True, clone_error=True)
294 def test_clone_image(self):
295 - # Test Failure Case(s)
296 - expected = ({}, False)
297 + driver = self.volume.driver
299 - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: False)
300 - image_loc = (object(), object())
301 - actual = self.volume.driver.clone_image(object(), image_loc, object())
302 - self.assertEqual(expected, actual)
303 + # Test uncloneable case(s)
304 + with mock.patch.object(driver, '_is_cloneable',
305 + lambda *args: False) as mock_is_cloneable:
306 + image_loc = (mock.Mock(), mock.Mock())
307 + actual = driver.clone_image(mock.Mock(), image_loc,
309 + self.assertEqual(({}, False), actual)
311 - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True)
312 - self.assertEqual(expected,
313 - self.volume.driver.clone_image(object(), None, None))
314 + self.assertEqual(({}, False),
315 + driver.clone_image(object(), None, None, {}))
317 - # Test Success Case(s)
318 + # Test success case(s)
319 expected = ({'provider_location': None}, True)
321 - self.stubs.Set(self.volume.driver, '_parse_location',
322 - lambda x: ('a', 'b', 'c', 'd'))
324 - self.stubs.Set(self.volume.driver, '_clone', lambda *args: None)
325 - self.stubs.Set(self.volume.driver, '_resize', lambda *args: None)
326 - actual = self.volume.driver.clone_image(object(), image_loc, object())
327 - self.assertEqual(expected, actual)
328 + mpo = mock.patch.object
329 + with mpo(driver, '_is_cloneable', lambda *args: True):
330 + with mpo(driver, '_parse_location',
331 + lambda x: ('a', 'b', 'c', 'd')):
332 + with mpo(driver, '_clone') as mock_clone:
333 + with mpo(driver, '_resize') as mock_resize:
334 + actual = driver.clone_image(mock.Mock(), image_loc,
336 + {'disk_format': 'raw'})
337 + self.assertEqual(expected, actual)
338 + mock_clone.assert_called()
339 + mock_resize.assert_called()
341 def test_clone_success(self):
342 - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True)
343 - self.stubs.Set(self.volume.driver, 'clone_image', lambda a, b, c: True)
344 + self.stubs.Set(self.volume.driver, '_is_cloneable', lambda *args: True)
345 + self.stubs.Set(self.volume.driver, 'clone_image',
346 + lambda a, b, c, d: True)
347 image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77'
348 - self.assertTrue(self.volume.driver.clone_image({}, image_id, image_id))
349 + self.assertTrue(self.volume.driver.clone_image({},
350 + image_id, image_id, {}))
351 diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py
352 index 4a0d475..928799f 100644
353 --- a/cinder/tests/test_volume.py
354 +++ b/cinder/tests/test_volume.py
355 @@ -1216,7 +1216,7 @@ class VolumeTestCase(BaseVolumeTestCase):
356 def fake_fetch_to_raw(ctx, image_service, image_id, path, size=None):
359 - def fake_clone_image(volume_ref, image_location, image_id):
360 + def fake_clone_image(volume_ref, image_location, image_id, image_meta):
361 return {'provider_location': None}, True
363 dst_fd, dst_path = tempfile.mkstemp()
364 diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py
365 index 21dd12b..dd2be3c 100644
366 --- a/cinder/volume/driver.py
367 +++ b/cinder/volume/driver.py
368 @@ -396,7 +396,7 @@ class VolumeDriver(object):
369 connector.disconnect_volume(attach_info['conn']['data'],
370 attach_info['device'])
372 - def clone_image(self, volume, image_location, image_id):
373 + def clone_image(self, volume, image_location, image_id, image_meta):
374 """Create a volume efficiently from an existing image.
376 image_location is a string whose format depends on the
377 @@ -407,6 +407,11 @@ class VolumeDriver(object):
378 It can be used by the driver to introspect internal
379 stores or registry to do an efficient image clone.
381 + image_meta is a dictionary that includes 'disk_format' (e.g.
382 + raw, qcow2) and other image attributes that allow drivers to
383 + decide whether they can clone the image without first requiring
386 Returns a dict of volume properties eg. provider_location,
387 boolean indicating whether cloning occurred
389 diff --git a/cinder/volume/drivers/gpfs.py b/cinder/volume/drivers/gpfs.py
390 index 9a1a397..8792ad8 100644
391 --- a/cinder/volume/drivers/gpfs.py
392 +++ b/cinder/volume/drivers/gpfs.py
393 @@ -463,7 +463,7 @@ class GPFSDriver(driver.VolumeDriver):
395 return '%sG' % size_in_g
397 - def clone_image(self, volume, image_location, image_id):
398 + def clone_image(self, volume, image_location, image_id, image_meta):
399 return self._clone_image(volume, image_location, image_id)
401 def _is_cloneable(self, image_id):
402 diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py
403 index 094436f..c6023eb 100644
404 --- a/cinder/volume/drivers/lvm.py
405 +++ b/cinder/volume/drivers/lvm.py
406 @@ -322,7 +322,7 @@ class LVMVolumeDriver(driver.VolumeDriver):
408 self.delete_snapshot(temp_snapshot)
410 - def clone_image(self, volume, image_location, image_id):
411 + def clone_image(self, volume, image_location, image_id, image_meta):
414 def backup_volume(self, context, backup, backup_service):
415 diff --git a/cinder/volume/drivers/netapp/nfs.py b/cinder/volume/drivers/netapp/nfs.py
416 index 602a1dc..9463137 100644
417 --- a/cinder/volume/drivers/netapp/nfs.py
418 +++ b/cinder/volume/drivers/netapp/nfs.py
419 @@ -374,7 +374,7 @@ class NetAppNFSDriver(nfs.NfsDriver):
420 LOG.warning(_('Exception during deleting %s'), ex.__str__())
423 - def clone_image(self, volume, image_location, image_id):
424 + def clone_image(self, volume, image_location, image_id, image_meta):
425 """Create a volume efficiently from an existing image.
427 image_location is a string whose format depends on the
428 diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py
429 index 775ab16..7ed59d0 100644
430 --- a/cinder/volume/drivers/rbd.py
431 +++ b/cinder/volume/drivers/rbd.py
432 @@ -706,7 +706,7 @@ class RBDDriver(driver.VolumeDriver):
433 with RADOSClient(self) as client:
434 return client.cluster.get_fsid()
436 - def _is_cloneable(self, image_location):
437 + def _is_cloneable(self, image_location, image_meta):
439 fsid, pool, image, snapshot = self._parse_location(image_location)
440 except exception.ImageUnacceptable as e:
441 @@ -718,6 +718,13 @@ class RBDDriver(driver.VolumeDriver):
445 + if image_meta['disk_format'] != 'raw':
446 + reason = _("rbd image clone requires image format to be "
447 + "'raw' but image {0} is '{1}'").format(
448 + image_location, image_meta['disk_format'])
452 # check that we can read the image
454 with RBDVolumeProxy(self, image,
455 @@ -730,9 +737,10 @@ class RBDDriver(driver.VolumeDriver):
456 dict(loc=image_location, err=e))
459 - def clone_image(self, volume, image_location, image_id):
460 + def clone_image(self, volume, image_location, image_id, image_meta):
461 image_location = image_location[0] if image_location else None
462 - if image_location is None or not self._is_cloneable(image_location):
463 + if image_location is None or not self._is_cloneable(
464 + image_location, image_meta):
466 prefix, pool, image, snapshot = self._parse_location(image_location)
467 self._clone(volume, pool, image, snapshot)
468 diff --git a/cinder/volume/drivers/scality.py b/cinder/volume/drivers/scality.py
469 index 4cf49c6..abd6c29 100644
470 --- a/cinder/volume/drivers/scality.py
471 +++ b/cinder/volume/drivers/scality.py
472 @@ -250,7 +250,7 @@ class ScalityDriver(driver.VolumeDriver):
474 self.local_path(volume))
476 - def clone_image(self, volume, image_location, image_id):
477 + def clone_image(self, volume, image_location, image_id, image_meta):
478 """Create a volume efficiently from an existing image.
480 image_location is a string whose format depends on the
481 diff --git a/cinder/volume/flows/create_volume/__init__.py b/cinder/volume/flows/create_volume/__init__.py
482 index bb7acd3..e5aa371 100644
483 --- a/cinder/volume/flows/create_volume/__init__.py
484 +++ b/cinder/volume/flows/create_volume/__init__.py
485 @@ -1438,7 +1438,7 @@ class CreateVolumeFromSpecTask(base.CinderTask):
486 # dict containing provider_location for cloned volume
488 model_update, cloned = self.driver.clone_image(
489 - volume_ref, image_location, image_id)
490 + volume_ref, image_location, image_id, image_meta)
492 # TODO(harlowja): what needs to be rolled back in the clone if this
493 # volume create fails?? Likely this should be a subflow or broken