From 73dac798e4d2650f83d6bafd1a59c53911d40aa9 Mon Sep 17 00:00:00 2001 From: Max Rasskazov Date: Fri, 21 Mar 2014 13:05:57 +0400 Subject: [PATCH] Patch MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch moved to code MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Patch has been added to specs by: > commit ecb1af2836444855774a19e2b2591990eed44217 > Author: Dmitry Burmistrov > Date: Tue Dec 3 16:12:39 2013 +0400 > > Add patch for convert non raw images according to OSCI-943 > Change-Id: I82cf1633267d85e591228c2e81b0be95d88168f3 Patch info: > 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 Change-request info: > Your change was committed before the commit hook was installed. > Amending the commit to add a gerrit change id. > remote: > remote: New Changes: > remote: http://gerrit.mirantis.com/13644 > remote: > To ssh://mrasskazov@gerrit.mirantis.com:29418/openstack/cinder.git > * [new branch] HEAD -> refs/publish/openstack-ci/fuel-5.0/2014.1/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch --- ...-clone-non-raw-images-in-rbd-backend.patch | 496 ------------------ debian/patches/series | 1 - ...-clone-non-raw-images-in-rbd-backend.patch | 496 ------------------ rpm/SPECS/openstack-cinder.spec | 2 - 4 files changed, 995 deletions(-) delete mode 100755 debian/patches/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch delete mode 100755 rpm/SOURCES/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch diff --git a/debian/patches/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch b/debian/patches/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch deleted file mode 100755 index 0b1f0850b..000000000 --- a/debian/patches/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch +++ /dev/null @@ -1,496 +0,0 @@ -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 - diff --git a/debian/patches/series b/debian/patches/series index be203b8c7..82e379d48 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -1,3 +1,2 @@ fix-sqlalchemy-requirements.patch fix-babel-requirements.patch -MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch diff --git a/rpm/SOURCES/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch b/rpm/SOURCES/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch deleted file mode 100755 index 0b1f0850b..000000000 --- a/rpm/SOURCES/MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch +++ /dev/null @@ -1,496 +0,0 @@ -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 - diff --git a/rpm/SPECS/openstack-cinder.spec b/rpm/SPECS/openstack-cinder.spec index e39dd511f..a973c2627 100644 --- a/rpm/SPECS/openstack-cinder.spec +++ b/rpm/SPECS/openstack-cinder.spec @@ -31,7 +31,6 @@ Patch0001: 0001-Ensure-we-don-t-access-the-net-when-building-docs.patch Patch0002: 0002-Use-updated-parallel-install-versions-of-epel-packag.patch Patch0003: 0003-Remove-runtime-dep-on-python-pbr-python-d2to1.patch Patch0004: 0004-Revert-Use-oslo.sphinx-and-remove-local-copy-of-doc-.patch -Patch0005: MIRA-Do-not-clone-non-raw-images-in-rbd-backend.patch BuildArch: noarch BuildRequires: intltool @@ -148,7 +147,6 @@ This package contains documentation files for cinder. %patch0002 -p1 %patch0003 -p1 %patch0004 -p1 -%patch0005 -p1 find . \( -name .gitignore -o -name .placeholder \) -delete -- 2.45.2