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

@@ -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'])

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'])

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,
+    def _create_volume_from_image(self, expected_status, raw=False,
-                                 clone_works=True):
+                                  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):
+        def mock_clone_image(volume, image_location, image_id, image_meta):
-            return {'provider_location': None}, True
+            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):
+        # See tests.image.fake for image types.
-            raise exception.CinderException()
+        if raw:
-
+            image_id = '155d900f-4e14-4e4c-a73d-069cbf4541e6'
-        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)
         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)
+        mpo = mock.patch.object
-            self.assertEqual(volume['status'], expected_status)
+        with mpo(self.volume.driver, 'create_volume') as mock_create_volume:
-        finally:
+            with mpo(self.volume.driver, 'clone_image', mock_clone_image):
-            # cleanup
+                with mpo(create_volume.CreateVolumeFromSpecTask,
-            db.volume_destroy(self.context, volume_id)
+                         '_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_create_vol_from_image_status_available(self):
-        """Verify that before cloning, an image is in the available state."""
+        """Clone raw image then verify volume is in available state."""
-        self._clone_volume_from_image('available', True)
+        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."""
+        """Fail to clone raw image then verify volume is in error state."""
-        self._clone_volume_from_image('error', False)
+        self._create_volume_from_image('error', raw=True, clone_error=True)
 
-    def test_clone_image(self):
+    def test_clone_failure(self):
-        # Test Failure Case(s)
+        driver = self.volume.driver
-        expected = ({}, False)
 
-        self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: False)
+        with mock.patch.object(driver, '_is_cloneable', lambda *args: False):
-        image_loc = (object(), object())
+            image_loc = (mock.Mock(), mock.Mock())
-        actual = self.volume.driver.clone_image(object(), image_loc, object())
+            actual = driver.clone_image(mock.Mock(), image_loc,
-        self.assertEqual(expected, actual)
+                                        mock.Mock(), {})
+            self.assertEqual(({}, False), actual)
 
-        self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True)
+        self.assertEqual(({}, False),
-        self.assertEqual(expected,
+                         driver.clone_image(object(), None, None, {}))
-                         self.volume.driver.clone_image(object(), None, None))
-
-        # 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)
 
     def test_clone_success(self):
-        self.stubs.Set(self.volume.driver, '_is_cloneable', lambda x: True)
+        expected = ({'provider_location': None}, True)
-        self.stubs.Set(self.volume.driver, 'clone_image', lambda a, b, c: True)
+        driver = self.volume.driver
-        image_id = 'c905cedb-7281-47e4-8a62-f26bc5fc4c77'
+        mpo = mock.patch.object
-        self.assertTrue(self.volume.driver.clone_image({}, image_id, image_id))
+        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()

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()

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
         """

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):

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):

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

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)

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

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