From bc52ae421921a9e62d118f8f31c150ff02451cb7 Mon Sep 17 00:00:00 2001 From: Chaynika Saikia Date: Thu, 6 Jul 2017 10:53:49 -0400 Subject: [PATCH] Add insufficient space async error in create vol cinder does not provide an async error message when a new volume is created from an image if there is insufficient space to download the image. The changes are made such that if there is an IOError in fetch() or insufficient space error from check_available_space() cinder message API generates an async error message for the user. A new exception ImageTooBig() has been created to handle this error and whenever this exception is come across, cinder generates an error message. Changes are made in the create_volume flow to handle this exception and generate the message. Changes are made in image_utils.py so that whenever there is not enough space, the appropriate exception is raised. New action and detail are added to message_field.py for this type of error. Change-Id: I184acd9c26d2a2a140c8205e4c93164204e8674d --- cinder/exception.py | 5 + cinder/image/image_utils.py | 20 +-- cinder/message/message_field.py | 13 +- cinder/tests/unit/test_image_utils.py | 17 +-- .../volume/flows/test_create_volume_flow.py | 136 +++++++++++++++++- cinder/tests/unit/volume/test_driver.py | 12 +- cinder/volume/driver.py | 7 + cinder/volume/flows/manager/create_volume.py | 83 +++++++---- 8 files changed, 234 insertions(+), 59 deletions(-) diff --git a/cinder/exception.py b/cinder/exception.py index 5fa23003631..ec70097e462 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -257,6 +257,11 @@ class ImageUnacceptable(Invalid): message = _("Image %(image_id)s is unacceptable: %(reason)s") +class ImageTooBig(Invalid): + message = _("Image %(image_id)s size exceeded available " + "disk space: %(reason)s") + + class DeviceUnavailable(Invalid): message = _("The device in the path %(path)s is unavailable: %(reason)s") diff --git a/cinder/image/image_utils.py b/cinder/image/image_utils.py index b43a502c9ce..a3a82417063 100644 --- a/cinder/image/image_utils.py +++ b/cinder/image/image_utils.py @@ -34,7 +34,6 @@ import tempfile from oslo_concurrency import processutils from oslo_config import cfg from oslo_log import log as logging -from oslo_utils import excutils from oslo_utils import fileutils from oslo_utils import imageutils from oslo_utils import timeutils @@ -215,14 +214,15 @@ def fetch(context, image_service, image_id, path, _user_id, _project_id): try: image_service.download(context, image_id, image_file) except IOError as e: - with excutils.save_and_reraise_exception(): - if e.errno == errno.ENOSPC: - # TODO(eharney): Fire an async error message for this - LOG.error("No space left in image_conversion_dir " - "path (%(path)s) while fetching " - "image %(image)s.", - {'path': os.path.dirname(path), - 'image': image_id}) + if e.errno == errno.ENOSPC: + params = {'path': os.path.dirname(path), + 'image': image_id} + reason = _("No space left in image_conversion_dir " + "path (%(path)s) while fetching " + "image %(image)s.") % params + LOG.exception(reason) + raise exception.ImageTooBig(image_id=image_id, + reason=reason) duration = timeutils.delta_seconds(start_time, timeutils.utcnow()) @@ -516,7 +516,7 @@ def check_available_space(dest, image_size, image_id): msg = ('There is no space to convert image. ' 'Requested: %(image_size)s, available: %(free_space)s' ) % {'image_size': image_size, 'free_space': free_space} - raise exception.ImageUnacceptable(image_id=image_id, reason=msg) + raise exception.ImageTooBig(image_id=image_id, reason=msg) def is_xenserver_format(image_meta): diff --git a/cinder/message/message_field.py b/cinder/message/message_field.py index 457614b7c1f..efdedcc5d2f 100644 --- a/cinder/message/message_field.py +++ b/cinder/message/message_field.py @@ -33,11 +33,13 @@ class Action(object): ATTACH_VOLUME = ('002', _('attach volume')) COPY_VOLUME_TO_IMAGE = ('003', _('copy volume to image')) UPDATE_ATTACHMENT = ('004', _('update attachment')) + COPY_IMAGE_TO_VOLUME = ('005', _('copy image to volume')) ALL = (SCHEDULE_ALLOCATE_VOLUME, ATTACH_VOLUME, COPY_VOLUME_TO_IMAGE, - UPDATE_ATTACHMENT) + UPDATE_ATTACHMENT, + COPY_IMAGE_TO_VOLUME) class Detail(object): @@ -55,13 +57,17 @@ class Detail(object): _("Volume's attach mode is invalid.")) QUOTA_EXCEED = ('006', _("Not enough quota resource for operation.")) + NOT_ENOUGH_SPACE_FOR_IMAGE = ('007', + _("Image used for creating volume exceeds " + "available space.")) ALL = (UNKNOWN_ERROR, DRIVER_NOT_INITIALIZED, NO_BACKEND_AVAILABLE, FAILED_TO_UPLOAD_VOLUME, VOLUME_ATTACH_MODE_INVALID, - QUOTA_EXCEED) + QUOTA_EXCEED, + NOT_ENOUGH_SPACE_FOR_IMAGE) # Exception and detail mappings EXCEPTION_DETAIL_MAPPINGS = { @@ -70,7 +76,8 @@ class Detail(object): VOLUME_ATTACH_MODE_INVALID: ['InvalidVolumeAttachMode'], QUOTA_EXCEED: ['ImageLimitExceeded', 'BackupLimitExceeded', - 'SnapshotLimitExceeded'] + 'SnapshotLimitExceeded'], + NOT_ENOUGH_SPACE_FOR_IMAGE: ['ImageTooBig'] } diff --git a/cinder/tests/unit/test_image_utils.py b/cinder/tests/unit/test_image_utils.py index 3358fc64768..0c1dfe2613d 100644 --- a/cinder/tests/unit/test_image_utils.py +++ b/cinder/tests/unit/test_image_utils.py @@ -25,7 +25,6 @@ from oslo_utils import units from cinder import exception from cinder.image import image_utils from cinder import test -from cinder.tests import fixtures from cinder.tests.unit import fake_constants as fake from cinder.volume import throttling @@ -298,28 +297,22 @@ class TestFetch(test.TestCase): .assert_called_once_with(None, None, None)) def test_fetch_enospc(self): - stdlog = self.useFixture(fixtures.StandardLogging()) - context = mock.sentinel.context image_service = mock.Mock() - e = IOError() + image_id = mock.sentinel.image_id + e = exception.ImageTooBig(image_id=image_id, reason = "fake") e.errno = errno.ENOSPC image_service.download.side_effect = e - image_id = mock.sentinel.image_id path = '/test_path' _user_id = mock.sentinel._user_id _project_id = mock.sentinel._project_id with mock.patch('cinder.image.image_utils.open', new=mock.mock_open(), create=True): - self.assertRaises(IOError, + self.assertRaises(exception.ImageTooBig, image_utils.fetch, context, image_service, image_id, path, _user_id, _project_id) - error_message = ('No space left in image_conversion_dir path ' - '(/) while fetching image %s.' % image_id) - - self.assertTrue(error_message in stdlog.logger.output) class TestVerifyImage(test.TestCase): @@ -1104,11 +1097,11 @@ class TestFetchToVolumeFormat(test.TestCase): data.backing_file = None data.virtual_size = units.Gi - mock_check_space.side_effect = exception.ImageUnacceptable( + mock_check_space.side_effect = exception.ImageTooBig( image_id='fake_image_id', reason='test') self.assertRaises( - exception.ImageUnacceptable, + exception.ImageTooBig, image_utils.fetch_to_volume_format, ctxt, image_service, image_id, dest, volume_format, blocksize, user_id=user_id, project_id=project_id, size=size, diff --git a/cinder/tests/unit/volume/flows/test_create_volume_flow.py b/cinder/tests/unit/volume/flows/test_create_volume_flow.py index 0def4f72d4a..bb451b0239a 100644 --- a/cinder/tests/unit/volume/flows/test_create_volume_flow.py +++ b/cinder/tests/unit/volume/flows/test_create_volume_flow.py @@ -24,6 +24,7 @@ from oslo_utils import imageutils from cinder import context from cinder import exception +from cinder.message import message_field from cinder import test from cinder.tests.unit.consistencygroup import fake_consistencygroup from cinder.tests.unit import fake_constants as fakes @@ -984,9 +985,11 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): self.internal_context.user_id = 'abc123' self.internal_context.project_id = 'def456' + @mock.patch('cinder.image.image_utils.check_available_space') def test_create_from_image_clone_image_and_skip_cache( - self, mock_get_internal_context, mock_create_from_img_dl, - mock_create_from_src, mock_handle_bootable, mock_fetch_img): + self, mock_check_space, mock_get_internal_context, + mock_create_from_img_dl, mock_create_from_src, + mock_handle_bootable, mock_fetch_img): self.mock_driver.clone_image.return_value = (None, True) volume = fake_volume.fake_volume_obj(self.ctxt, host='host@backend#pool') @@ -1009,6 +1012,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): image_meta, self.mock_image_service) + # Make sure check_available_space is always called + self.assertTrue(mock_check_space.called) + # Make sure clone_image is always called even if the cache is enabled self.assertTrue(self.mock_driver.clone_image.called) @@ -1026,8 +1032,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): ) @mock.patch('cinder.image.image_utils.qemu_img_info') + @mock.patch('cinder.image.image_utils.check_available_space') def test_create_from_image_cannot_use_cache( - self, mock_qemu_info, mock_get_internal_context, + self, mock_qemu_info, mock_check_space, mock_get_internal_context, mock_create_from_img_dl, mock_create_from_src, mock_handle_bootable, mock_fetch_img): mock_get_internal_context.return_value = None @@ -1058,6 +1065,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): image_meta, self.mock_image_service) + # Make sure check_available_space is always called + self.assertTrue(mock_check_space.called) + # Make sure clone_image is always called self.assertTrue(self.mock_driver.clone_image.called) @@ -1162,8 +1172,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 1}) self.assertEqual(volume_size, volume.size) + @mock.patch('cinder.image.image_utils.check_available_space') def test_create_from_image_bigger_size( - self, mock_get_internal_context, + self, mock_check_space, mock_get_internal_context, mock_create_from_img_dl, mock_create_from_src, mock_handle_bootable, mock_fetch_img): volume = fake_volume.fake_volume_obj(self.ctxt) @@ -1375,8 +1386,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): self.assertFalse(self.mock_cache.create_cache_entry.called) @mock.patch('cinder.image.image_utils.qemu_img_info') + @mock.patch('cinder.image.image_utils.check_available_space') def test_create_from_image_no_internal_context( - self, mock_qemu_info, mock_get_internal_context, + self, mock_chk_space, mock_qemu_info, mock_get_internal_context, mock_create_from_img_dl, mock_create_from_src, mock_handle_bootable, mock_fetch_img): self.mock_driver.clone_image.return_value = (None, False) @@ -1405,6 +1417,9 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): image_meta, self.mock_image_service) + # Make sure check_available_space is always called + self.assertTrue(mock_chk_space.called) + # Make sure clone_image is always called self.assertTrue(self.mock_driver.clone_image.called) @@ -1481,3 +1496,114 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase): # Make sure we didn't try and create a cache entry self.assertFalse(self.mock_cache.ensure_space.called) self.assertFalse(self.mock_cache.create_cache_entry.called) + + @mock.patch('cinder.image.image_utils.check_available_space') + @mock.patch('cinder.image.image_utils.qemu_img_info') + @mock.patch('cinder.message.api.API.create') + def test_create_from_image_insufficient_space( + self, mock_message_create, mock_qemu_info, mock_check_space, + mock_get_internal_context, + mock_create_from_img_dl, mock_create_from_src, + mock_handle_bootable, mock_fetch_img): + image_info = imageutils.QemuImgInfo() + image_info.virtual_size = '2147483648' + mock_qemu_info.return_value = image_info + self.mock_driver.clone_image.return_value = (None, False) + self.mock_cache.get_entry.return_value = None + + volume = fake_volume.fake_volume_obj(self.ctxt, size=1, + host='foo@bar#pool') + image_volume = fake_volume.fake_db_volume(size=2) + self.mock_db.volume_create.return_value = image_volume + + image_location = 'someImageLocationStr' + image_id = fakes.IMAGE_ID + image_meta = mock.MagicMock() + mock_check_space.side_effect = exception.ImageTooBig( + image_id=image_id, reason="fake") + + manager = create_volume_manager.CreateVolumeFromSpecTask( + self.mock_volume_manager, + self.mock_db, + self.mock_driver, + image_volume_cache=self.mock_cache + ) + + self.assertRaises( + exception.ImageTooBig, + manager._create_from_image, + self.ctxt, + volume, + image_location, + image_id, + image_meta, + self.mock_image_service + ) + + mock_message_create.assert_called_once_with( + self.ctxt, message_field.Action.COPY_IMAGE_TO_VOLUME, + resource_uuid=volume.id, + detail=message_field.Detail.NOT_ENOUGH_SPACE_FOR_IMAGE, + exception=mock.ANY) + + # The volume size should NOT be changed when in this case + self.assertFalse(self.mock_db.volume_update.called) + + # Make sure we didn't try and create a cache entry + self.assertFalse(self.mock_cache.ensure_space.called) + self.assertFalse(self.mock_cache.create_cache_entry.called) + + @mock.patch('cinder.image.image_utils.check_available_space') + @mock.patch('cinder.image.image_utils.qemu_img_info') + @mock.patch('cinder.message.api.API.create') + def test_create_from_image_cache_insufficient_size( + self, mock_message_create, mock_qemu_info, mock_check_space, + mock_get_internal_context, + mock_create_from_img_dl, mock_create_from_src, + mock_handle_bootable, mock_fetch_img): + image_info = imageutils.QemuImgInfo() + image_info.virtual_size = '1073741824' + mock_qemu_info.return_value = image_info + self.mock_driver.clone_image.return_value = (None, False) + self.mock_cache.get_entry.return_value = None + volume = fake_volume.fake_volume_obj(self.ctxt, size=1, + host='foo@bar#pool') + image_volume = fake_volume.fake_db_volume(size=2) + self.mock_db.volume_create.return_value = image_volume + image_id = fakes.IMAGE_ID + mock_create_from_img_dl.side_effect = exception.ImageTooBig( + image_id=image_id, reason="fake") + + image_location = 'someImageLocationStr' + image_meta = mock.MagicMock() + + manager = create_volume_manager.CreateVolumeFromSpecTask( + self.mock_volume_manager, + self.mock_db, + self.mock_driver, + image_volume_cache=self.mock_cache + ) + + self.assertRaises( + exception.ImageTooBig, + manager._create_from_image_cache_or_download, + self.ctxt, + volume, + image_location, + image_id, + image_meta, + self.mock_image_service + ) + + mock_message_create.assert_called_once_with( + self.ctxt, message_field.Action.COPY_IMAGE_TO_VOLUME, + resource_uuid=volume.id, + detail=message_field.Detail.NOT_ENOUGH_SPACE_FOR_IMAGE, + exception=mock.ANY) + + # The volume size should NOT be changed when in this case + self.assertFalse(self.mock_db.volume_update.called) + + # Make sure we didn't try and create a cache entry + self.assertFalse(self.mock_cache.ensure_space.called) + self.assertFalse(self.mock_cache.create_cache_entry.called) diff --git a/cinder/tests/unit/volume/test_driver.py b/cinder/tests/unit/volume/test_driver.py index 9925d21edc2..8379dbc804e 100644 --- a/cinder/tests/unit/volume/test_driver.py +++ b/cinder/tests/unit/volume/test_driver.py @@ -442,8 +442,12 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase): @mock.patch.object(cinder.volume.driver.VolumeDriver, '_detach_volume') @mock.patch.object(cinder.utils, 'brick_attach_volume_encryptor') @mock.patch.object(cinder.utils, 'brick_detach_volume_encryptor') + @ddt.data(exception.ImageUnacceptable( + reason='fake', image_id=fake.IMAGE_ID), + exception.ImageTooBig( + reason='fake image size exceeded', image_id=fake.IMAGE_ID)) def test_copy_image_to_encrypted_volume_failed_fetch( - self, + self, excep, mock_detach_encryptor, mock_attach_encryptor, mock_detach_volume, mock_attach_volume, mock_fetch_to_raw, mock_get_connector_properties): @@ -462,12 +466,10 @@ class GenericVolumeDriverTestCase(BaseDriverTestCase): mock_get_connector_properties.return_value = properties mock_attach_volume.return_value = [attach_info, volume] - raised_exception = exception.ImageUnacceptable(reason='fake', - image_id=fake.IMAGE_ID) - mock_fetch_to_raw.side_effect = raised_exception + mock_fetch_to_raw.side_effect = excep encryption = {'encryption_key_id': fake.ENCRYPTION_KEY_ID} - self.assertRaises(exception.ImageUnacceptable, + self.assertRaises(type(excep), self.volume.driver.copy_image_to_encrypted_volume, self.context, volume, image_service, fake.IMAGE_ID) diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 01a21c8608c..d3e8dd69b6d 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -804,6 +804,13 @@ class BaseVD(object): attach_info['device']['path'], self.configuration.volume_dd_blocksize, size=volume['size']) + except exception.ImageTooBig: + with excutils.save_and_reraise_exception(): + LOG.exception("Copying image %(image_id)s " + "to volume failed due to " + "insufficient available space.", + {'image_id': image_id}) + finally: if encrypted: utils.brick_detach_volume_encryptor(attach_info, diff --git a/cinder/volume/flows/manager/create_volume.py b/cinder/volume/flows/manager/create_volume.py index 02773c0805a..3cc8de31ac5 100644 --- a/cinder/volume/flows/manager/create_volume.py +++ b/cinder/volume/flows/manager/create_volume.py @@ -16,6 +16,7 @@ import traceback from oslo_concurrency import processutils from oslo_config import cfg from oslo_log import log as logging +from oslo_utils import excutils from oslo_utils import timeutils import taskflow.engines from taskflow.patterns import linear_flow @@ -28,6 +29,8 @@ from cinder import flow_utils from cinder.i18n import _ from cinder.image import glance from cinder.image import image_utils +from cinder.message import api as message_api +from cinder.message import message_field from cinder import objects from cinder.objects import consistencygroup from cinder import utils @@ -91,6 +94,7 @@ class OnFailureRescheduleTask(flow_utils.CinderTask): exception.SnapshotNotFound, exception.VolumeTypeNotFound, exception.ImageUnacceptable, + exception.ImageTooBig, ] def execute(self, **kwargs): @@ -372,6 +376,7 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): self.db = db self.driver = driver self.image_volume_cache = image_volume_cache + self.message = message_api.API() def _handle_bootable_volume_glance_meta(self, context, volume, **kwargs): @@ -557,6 +562,11 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): LOG.exception("Failed to copy image to volume: %(volume_id)s", {'volume_id': volume.id}) raise exception.ImageUnacceptable(ex) + except exception.ImageTooBig as ex: + LOG.exception("Failed to copy image %(image_id)s to volume: " + "%(volume_id)s", + {'volume_id': volume.id, 'image_id': image_id}) + excutils.save_and_reraise_exception() except Exception as ex: LOG.exception("Failed to copy image %(image_id)s to " "volume: %(volume_id)s", @@ -683,8 +693,14 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): "%(updates)s", {'volume_id': volume.id, 'updates': model_update}) - self._copy_image_to_volume(context, volume, image_meta, image_location, - image_service) + try: + self._copy_image_to_volume(context, volume, image_meta, + image_location, image_service) + except exception.ImageTooBig: + with excutils.save_and_reraise_exception(): + LOG.exception("Failed to copy image to volume " + "%(volume_id)s due to insufficient space", + {'volume_id': volume.id}) return model_update def _create_from_image_cache(self, context, internal_context, volume, @@ -757,28 +773,38 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): backend_name = volume_utils.extract_host(volume.service_topic_queue) try: if not cloned: - with image_utils.TemporaryImages.fetch( - image_service, context, image_id, - backend_name) as tmp_image: - # Try to create the volume as the minimal size, then we can - # extend once the image has been downloaded. - data = image_utils.qemu_img_info(tmp_image) + try: + with image_utils.TemporaryImages.fetch( + image_service, context, image_id, + backend_name) as tmp_image: + # Try to create the volume as the minimal size, + # then we can extend once the image has been + # downloaded. + data = image_utils.qemu_img_info(tmp_image) - virtual_size = image_utils.check_virtual_size( - data.virtual_size, volume.size, image_id) + virtual_size = image_utils.check_virtual_size( + data.virtual_size, volume.size, image_id) - if should_create_cache_entry: - if virtual_size and virtual_size != original_size: - volume.size = virtual_size - volume.save() - - model_update = self._create_from_image_download( - context, - volume, - image_location, - image_meta, - image_service - ) + if should_create_cache_entry: + if virtual_size and virtual_size != original_size: + volume.size = virtual_size + volume.save() + model_update = self._create_from_image_download( + context, + volume, + image_location, + image_meta, + image_service + ) + except exception.ImageTooBig as e: + with excutils.save_and_reraise_exception(): + self.message.create( + context, + message_field.Action.COPY_IMAGE_TO_VOLUME, + resource_uuid=volume.id, + detail= + message_field.Detail.NOT_ENOUGH_SPACE_FOR_IMAGE, + exception=e) if should_create_cache_entry: # Update the newly created volume db entry before we clone it @@ -817,8 +843,17 @@ class CreateVolumeFromSpecTask(flow_utils.CinderTask): if (CONF.image_conversion_dir and not os.path.exists(CONF.image_conversion_dir)): os.makedirs(CONF.image_conversion_dir) - image_utils.check_available_space(CONF.image_conversion_dir, - image_meta['size'], image_id) + try: + image_utils.check_available_space(CONF.image_conversion_dir, + image_meta['size'], image_id) + except exception.ImageTooBig as err: + with excutils.save_and_reraise_exception(): + self.message.create( + context, + message_field.Action.COPY_IMAGE_TO_VOLUME, + resource_uuid=volume.id, + detail=message_field.Detail.NOT_ENOUGH_SPACE_FOR_IMAGE, + exception=err) virtual_size = image_meta.get('virtual_size') if virtual_size: