From e066158b5235a3879fe90fa3bd813fc3363c01f5 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 based on change #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 | 156 +++++++++++------- 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, 127 insertions(+), 80 deletions(-) diff --git a/cinder/tests/test_gpfs.py b/cinder/tests/test_gpfs.py index 3dd7eaf1e..adb87c0e5 100644 --- a/cinder/tests/test_gpfs.py +++ b/cinder/tests/test_gpfs.py @@ -291,7 +291,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']) @@ -312,7 +313,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 397e671c0..a7f3b6841 100644 --- a/cinder/tests/test_netapp_nfs.py +++ b/cinder/tests/test_netapp_nfs.py @@ -481,7 +481,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): @@ -505,7 +505,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 @@ -541,7 +541,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): @@ -578,7 +578,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): @@ -617,7 +617,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']) @@ -664,7 +664,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 7b05ed1ab..43ab26162 100644 --- a/cinder/tests/test_rbd.py +++ b/cinder/tests/test_rbd.py @@ -35,6 +35,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__) @@ -310,7 +311,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') @@ -327,12 +329,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') @@ -347,7 +351,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 @@ -567,26 +580,31 @@ 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. - """ - def fake_clone_image(volume, image_location, image_id): - return {'provider_location': None}, True - def fake_clone_error(volume, image_location, image_id): - raise exception.CinderException() + NOTE: if clone_error is True we force the image type to raw otherwise + clone_image is not called + """ + 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 - 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: + image_id = '155d900f-4e14-4e4c-a73d-069cbf4541e6' else: - self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_error) + 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, @@ -596,58 +614,72 @@ 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) - 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) - - 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) + mpo = mock.patch.object + with mpo(self.volume.driver, 'create_volume') as mock_create_volume: + with mpo(self.volume.driver, 'clone_image', mock_clone_image): + with mpo(create_volume.CreateVolumeFromSpecTask, + '_copy_image_to_volume') as mock_copy_image_to_volume: + + 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) + + self.assertEqual(self.called, ['clone_image']) + mock_create_volume.assert_called() + mock_copy_image_to_volume.assert_called() - def test_clone_image(self): - # Test Failure Case(s) - expected = ({}, False) + def test_create_vol_from_image_status_available(self): + """Clone raw image then verify volume is in available state.""" + self._create_volume_from_image('available', raw=True) - 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) + 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) - self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True) - self.assertEqual(expected, - self.volume.driver.clone_image(object(), None, None)) + def test_create_vol_from_image_status_error(self): + """Fail to clone raw image then verify volume is in error state.""" + self._create_volume_from_image('error', raw=True, clone_error=True) - # Test Success Case(s) - expected = ({'provider_location': None}, True) + def test_clone_failure(self): + driver = self.volume.driver - self.stubs.Set(self.volume.driver, '_parse_location', - lambda x: ('a', 'b', 'c', 'd')) + with mock.patch.object(driver, '_is_cloneable', lambda *args: False): + 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, '_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) + self.assertEqual(({}, False), + driver.clone_image(object(), None, None, {})) 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) - image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77' - self.assertTrue(self.volume.driver.clone_image({}, image_id, image_id)) + expected = ({'provider_location': None}, True) + driver = self.volume.driver + mpo = mock.patch.object + with mpo(driver, '_is_cloneable', lambda *args: True): + with mpo(driver, '_parse_location', lambda x: (1, 2, 3, 4)): + with mpo(driver, '_clone') as mock_clone: + with mpo(driver, '_resize') as mock_resize: + image_loc = (mock.Mock(), mock.Mock()) + 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() diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 16057af31..11d5b28c4 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -1486,7 +1486,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 67c358557..45bd41783 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -400,7 +400,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 @@ -411,6 +411,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 407bd5956..97cdafd91 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 3529fe955..35c796862 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -317,7 +317,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 2c3dfba32..31b8667ae 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 88ffe0d1b..95d2ec697 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -717,7 +717,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: @@ -729,6 +729,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, @@ -741,9 +748,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 4cf49c6b7..abd6c2910 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 7f3a6efa2..5984b0ee8 100644 --- a/cinder/volume/flows/create_volume/__init__.py +++ b/cinder/volume/flows/create_volume/__init__.py @@ -1364,7 +1364,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 -- 2.45.2