Skip to content

Commit

Permalink
Set bootable flag for volume cloned from image
Browse files Browse the repository at this point in the history 
In previous code we don't set the bootable flag for volume
cloned from image. This does not appear to break booting
from volume at present but the status displayed by the cinder
client is broken.

The bug is reported and fixed several months ago(bug #1185533)
but the author doesn't provide unit test with the patch. Now it
has been broken again by somebody else.The patch re-fixed this
bug together with unit tests.

Also add a function named _handle_bootable_volume_glance_meta,
it is a combination of enable_bootable_flag and glance_metadata
handling. There are 3 kinds of volume creation tasks may required
to copy/fetch the glance metadata. In previous code every kind of
task have its own handling code, this patch consolidate them together
to share some common code.

fixed bug #1211272
fixed bug #1185533

Change-Id: I86007e26efbebe58dc0c5995b6f14f68258144b5
  • Loading branch information
@xiaoxichen819 @openstack-gerrit
xiaoxichen819 authored and openstack-gerrit committed Aug 21, 2013
1 parent d2b48cd commit 162aa28
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 58 deletions.
40 changes: 32 additions & 8 deletions cinder/tests/test_volume.py
View file
Expand Up @@ -913,9 +913,13 @@ def test_delete_busy_snapshot(self):
self.volume.delete_snapshot(self.context, snapshot_id)
self.volume.delete_volume(self.context, volume_id)

def _create_volume_from_image(self, fakeout_copy_image_to_volume=False):
"""Call copy image to volume, Test the status of volume after calling
copying image to volume.
def _create_volume_from_image(self, fakeout_copy_image_to_volume=False,
fakeout_clone_image=False):
"""Test function of create_volume_from_image.
Test cases call this function to create a volume from image, caller
can choose whether to fake out copy_image_to_volume and conle_image,
after calling this, test cases should check status of the volume.
"""
def fake_local_path(volume):
return dst_path
Expand All @@ -927,9 +931,14 @@ def fake_copy_image_to_volume(context, volume,
def fake_fetch_to_raw(context, image_service, image_id, vol_path):
pass

def fake_clone_image(volume_ref, image_location, image_id):
return {'provider_location': None}, True

dst_fd, dst_path = tempfile.mkstemp()
os.close(dst_fd)
self.stubs.Set(self.volume.driver, 'local_path', fake_local_path)
if fakeout_clone_image:
self.stubs.Set(self.volume.driver, 'clone_image', fake_clone_image)
self.stubs.Set(image_utils, 'fetch_to_raw', fake_fetch_to_raw)
if fakeout_copy_image_to_volume:
self.stubs.Set(self.volume, '_copy_image_to_volume',
Expand All @@ -948,17 +957,31 @@ def fake_fetch_to_raw(context, image_service, image_id, vol_path):
volume = db.volume_get(self.context, volume_id)
return volume

def test_create_volume_from_image_status_available(self):
"""Verify that before copying image to volume, it is in available
state.
def test_create_volume_from_image_cloned_status_available(self):
"""Test create volume from image via cloning.
Verify that after cloning image to volume, it is in available
state and is bootable.
"""
volume = self._create_volume_from_image()
self.assertEqual(volume['status'], 'available')
self.assertEqual(volume['bootable'], True)
self.volume.delete_volume(self.context, volume['id'])

def test_create_volume_from_image_not_cloned_status_available(self):
"""Test create volume from image via full copy.
Verify that after copying image to volume, it is in available
state and is bootable.
"""
volume = self._create_volume_from_image(fakeout_clone_image=True)
self.assertEqual(volume['status'], 'available')
self.assertEqual(volume['bootable'], True)
self.volume.delete_volume(self.context, volume['id'])

def test_create_volume_from_image_exception(self):
"""Verify that create volume from image, the volume status is
'downloading'.
"""Verify that create volume from a non-existing image, the volume
status is 'error' and is not bootable.
"""
dst_fd, dst_path = tempfile.mkstemp()
os.close(dst_fd)
Expand All @@ -984,6 +1007,7 @@ def test_create_volume_from_image_exception(self):
image_id)
volume = db.volume_get(self.context, volume_id)
self.assertEqual(volume['status'], "error")
self.assertEqual(volume['bootable'], False)
# cleanup
db.volume_destroy(self.context, volume_id)
os.unlink(dst_path)
Expand Down
112 changes: 62 additions & 50 deletions cinder/volume/flows/create_volume.py
View file
Expand Up @@ -1215,6 +1215,60 @@ def __init__(self, db, host, driver):
}
self.host = host

def _handle_bootable_volume_glance_meta(self, context, volume_id,
**kwargs):
"""Enable bootable flag and properly handle glance metadata.
Caller should provide one and only one of snapshot_id,source_volid
and image_id. If an image_id specified, a image_meta should also be
provided, otherwise will be treated as an empty dictionary.
"""

log_template = _("Copying metadata from %(src_type)s %(src_id)s to "
"%(vol_id)s")
exception_template = _("Failed updating volume %(vol_id)s metadata"
" using the provided %(src_type)s"
" %(src_id)s metadata")
src_type = None
src_id = None
self._enable_bootable_flag(context, volume_id)
try:
if kwargs.get('snapshot_id'):
src_type = 'snapshot'
src_id = kwargs['snapshot_id']
snapshot_id = src_id
LOG.debug(log_template % {'src_type': src_type,
'src_id': src_id,
'vol_id': volume_id})
self.db.volume_glance_metadata_copy_to_volume(
context, volume_id, snapshot_id)
elif kwargs.get('source_volid'):
src_type = 'source volume'
src_id = kwargs['source_volid']
source_volid = src_id
LOG.debug(log_template % {'src_type': src_type,
'src_id': src_id,
'vol_id': volume_id})
self.db.volume_glance_metadata_copy_from_volume_to_volume(
context,
source_volid,
volume_id)
elif kwargs.get('image_id'):
src_type = 'image'
src_id = kwargs['image_id']
image_id = src_id
image_meta = kwargs.get('image_meta', {})
LOG.debug(log_template % {'src_type': src_type,
'src_id': src_id,
'vol_id': volume_id})
self._capture_volume_image_metadata(context, volume_id,
image_id, image_meta)
except exception.CinderException as ex:
LOG.exception(exception_template % {'src_type': src_type,
'src_id': src_id,
'vol_id': volume_id})
raise exception.MetadataCopyFailure(reason=ex)

def _create_from_snapshot(self, context, volume_ref, snapshot_id,
**kwargs):
volume_id = volume_ref['id']
Expand All @@ -1237,21 +1291,8 @@ def _create_from_snapshot(self, context, volume_ref, snapshot_id,
'snapshot_ref_id': snapshot_ref['volume_id']})
raise exception.MetadataUpdateFailure(reason=ex)
if make_bootable:
self._enable_bootable_flag(context, volume_id)
try:
LOG.debug(_("Copying metadata from snapshot "
"%(snap_volume_id)s to %(volume_id)s") %
{'snap_volume_id': snapshot_id,
'volume_id': volume_id})
self.db.volume_glance_metadata_copy_to_volume(
context, volume_id, snapshot_id)
except exception.CinderException as ex:
LOG.exception(_("Failed updating volume %(volume_id)s "
"metadata using the provided glance "
"snapshot %(snapshot_id)s metadata") %
{'volume_id': volume_id,
'snapshot_id': snapshot_id})
raise exception.MetadataCopyFailure(reason=ex)
self._handle_bootable_volume_glance_meta(context, volume_id,
snapshot_id=snapshot_id)
return model_update

def _enable_bootable_flag(self, context, volume_id):
Expand All @@ -1277,25 +1318,8 @@ def _create_from_source_volume(self, context, volume_ref,
# point the volume has already been created and further failures
# will not destroy the volume (although they could in the future).
if srcvol_ref.bootable:
self._enable_bootable_flag(context, volume_ref['id'])
try:
LOG.debug(_('Copying metadata from source volume '
'%(source_volid)s to cloned volume '
'%(clone_vol_id)s') % {
'source_volid': source_volid,
'clone_vol_id': volume_ref['id'],
})
self.db.volume_glance_metadata_copy_from_volume_to_volume(
context,
source_volid,
volume_ref['id'])
except exception.CinderException as ex:
LOG.exception(_("Failed updating cloned volume %(volume_id)s"
" metadata using the provided source volumes"
" %(source_volid)s metadata") %
{'volume_id': volume_ref['id'],
'source_volid': source_volid})
raise exception.MetadataCopyFailure(reason=ex)
self._handle_bootable_volume_glance_meta(context, volume_ref['id'],
source_volid=source_volid)
return model_update

def _copy_image_to_volume(self, context, volume_ref,
Expand Down Expand Up @@ -1334,8 +1358,6 @@ def _copy_image_to_volume(self, context, volume_ref,

def _capture_volume_image_metadata(self, context, volume_id,
image_id, image_meta):
if not image_meta:
image_meta = {}

# Save some base attributes into the volume metadata
base_metadata = {
Expand Down Expand Up @@ -1391,7 +1413,6 @@ def _create_from_image(self, context, volume_ref,
# and clone status.
model_update, cloned = self.driver.clone_image(
volume_ref, image_location, image_id)
make_bootable = False
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
Expand All @@ -1413,19 +1434,10 @@ def _create_from_image(self, context, volume_ref,
'updates': updates})
self._copy_image_to_volume(context, volume_ref,
image_id, image_location, image_service)
make_bootable = True
if make_bootable:
self._enable_bootable_flag(context, volume_ref['id'])
try:
self._capture_volume_image_metadata(context, volume_ref['id'],
image_id, image_meta)
except exception.CinderException as ex:
LOG.exception(_("Failed updating volume %(volume_id)s metadata"
" using the provided image metadata"
" %(image_meta)s from image %(image_id)s") %
{'volume_id': volume_ref['id'],
'image_meta': image_meta, 'image_id': image_id})
raise exception.MetadataUpdateFailure(reason=ex)

self._handle_bootable_volume_glance_meta(context, volume_ref['id'],
image_id=image_id,
image_meta=image_meta)
return model_update

def _create_raw_volume(self, context, volume_ref, **kwargs):
Expand Down

0 comments on commit 162aa28

Please sign in to comment.