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
This commit is contained in:
parent
1622b284c6
commit
e066158b52
|
@ -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'])
|
||||
|
|
|
@ -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'])
|
||||
|
|
|
@ -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.
|
||||
|
||||
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()
|
||||
|
||||
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)
|
||||
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_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)
|
||||
"""Fail to clone raw image 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)
|
||||
def test_clone_failure(self):
|
||||
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)
|
||||
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, '_is_cloneable', lambda x: True)
|
||||
self.assertEqual(expected,
|
||||
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)
|
||||
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()
|
||||
|
|
|
@ -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()
|
||||
|
|
|
@ -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
|
||||
"""
|
||||
|
|
|
@ -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):
|
||||
|
|
|
@ -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):
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue