From fd7e9dd59ffa346bed5a11e5312f4bb1bf114ab4 Mon Sep 17 00:00:00 2001 From: Dmitry Borodaenko Date: Wed, 27 Nov 2013 14:33:00 -0800 Subject: [PATCH] Do not clone non-raw images in rbd backend RBD backend only supports booting from images in raw format. A volume that was cloned from an image in any other format is not bootable. The RBD driver will consider non-raw images to be uncloneable to trigger automatic conversion to raw format. Includes conversion of the corresponding unit test to use mock (instead of mox) and expanded comments and error messages from patchset #58893 by Edward Hope-Morley. Change-Id: I5725d2f7576bc1b3e9b874ba944ad17d33a6e2cb Closes-Bug: #1246219 Closes-Bug: #1247998 --- cinder/tests/test_gpfs.py | 6 +- cinder/tests/test_netapp_nfs.py | 12 +- cinder/tests/test_rbd.py | 168 +++++++++++++++++--------- cinder/tests/test_volume.py | 2 +- cinder/volume/driver.py | 7 +- cinder/volume/drivers/gpfs.py | 2 +- cinder/volume/drivers/lvm.py | 2 +- cinder/volume/drivers/netapp/nfs.py | 2 +- cinder/volume/drivers/rbd.py | 14 ++- cinder/volume/drivers/scality.py | 2 +- cinder/volume/flows/create_volume/__init__.py | 2 +- 11 files changed, 144 insertions(+), 75 deletions(-) diff --git a/cinder/tests/test_gpfs.py b/cinder/tests/test_gpfs.py index 4fdb788..1f47c6b 100644 --- a/cinder/tests/test_gpfs.py +++ b/cinder/tests/test_gpfs.py @@ -288,7 +288,8 @@ class GPFSDriverTestCase(test.TestCase): CONF.gpfs_images_share_mode = 'copy_on_write' self.driver.clone_image(volume, None, - self.image_id) + self.image_id, + {}) self.assertTrue(os.path.exists(volumepath)) self.volume.delete_volume(self.context, volume['id']) @@ -309,7 +310,8 @@ class GPFSDriverTestCase(test.TestCase): CONF.gpfs_images_share_mode = 'copy' self.driver.clone_image(volume, None, - self.image_id) + self.image_id, + {}) self.assertTrue(os.path.exists(volumepath)) self.volume.delete_volume(self.context, volume['id']) diff --git a/cinder/tests/test_netapp_nfs.py b/cinder/tests/test_netapp_nfs.py index 950efc8..042280e 100644 --- a/cinder/tests/test_netapp_nfs.py +++ b/cinder/tests/test_netapp_nfs.py @@ -469,7 +469,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase): drv._post_clone_image(volume) mox.ReplayAll() - drv. clone_image(volume, ('image_location', None), 'image_id') + drv.clone_image(volume, ('image_location', None), 'image_id', {}) mox.VerifyAll() def get_img_info(self, format): @@ -493,7 +493,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase): mox.ReplayAll() (prop, cloned) = drv. clone_image( - volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id') + volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id', {}) mox.VerifyAll() if not cloned and not prop['provider_location']: pass @@ -529,7 +529,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase): mox.ReplayAll() drv. clone_image( - volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id') + volume, ('nfs://127.0.0.1:/share/img-id', None), 'image_id', {}) mox.VerifyAll() def test_clone_image_cloneableshare_notraw(self): @@ -566,7 +566,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase): mox.ReplayAll() drv. clone_image( - volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id') + volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id', {}) mox.VerifyAll() def test_clone_image_file_not_discovered(self): @@ -605,7 +605,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase): mox.ReplayAll() vol_dict, result = drv. clone_image( - volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id') + volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id', {}) mox.VerifyAll() self.assertFalse(result) self.assertFalse(vol_dict['bootable']) @@ -652,7 +652,7 @@ class NetappDirectCmodeNfsDriverTestCase(test.TestCase): mox.ReplayAll() vol_dict, result = drv. clone_image( - volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id') + volume, ('nfs://127.0.0.1/share/img-id', None), 'image_id', {}) mox.VerifyAll() self.assertFalse(result) self.assertFalse(vol_dict['bootable']) diff --git a/cinder/tests/test_rbd.py b/cinder/tests/test_rbd.py index 60de09b..054bbb5 100644 --- a/cinder/tests/test_rbd.py +++ b/cinder/tests/test_rbd.py @@ -34,6 +34,7 @@ from cinder.tests.test_volume import DriverTestCase from cinder import units from cinder.volume import configuration as conf import cinder.volume.drivers.rbd as driver +from cinder.volume.flows import create_volume LOG = logging.getLogger(__name__) @@ -247,7 +248,8 @@ class RBDTestCase(test.TestCase): self.assertRaises(exception.ImageUnacceptable, self.driver._parse_location, loc) - self.assertFalse(self.driver._is_cloneable(loc)) + self.assertFalse( + self.driver._is_cloneable(loc, {'disk_format': 'raw'})) def test_cloneable(self): self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc') @@ -264,12 +266,14 @@ class RBDTestCase(test.TestCase): self.mox.ReplayAll() - self.assertTrue(self.driver._is_cloneable(location)) + self.assertTrue( + self.driver._is_cloneable(location, {'disk_format': 'raw'})) def test_uncloneable_different_fsid(self): self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc') location = 'rbd://def/pool/image/snap' - self.assertFalse(self.driver._is_cloneable(location)) + self.assertFalse( + self.driver._is_cloneable(location, {'disk_format': 'raw'})) def test_uncloneable_unreadable(self): self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc') @@ -284,7 +288,16 @@ class RBDTestCase(test.TestCase): self.mox.ReplayAll() - self.assertFalse(self.driver._is_cloneable(location)) + self.assertFalse( + self.driver._is_cloneable(location, {'disk_format': 'raw'})) + + def test_uncloneable_bad_format(self): + self.stubs.Set(self.driver, '_get_fsid', lambda: 'abc') + location = 'rbd://abc/pool/image/snap' + formats = ['qcow2', 'vmdk', 'vdi'] + for f in formats: + self.assertFalse( + self.driver._is_cloneable(location, {'disk_format': f})) def _copy_image(self): @contextlib.contextmanager @@ -504,26 +517,37 @@ class ManagedRBDTestCase(DriverTestCase): super(ManagedRBDTestCase, self).setUp() fake_image.stub_out_image_service(self.stubs) self.volume.driver.set_initialized() + self.called = [] - def _clone_volume_from_image(self, expected_status, - clone_works=True): + def _create_volume_from_image(self, expected_status, raw=False, + clone_error=False): """Try to clone a volume from an image, and check the status afterwards. + + NOTE: if clone_error is True we force the image type to raw otherwise + clone_image is not called """ - def fake_clone_image(volume, image_location, image_id): - return {'provider_location': None}, True + def mock_clone_image(volume, image_location, image_id, image_meta): + self.called.append('clone_image') + if clone_error: + raise exception.CinderException() + else: + return {'provider_location': None}, True - def fake_clone_error(volume, image_location, image_id): - raise exception.CinderException() + if clone_error: + raw = True - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True) - if clone_works: - self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_image) + # See tests.image.fake for image types. + if raw: + expected_calls = ['clone_image'] + image_id = '155d900f-4e14-4e4c-a73d-069cbf4541e6' else: - self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_error) + expected_calls = ['clone_image', 'create_volume', + 'copy_image_to_volume'] + image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' - image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' volume_id = 1 + # creating volume testdata db.volume_create(self.context, {'id': volume_id, @@ -533,58 +557,88 @@ class ManagedRBDTestCase(DriverTestCase): 'status': 'creating', 'instance_uuid': None, 'host': 'dummy'}) - try: - if clone_works: - self.volume.create_volume(self.context, - volume_id, - image_id=image_id) - else: - self.assertRaises(exception.CinderException, - self.volume.create_volume, - self.context, - volume_id, - image_id=image_id) - - volume = db.volume_get(self.context, volume_id) - self.assertEqual(volume['status'], expected_status) - finally: - # cleanup - db.volume_destroy(self.context, volume_id) + + with mock.patch.object(self.volume.driver, 'create_volume') as \ + mock_create_volume: + with mock.patch.object(self.volume.driver, 'clone_image', + mock_clone_image): + with mock.patch.object(create_volume.CreateVolumeFromSpecTask, + '_copy_image_to_volume') as \ + mock_copy_image_to_volume: + self.volume.driver._is_cloneable = mock.Mock() + self.volume.driver._is_cloneable.return_value = True + + try: + if not clone_error: + self.volume.create_volume(self.context, + volume_id, + image_id=image_id) + else: + self.assertRaises(exception.CinderException, + self.volume.create_volume, + self.context, + volume_id, + image_id=image_id) + + volume = db.volume_get(self.context, volume_id) + self.assertEqual(volume['status'], expected_status) + finally: + # cleanup + db.volume_destroy(self.context, volume_id) + + if raw: + self.assertEquals(self.called, ['clone_image']) + + mock_create_volume.assert_called() + mock_copy_image_to_volume.assert_called() def test_create_vol_from_image_status_available(self): - """Verify that before cloning, an image is in the available state.""" - self._clone_volume_from_image('available', True) + """Clone raw image then verify volume is in available state.""" + self._create_volume_from_image('available', raw=True) + + def test_create_vol_from_non_raw_image_status_available(self): + """Clone non-raw image then verify volume is in available state.""" + self._create_volume_from_image('available', raw=False) def test_create_vol_from_image_status_error(self): - """Verify that before cloning, an image is in the available state.""" - self._clone_volume_from_image('error', False) + """Clone raw image with failure then verify volume is in error + state. + """ + self._create_volume_from_image('error', raw=True, clone_error=True) def test_clone_image(self): - # Test Failure Case(s) - expected = ({}, False) + driver = self.volume.driver - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: False) - image_loc = (object(), object()) - actual = self.volume.driver.clone_image(object(), image_loc, object()) - self.assertEqual(expected, actual) + # Test uncloneable case(s) + with mock.patch.object(driver, '_is_cloneable', + lambda *args: False) as mock_is_cloneable: + image_loc = (mock.Mock(), mock.Mock()) + actual = driver.clone_image(mock.Mock(), image_loc, + mock.Mock(), {}) + self.assertEqual(({}, False), actual) - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True) - self.assertEqual(expected, - self.volume.driver.clone_image(object(), None, None)) + self.assertEqual(({}, False), + driver.clone_image(object(), None, None, {})) - # Test Success Case(s) + # Test success case(s) expected = ({'provider_location': None}, True) - - self.stubs.Set(self.volume.driver, '_parse_location', - lambda x: ('a', 'b', 'c', 'd')) - - self.stubs.Set(self.volume.driver, '_clone', lambda *args: None) - self.stubs.Set(self.volume.driver, '_resize', lambda *args: None) - actual = self.volume.driver.clone_image(object(), image_loc, object()) - self.assertEqual(expected, actual) + mpo = mock.patch.object + with mpo(driver, '_is_cloneable', lambda *args: True): + with mpo(driver, '_parse_location', + lambda x: ('a', 'b', 'c', 'd')): + with mpo(driver, '_clone') as mock_clone: + with mpo(driver, '_resize') as mock_resize: + actual = driver.clone_image(mock.Mock(), image_loc, + mock.Mock(), + {'disk_format': 'raw'}) + self.assertEqual(expected, actual) + mock_clone.assert_called() + mock_resize.assert_called() def test_clone_success(self): - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True) - self.stubs.Set(self.volume.driver, 'clone_image', lambda a, b, c: True) + self.stubs.Set(self.volume.driver, '_is_cloneable', lambda *args: True) + self.stubs.Set(self.volume.driver, 'clone_image', + lambda a, b, c, d: True) image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' - self.assertTrue(self.volume.driver.clone_image({}, image_id, image_id)) + self.assertTrue(self.volume.driver.clone_image({}, + image_id, image_id, {})) diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 4a0d475..928799f 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -1216,7 +1216,7 @@ class VolumeTestCase(BaseVolumeTestCase): def fake_fetch_to_raw(ctx, image_service, image_id, path, size=None): pass - def fake_clone_image(volume_ref, image_location, image_id): + def fake_clone_image(volume_ref, image_location, image_id, image_meta): return {'provider_location': None}, True dst_fd, dst_path = tempfile.mkstemp() diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 21dd12b..dd2be3c 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -396,7 +396,7 @@ class VolumeDriver(object): connector.disconnect_volume(attach_info['conn']['data'], attach_info['device']) - def clone_image(self, volume, image_location, image_id): + def clone_image(self, volume, image_location, image_id, image_meta): """Create a volume efficiently from an existing image. image_location is a string whose format depends on the @@ -407,6 +407,11 @@ class VolumeDriver(object): It can be used by the driver to introspect internal stores or registry to do an efficient image clone. + image_meta is a dictionary that includes 'disk_format' (e.g. + raw, qcow2) and other image attributes that allow drivers to + decide whether they can clone the image without first requiring + conversion. + Returns a dict of volume properties eg. provider_location, boolean indicating whether cloning occurred """ diff --git a/cinder/volume/drivers/gpfs.py b/cinder/volume/drivers/gpfs.py index 9a1a397..8792ad8 100644 --- a/cinder/volume/drivers/gpfs.py +++ b/cinder/volume/drivers/gpfs.py @@ -463,7 +463,7 @@ class GPFSDriver(driver.VolumeDriver): return '100M' return '%sG' % size_in_g - def clone_image(self, volume, image_location, image_id): + def clone_image(self, volume, image_location, image_id, image_meta): return self._clone_image(volume, image_location, image_id) def _is_cloneable(self, image_id): diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 094436f..c6023eb 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -322,7 +322,7 @@ class LVMVolumeDriver(driver.VolumeDriver): finally: self.delete_snapshot(temp_snapshot) - def clone_image(self, volume, image_location, image_id): + def clone_image(self, volume, image_location, image_id, image_meta): return None, False def backup_volume(self, context, backup, backup_service): diff --git a/cinder/volume/drivers/netapp/nfs.py b/cinder/volume/drivers/netapp/nfs.py index 602a1dc..9463137 100644 --- a/cinder/volume/drivers/netapp/nfs.py +++ b/cinder/volume/drivers/netapp/nfs.py @@ -374,7 +374,7 @@ class NetAppNFSDriver(nfs.NfsDriver): LOG.warning(_('Exception during deleting %s'), ex.__str__()) return False - def clone_image(self, volume, image_location, image_id): + def clone_image(self, volume, image_location, image_id, image_meta): """Create a volume efficiently from an existing image. image_location is a string whose format depends on the diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 775ab16..7ed59d0 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -706,7 +706,7 @@ class RBDDriver(driver.VolumeDriver): with RADOSClient(self) as client: return client.cluster.get_fsid() - def _is_cloneable(self, image_location): + def _is_cloneable(self, image_location, image_meta): try: fsid, pool, image, snapshot = self._parse_location(image_location) except exception.ImageUnacceptable as e: @@ -718,6 +718,13 @@ class RBDDriver(driver.VolumeDriver): LOG.debug(reason) return False + if image_meta['disk_format'] != 'raw': + reason = _("rbd image clone requires image format to be " + "'raw' but image {0} is '{1}'").format( + image_location, image_meta['disk_format']) + LOG.debug(reason) + return False + # check that we can read the image try: with RBDVolumeProxy(self, image, @@ -730,9 +737,10 @@ class RBDDriver(driver.VolumeDriver): dict(loc=image_location, err=e)) return False - def clone_image(self, volume, image_location, image_id): + def clone_image(self, volume, image_location, image_id, image_meta): image_location = image_location[0] if image_location else None - if image_location is None or not self._is_cloneable(image_location): + if image_location is None or not self._is_cloneable( + image_location, image_meta): return ({}, False) prefix, pool, image, snapshot = self._parse_location(image_location) self._clone(volume, pool, image, snapshot) diff --git a/cinder/volume/drivers/scality.py b/cinder/volume/drivers/scality.py index 4cf49c6..abd6c29 100644 --- a/cinder/volume/drivers/scality.py +++ b/cinder/volume/drivers/scality.py @@ -250,7 +250,7 @@ class ScalityDriver(driver.VolumeDriver): image_meta, self.local_path(volume)) - def clone_image(self, volume, image_location, image_id): + def clone_image(self, volume, image_location, image_id, image_meta): """Create a volume efficiently from an existing image. image_location is a string whose format depends on the diff --git a/cinder/volume/flows/create_volume/__init__.py b/cinder/volume/flows/create_volume/__init__.py index bb7acd3..e5aa371 100644 --- a/cinder/volume/flows/create_volume/__init__.py +++ b/cinder/volume/flows/create_volume/__init__.py @@ -1438,7 +1438,7 @@ class CreateVolumeFromSpecTask(base.CinderTask): # dict containing provider_location for cloned volume # and clone status. model_update, cloned = self.driver.clone_image( - volume_ref, image_location, image_id) + volume_ref, image_location, image_id, image_meta) if not cloned: # TODO(harlowja): what needs to be rolled back in the clone if this # volume create fails?? Likely this should be a subflow or broken -- 1.8.5.1