From 25a2970124d8be372ae366ed8bb9bbacc604271f Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Tue, 9 Apr 2024 08:54:55 +0530 Subject: [PATCH] Fix reimage with snapshot backed image When we create a snapshot of a server that is volume-backed, glance creates a zero sized metadata entry of the image which is backed by the volume snapshot. When we try to reimage a volume with that given image, we need to fetch the volume snapshot details from the image metadata. Now to reimage, we need to create a new volume from snapshot and copy the data from our snapshot-volume to our original volume for which we use the generic revert to snapshot mechanism as it performs the same steps. Closes-Bug: #2062539 Change-Id: Ic4bf44c320ad53b514178ecd4d5f57f037169bfe (cherry picked from commit ae22195df7d674ba017f5e301868522ccb21c5b4) --- cinder/api/contrib/volume_actions.py | 41 ++++++++- .../unit/api/contrib/test_volume_actions.py | 88 ++++++++++++++++++- cinder/tests/unit/volume/test_rpcapi.py | 9 +- .../tests/unit/volume/test_volume_reimage.py | 37 +++++++- cinder/volume/api.py | 17 ++-- cinder/volume/manager.py | 28 ++++-- cinder/volume/rpcapi.py | 14 ++- ...x-reimage-image-snap-15ecd5fce9973d5d.yaml | 5 ++ 8 files changed, 213 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/fix-reimage-image-snap-15ecd5fce9973d5d.yaml diff --git a/cinder/api/contrib/volume_actions.py b/cinder/api/contrib/volume_actions.py index 0bb7ed1b875..6943cbc8e52 100644 --- a/cinder/api/contrib/volume_actions.py +++ b/cinder/api/contrib/volume_actions.py @@ -26,6 +26,7 @@ from cinder.api.schemas import volume_actions as volume_action from cinder.api import validation from cinder import exception from cinder.i18n import _ +from cinder.image import glance from cinder.policies import volume_actions as policy from cinder import volume from cinder.volume import volume_utils @@ -327,6 +328,42 @@ class VolumeActionsController(wsgi.Controller): self.volume_api.update(context, volume, update_dict) + def _get_image_snapshot_and_check_size(self, context, image_uuid, + volume_size): + image_snapshot = None + if image_uuid: + image_service = glance.get_default_image_service() + image_meta = image_service.show(context, image_uuid) + if image_meta is not None: + bdms = image_meta.get('properties', {}).get( + 'block_device_mapping', []) + if bdms: + boot_bdm = [bdm for bdm in bdms if ( + bdm.get('source_type') == 'snapshot' and + bdm.get('boot_index') == 0)] + if boot_bdm: + try: + # validate size + image_snap_size = boot_bdm[0].get('volume_size') + if image_snap_size > volume_size: + msg = (_( + "Volume size must be greater than the " + "image size. (Image: %(img_size)s, " + "Volume: %(vol_size)s).") % { + 'img_size': image_snap_size, + 'vol_size': volume_size}) + raise webob.exc.HTTPBadRequest(explanation=msg) + image_snapshot = self.volume_api.get_snapshot( + context, boot_bdm[0].get('snapshot_id')) + except exception.NotFound: + explanation = _( + 'Nova specific image is found, but boot ' + 'volume snapshot id:%s not found.' + ) % boot_bdm[0].get('snapshot_id') + raise webob.exc.HTTPNotFound( + explanation=explanation) + return image_snapshot + @wsgi.Controller.api_version(mv.SUPPORT_REIMAGE_VOLUME) @wsgi.response(HTTPStatus.ACCEPTED) @wsgi.action('os-reimage') @@ -341,9 +378,11 @@ class VolumeActionsController(wsgi.Controller): reimage_reserved = strutils.bool_from_string(reimage_reserved, strict=True) image_id = params['image_id'] + image_snap = self._get_image_snapshot_and_check_size( + context, image_id, volume.size) try: self.volume_api.reimage(context, volume, image_id, - reimage_reserved) + reimage_reserved, image_snap) except exception.InvalidVolume as error: raise webob.exc.HTTPBadRequest(explanation=error.msg) diff --git a/cinder/tests/unit/api/contrib/test_volume_actions.py b/cinder/tests/unit/api/contrib/test_volume_actions.py index acba239deed..fcec6d40e25 100644 --- a/cinder/tests/unit/api/contrib/test_volume_actions.py +++ b/cinder/tests/unit/api/contrib/test_volume_actions.py @@ -1592,8 +1592,11 @@ class VolumeImageActionsTest(test.TestCase): return req @ddt.data(None, False, True) + @mock.patch.object(volume_actions.VolumeActionsController, + '_get_image_snapshot_and_check_size') @mock.patch.object(volume_api.API, "reimage") - def test_volume_reimage(self, reimage_reserved, mock_image): + def test_volume_reimage( + self, reimage_reserved, mock_image, mock_get_img_snap): vol = utils.create_volume(self.context) body = {"os-reimage": {"image_id": fake.IMAGE_ID}} if reimage_reserved is not None: @@ -1619,7 +1622,9 @@ class VolumeImageActionsTest(test.TestCase): self.assertRaises(exception.VersionNotFoundForAPIMethod, self.controller._reimage, req, vol.id, body=body) - def test_reimage_volume_invalid_status(self): + @mock.patch.object(volume_actions.VolumeActionsController, + '_get_image_snapshot_and_check_size') + def test_reimage_volume_invalid_status(self, mock_get_img_snap): def fake_reimage_volume(*args, **kwargs): msg = "Volume status must be available." raise exception.InvalidVolume(reason=msg) @@ -1633,8 +1638,11 @@ class VolumeImageActionsTest(test.TestCase): self.controller._reimage, req, vol.id, body=body) + @mock.patch.object(volume_actions.VolumeActionsController, + '_get_image_snapshot_and_check_size') @mock.patch('cinder.context.RequestContext.authorize') - def test_reimage_volume_attach_more_than_one_server(self, mock_authorize): + def test_reimage_volume_attach_more_than_one_server(self, mock_authorize, + mock_get_img_snap): vol = utils.create_volume(self.context) va_objs = [objects.VolumeAttachment(context=self.context, id=i) for i in [fake.OBJECT_ID, fake.OBJECT2_ID, fake.OBJECT3_ID]] @@ -1646,3 +1654,77 @@ class VolumeImageActionsTest(test.TestCase): req = self._build_reimage_req(body, vol) self.assertRaises(webob.exc.HTTPConflict, self.controller._reimage, req, vol.id, body=body) + + @mock.patch.object(volume_api.API, 'get_snapshot') + @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj) + @mock.patch.object(glance, 'get_default_image_service') + @mock.patch.object(volume_api.API, "reimage") + def test_volume_reimage_image_snapshot( + self, mock_image, mock_image_service, mock_get_snap): + vol = utils.create_volume(self.context) + image_meta = { + 'properties': { + 'block_device_mapping': [ + { + 'source_type': 'snapshot', + 'boot_index': 0, + 'volume_size': 1, + } + ] + } + } + mock_image_service.return_value = mock.MagicMock() + mock_image_service.return_value.show.return_value = image_meta + body = {"os-reimage": {"image_id": fake.IMAGE_ID}} + req = self._build_reimage_req(body, vol.id) + self.controller._reimage(req, vol.id, body=body) + + @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj) + @mock.patch.object(glance, 'get_default_image_service') + @mock.patch.object(volume_api.API, "reimage") + def test_volume_reimage_image_snapshot_size_mismatch( + self, mock_image, mock_image_service): + vol = utils.create_volume(self.context) + image_meta = { + 'properties': { + 'block_device_mapping': [ + { + 'source_type': 'snapshot', + 'boot_index': 0, + 'volume_size': 2, + } + ] + } + } + mock_image_service.return_value = mock.MagicMock() + mock_image_service.return_value.show.return_value = image_meta + body = {"os-reimage": {"image_id": fake.IMAGE_ID}} + req = self._build_reimage_req(body, vol.id) + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._reimage, req, vol.id, body=body) + + @mock.patch.object(volume_api.API, 'get_snapshot') + @mock.patch.object(volume_api.API, 'get', fake_volume_get_obj) + @mock.patch.object(glance, 'get_default_image_service') + @mock.patch.object(volume_api.API, "reimage") + def test_volume_reimage_image_snapshot_snap_not_found( + self, mock_image, mock_image_service, mock_get_snap): + vol = utils.create_volume(self.context) + image_meta = { + 'properties': { + 'block_device_mapping': [ + { + 'source_type': 'snapshot', + 'boot_index': 0, + 'volume_size': 1, + } + ] + } + } + mock_image_service.return_value = mock.MagicMock() + mock_image_service.return_value.show.return_value = image_meta + mock_get_snap.side_effect = exception.NotFound + body = {"os-reimage": {"image_id": fake.IMAGE_ID}} + req = self._build_reimage_req(body, vol.id) + self.assertRaises(webob.exc.HTTPNotFound, + self.controller._reimage, req, vol.id, body=body) diff --git a/cinder/tests/unit/volume/test_rpcapi.py b/cinder/tests/unit/volume/test_rpcapi.py index 9840738cbb0..254250b9b42 100644 --- a/cinder/tests/unit/volume/test_rpcapi.py +++ b/cinder/tests/unit/volume/test_rpcapi.py @@ -677,11 +677,16 @@ class VolumeRPCAPITestCase(test.RPCAPITestCase): group=self.fake_group, version='3.14') - def test_reimage(self): + @ddt.data('3.18', '3.20') + def test_reimage(self, version): + if version == '3.18': + self.can_send_version_mock.side_effect = ( + True, True, False, False) self._test_rpc_api('reimage', rpc_method='cast', server=self.fake_volume_obj.host, volume=self.fake_volume_obj, image_meta={'id': fake.IMAGE_ID, 'container_format': 'fake_type', 'disk_format': 'fake_format'}, - version='3.18') + image_snap='fake_snap', + version=version) diff --git a/cinder/tests/unit/volume/test_volume_reimage.py b/cinder/tests/unit/volume/test_volume_reimage.py index ca19539b5f9..1a76d72876f 100644 --- a/cinder/tests/unit/volume/test_volume_reimage.py +++ b/cinder/tests/unit/volume/test_volume_reimage.py @@ -50,6 +50,26 @@ class VolumeReimageTestCase(base.BaseVolumeTestCase): disable_sparse=True) self.assertEqual(volume.status, 'available') + def test_volume_reimage_image_snapshot(self): + volume = tests_utils.create_volume(self.context, status='downloading', + previous_status='available') + self.assertEqual(volume.status, 'downloading') + self.assertEqual(volume.previous_status, 'available') + self.volume.create_volume(self.context, volume) + + with mock.patch.object(self.volume.driver, 'copy_image_to_volume' + ) as mock_cp_img, \ + mock.patch.object(self.volume, '_revert_to_snapshot_generic' + ) as generic_revert: + fake_snap = mock.MagicMock( + id='08f850d7-8b43-4656-a71c-647c864a3599') + self.volume.reimage( + self.context, volume, self.image_meta, image_snap=fake_snap) + mock_cp_img.assert_not_called() + generic_revert.assert_called_once_with( + self.context, volume, fake_snap) + self.assertEqual(volume.status, 'available') + def test_volume_reimage_raise_exception(self): volume = tests_utils.create_volume(self.context) self.volume.create_volume(self.context, volume) @@ -107,7 +127,7 @@ class VolumeReimageTestCase(base.BaseVolumeTestCase): self.volume_api.reimage(self.context, volume, self.image_meta['id']) mock_check.assert_called_once_with(self.image_meta, volume.size) mock_reimage.assert_called_once_with(self.context, volume, - self.image_meta) + self.image_meta, image_snap=None) @mock.patch('cinder.volume.volume_utils.check_image_metadata') @mock.patch('cinder.volume.rpcapi.VolumeAPI.reimage') @@ -139,7 +159,7 @@ class VolumeReimageTestCase(base.BaseVolumeTestCase): reimage_reserved=True) mock_check.assert_called_once_with(self.image_meta, volume.size) mock_reimage.assert_called_once_with(self.context, volume, - self.image_meta) + self.image_meta, image_snap=None) def test_volume_reimage_api_with_invaild_status(self): volume = tests_utils.create_volume(self.context) @@ -167,3 +187,16 @@ class VolumeReimageTestCase(base.BaseVolumeTestCase): self.assertIn("status must be " "available or error or reserved", str(ex)) + + @mock.patch('cinder.volume.volume_utils.check_image_metadata') + @mock.patch('cinder.volume.rpcapi.VolumeAPI.reimage') + def test_volume_reimage_api_image_snapshot( + self, mock_reimage, mock_check): + volume = tests_utils.create_volume(self.context) + self.volume_api.reimage( + self.context, volume, self.image_meta['id'], + image_snap='fake_snap') + mock_check.assert_called_once_with(self.image_meta, volume['size']) + mock_reimage.assert_called_once_with(self.context, volume, + self.image_meta, + image_snap='fake_snap') diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 0e20baff0f5..00adf73425c 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -2689,7 +2689,8 @@ class API(base.Base): volume_utils.notify_about_volume_usage(ctxt, volume, "detach.end") return volume.volume_attachment - def reimage(self, context, volume, image_id, reimage_reserved=False): + def reimage(self, context, volume, image_id, reimage_reserved=False, + image_snap=None): if volume.status in ['reserved']: context.authorize(vol_action_policy.REIMAGE_RESERVED_POLICY, target_obj=volume) @@ -2717,6 +2718,10 @@ class API(base.Base): raise exception.InvalidVolume(reason=msg) image_meta = self.image_service.show(context, image_id) try: + # If the source of the image is a volume snapshot + # (image_snap is not None), we will get image 'size' as 0 and + # 'virtual_size' as None but at least we will verify the image + # 'status' and 'min_disk' properties. volume_utils.check_image_metadata(image_meta, volume['size']) # Currently we only raise InvalidInput and ImageUnacceptable # exceptions in the check_image_metadata call but having Exception @@ -2725,16 +2730,18 @@ class API(base.Base): # Also this helps makes adding new exceptions easier in the future. except Exception: with excutils.save_and_reraise_exception(): - LOG.exception("Failed to reimage volume %(volume_id)s with " - "image %(image_id)s", - {'volume_id': volume.id, 'image_id': image_id}) + LOG.exception("Failed to reimage volume %(volume_id)s " + "with image %(image_id)s", + {'volume_id': volume.id, + 'image_id': image_id}) volume.conditional_update( {'status': volume.model.previous_status, 'previous_status': None}, {'status': 'downloading'}) self.volume_rpcapi.reimage(context, volume, - image_meta) + image_meta, + image_snap=image_snap) class HostAPI(base.Base): diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index d691710ddd1..d78129c004d 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -5350,19 +5350,29 @@ class VolumeManager(manager.CleanableManager, self.db.volume_glance_metadata_bulk_create(context, volume.id, volume_meta) - def reimage(self, context, volume, image_meta): + def reimage(self, context, volume, image_meta, image_snap=None): """Reimage a volume with specific image.""" image_id = None try: - image_id = image_meta['id'] - image_service, _ = glance.get_remote_image_service( - context, image_meta['id']) - image_location = image_service.get_location(context, image_id) + if image_snap: + # We are not calling the driver method here since the snapshot + # could belong to a different volume. + # Even if the snapshot belongs to a different volume, we are + # doing generic revert where we create a volume out of the + # snapshot and do a copy so we are safe here. + # Size checks are already done on the API layer so we don't + # need to worry about the image fitting into the volume. + self._revert_to_snapshot_generic(context, volume, image_snap) + else: + image_id = image_meta['id'] + image_service, _ = glance.get_remote_image_service( + context, image_meta['id']) + image_location = image_service.get_location(context, image_id) - volume_utils.copy_image_to_volume(self.driver, context, volume, - image_meta, image_location, - image_service, - disable_sparse=True) + volume_utils.copy_image_to_volume(self.driver, context, volume, + image_meta, image_location, + image_service, + disable_sparse=True) self._refresh_volume_glance_meta(context, volume, image_meta) volume.status = volume.previous_status diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index 58de69c1c6c..15c7e8552c5 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -139,9 +139,10 @@ class VolumeAPI(rpc.RPCAPI): 3.17 - Make get_backup_device a cast (async) 3.18 - Add reimage method 3.19 - Add extend_volume_completion method + 3.20 - Add image_snap parameter to reimage method """ - RPC_API_VERSION = '3.19' + RPC_API_VERSION = '3.20' RPC_DEFAULT_VERSION = '3.0' TOPIC = constants.VOLUME_TOPIC BINARY = constants.VOLUME_BINARY @@ -544,6 +545,11 @@ class VolumeAPI(rpc.RPCAPI): group=group) @rpc.assert_min_rpc_version('3.18') - def reimage(self, ctxt, volume, image_meta): - cctxt = self._get_cctxt(volume.service_topic_queue, version='3.18') - cctxt.cast(ctxt, 'reimage', volume=volume, image_meta=image_meta) + def reimage(self, ctxt, volume, image_meta, image_snap=None): + cctxt = self._get_cctxt( + volume.service_topic_queue, version=('3.20', '3.18')) + if cctxt.can_send_version('3.20'): + cctxt.cast(ctxt, 'reimage', volume=volume, image_meta=image_meta, + image_snap=image_snap) + else: + cctxt.cast(ctxt, 'reimage', volume=volume, image_meta=image_meta) diff --git a/releasenotes/notes/fix-reimage-image-snap-15ecd5fce9973d5d.yaml b/releasenotes/notes/fix-reimage-image-snap-15ecd5fce9973d5d.yaml new file mode 100644 index 00000000000..a34de0a343b --- /dev/null +++ b/releasenotes/notes/fix-reimage-image-snap-15ecd5fce9973d5d.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + `Bug #2062539 `_: Fixed + reimage operation when the image is backed by a volume snapshot.