From 581b2694a550e9773d596cc9da3c8ecb9bd0c1d8 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Tue, 28 Jan 2020 20:33:48 +0000 Subject: [PATCH] compute: Extract _get_bdm_image_metadata into nova.utils This function extracts image metadata from a given block_device_mapping and is extremely useful outside of the compute API. This change simply extracts the function into nova.utils alongside get_image_metadata_from_volume and other similar functions. Change-Id: I8ba52f0cd877cefc1f7d3c10d8a07a2a1c21cb34 --- nova/compute/api.py | 52 +---------------- .../api/openstack/compute/test_serversV21.py | 16 +++--- nova/tests/unit/compute/test_compute.py | 17 ++++-- nova/tests/unit/compute/test_compute_api.py | 25 ++++++--- nova/utils.py | 56 +++++++++++++++++++ 5 files changed, 94 insertions(+), 72 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 1064a77b69e8..89ff8f468fac 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -1382,53 +1382,6 @@ class API(base.Base): return certs_to_return - def _get_bdm_image_metadata(self, context, block_device_mapping, - legacy_bdm=True): - """If we are booting from a volume, we need to get the - volume details from Cinder and make sure we pass the - metadata back accordingly. - """ - if not block_device_mapping: - return {} - - for bdm in block_device_mapping: - if (legacy_bdm and - block_device.get_device_letter( - bdm.get('device_name', '')) != 'a'): - continue - elif not legacy_bdm and bdm.get('boot_index') != 0: - continue - - volume_id = bdm.get('volume_id') - snapshot_id = bdm.get('snapshot_id') - if snapshot_id: - # NOTE(alaski): A volume snapshot inherits metadata from the - # originating volume, but the API does not expose metadata - # on the snapshot itself. So we query the volume for it below. - snapshot = self.volume_api.get_snapshot(context, snapshot_id) - volume_id = snapshot['volume_id'] - - if bdm.get('image_id'): - try: - image_id = bdm['image_id'] - image_meta = self.image_api.get(context, image_id) - return image_meta - except Exception: - raise exception.InvalidBDMImage(id=image_id) - elif volume_id: - try: - volume = self.volume_api.get(context, volume_id) - except exception.CinderConnectionFailed: - raise - except Exception: - raise exception.InvalidBDMVolume(id=volume_id) - - if not volume.get('bootable', True): - raise exception.InvalidBDMVolumeNotBootable(id=volume_id) - - return utils.get_image_metadata_from_volume(volume) - return {} - @staticmethod def _get_requested_instance_group(context, filter_properties): if (not filter_properties or @@ -1481,8 +1434,9 @@ class API(base.Base): "when booting from volume") raise exception.CertificateValidationFailed(message=msg) image_id = None - boot_meta = self._get_bdm_image_metadata( - context, block_device_mapping, legacy_bdm) + boot_meta = utils.get_bdm_image_metadata( + context, self.image_api, self.volume_api, block_device_mapping, + legacy_bdm) self._check_auto_disk_config(image=boot_meta, auto_disk_config=auto_disk_config) diff --git a/nova/tests/unit/api/openstack/compute/test_serversV21.py b/nova/tests/unit/api/openstack/compute/test_serversV21.py index 4efc815d01c5..5eef44e55076 100644 --- a/nova/tests/unit/api/openstack/compute/test_serversV21.py +++ b/nova/tests/unit/api/openstack/compute/test_serversV21.py @@ -4685,7 +4685,7 @@ class ServersControllerCreateTest(test.TestCase): @mock.patch('nova.compute.api.API._get_volumes_for_bdms') @mock.patch.object(compute_api.API, '_validate_bdm') - @mock.patch.object(compute_api.API, '_get_bdm_image_metadata') + @mock.patch('nova.utils.get_bdm_image_metadata') def test_create_instance_with_bdms_and_no_image( self, mock_bdm_image_metadata, mock_validate_bdm, mock_get_vols): mock_bdm_image_metadata.return_value = {} @@ -4706,11 +4706,11 @@ class ServersControllerCreateTest(test.TestCase): mock_validate_bdm.assert_called_once() mock_bdm_image_metadata.assert_called_once_with( - mock.ANY, mock.ANY, False) + mock.ANY, mock.ANY, mock.ANY, mock.ANY, False) @mock.patch('nova.compute.api.API._get_volumes_for_bdms') @mock.patch.object(compute_api.API, '_validate_bdm') - @mock.patch.object(compute_api.API, '_get_bdm_image_metadata') + @mock.patch('nova.utils.get_bdm_image_metadata') def test_create_instance_with_bdms_and_empty_imageRef( self, mock_bdm_image_metadata, mock_validate_bdm, mock_get_volumes): mock_bdm_image_metadata.return_value = {} @@ -4971,7 +4971,7 @@ class ServersControllerCreateTest(test.TestCase): self.assertRaises(webob.exc.HTTPBadRequest, self._test_create, params, no_image=True) - @mock.patch('nova.compute.api.API._get_bdm_image_metadata') + @mock.patch('nova.utils.get_bdm_image_metadata') def test_create_instance_non_bootable_volume_fails(self, fake_bdm_meta): params = {'block_device_mapping_v2': self.bdm_v2} fake_bdm_meta.side_effect = exception.InvalidBDMVolumeNotBootable(id=1) @@ -5042,7 +5042,7 @@ class ServersControllerCreateTest(test.TestCase): self.stub_out('nova.compute.api.API.create', create) self._test_create_bdm(params) - @mock.patch.object(compute_api.API, '_get_bdm_image_metadata') + @mock.patch('nova.utils.get_bdm_image_metadata') def test_create_instance_with_volumes_enabled_and_bdms_no_image( self, mock_get_bdm_image_metadata): """Test that the create works if there is no image supplied but @@ -5066,9 +5066,9 @@ class ServersControllerCreateTest(test.TestCase): self.stub_out('nova.compute.api.API.create', create) self._test_create_bdm(params, no_image=True) mock_get_bdm_image_metadata.assert_called_once_with( - mock.ANY, self.bdm, True) + mock.ANY, mock.ANY, mock.ANY, self.bdm, True) - @mock.patch.object(compute_api.API, '_get_bdm_image_metadata') + @mock.patch('nova.utils.get_bdm_image_metadata') def test_create_instance_with_imageRef_as_empty_string( self, mock_bdm_image_metadata): volume = { @@ -5111,7 +5111,7 @@ class ServersControllerCreateTest(test.TestCase): self.assertRaises(exception.ValidationError, self._test_create_bdm, params) - @mock.patch('nova.compute.api.API._get_bdm_image_metadata') + @mock.patch('nova.utils.get_bdm_image_metadata') def test_create_instance_non_bootable_volume_fails_legacy_bdm( self, fake_bdm_meta): bdm = [{ diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 8ac5459f5f6e..eb63b72e9082 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -681,8 +681,9 @@ class ComputeVolumeTestCase(BaseTestCase): 'delete_on_termination': False, }] - image_meta = self.compute_api._get_bdm_image_metadata( - self.context, block_device_mapping) + image_meta = utils.get_bdm_image_metadata( + self.context, self.compute_api.image_api, + self.compute_api.volume_api, block_device_mapping) if metadata: self.assertEqual(image_meta['properties']['vol_test_key'], 'vol_test_value') @@ -701,8 +702,10 @@ class ComputeVolumeTestCase(BaseTestCase): 'delete_on_termination': False, }] - image_meta = self.compute_api._get_bdm_image_metadata( - self.context, block_device_mapping, legacy_bdm=False) + image_meta = utils.get_bdm_image_metadata( + self.context, self.compute_api.image_api, + self.compute_api.volume_api, block_device_mapping, + legacy_bdm=False) if metadata: self.assertEqual(image_meta['properties']['vol_test_key'], 'vol_test_value') @@ -734,8 +737,10 @@ class ComputeVolumeTestCase(BaseTestCase): 'delete_on_termination': True, }] - image_meta = self.compute_api._get_bdm_image_metadata( - self.context, block_device_mapping, legacy_bdm=False) + image_meta = utils.get_bdm_image_metadata( + self.context, self.compute_api.image_api, + self.compute_api.volume_api, block_device_mapping, + legacy_bdm=False) if metadata: self.assertEqual('img_test_value', diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index c0e99d4d3577..c3716e08ed8f 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -3516,11 +3516,14 @@ class _ComputeAPIUnitTestMixIn(object): side_effect=get_vol_data): if not is_bootable: self.assertRaises(exception.InvalidBDMVolumeNotBootable, - self.compute_api._get_bdm_image_metadata, - self.context, block_device_mapping) + utils.get_bdm_image_metadata, + self.context, self.compute_api.image_api, + self.compute_api.volume_api, + block_device_mapping) else: - meta = self.compute_api._get_bdm_image_metadata(self.context, - block_device_mapping) + meta = utils.get_bdm_image_metadata( + self.context, self.compute_api.image_api, + self.compute_api.volume_api, block_device_mapping) self.assertEqual(expected_meta, meta) def test_boot_volume_non_bootable(self): @@ -3543,8 +3546,9 @@ class _ComputeAPIUnitTestMixIn(object): {"min_ram": 256, "min_disk": 128, "foo": "bar"}} with mock.patch.object(self.compute_api.volume_api, 'get', return_value=fake_volume): - meta = self.compute_api._get_bdm_image_metadata( - self.context, block_device_mapping) + meta = utils.get_bdm_image_metadata( + self.context, self.compute_api.image_api, + self.compute_api.volume_api, block_device_mapping) self.assertEqual(256, meta['min_ram']) self.assertEqual(128, meta['min_disk']) self.assertEqual('active', meta['status']) @@ -3569,8 +3573,9 @@ class _ComputeAPIUnitTestMixIn(object): mock.patch.object(self.compute_api.volume_api, 'get_snapshot', return_value=fake_snapshot)) as ( volume_get, volume_get_snapshot): - meta = self.compute_api._get_bdm_image_metadata( - self.context, block_device_mapping) + meta = utils.get_bdm_image_metadata( + self.context, self.compute_api.image_api, + self.compute_api.volume_api, block_device_mapping) self.assertEqual(256, meta['min_ram']) self.assertEqual(128, meta['min_disk']) self.assertEqual('active', meta['status']) @@ -4443,8 +4448,10 @@ class _ComputeAPIUnitTestMixIn(object): 'device_name': 'vda', }))] self.assertRaises(exception.CinderConnectionFailed, - self.compute_api._get_bdm_image_metadata, + utils.get_bdm_image_metadata, self.context, + self.compute_api.image_api, + self.compute_api.volume_api, bdms, legacy_bdm=True) def test_get_volumes_for_bdms_errors(self): diff --git a/nova/utils.py b/nova/utils.py index 50222092d202..87217abfa16f 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -50,6 +50,7 @@ from oslo_utils import units import six from six.moves import range +from nova import block_device import nova.conf from nova import exception from nova.i18n import _, _LE, _LW @@ -782,6 +783,61 @@ def get_image_from_system_metadata(system_meta): return image_meta +def get_bdm_image_metadata(context, image_api, volume_api, + block_device_mapping, legacy_bdm=True): + """Attempt to retrive image metadata from a given block_device_mapping. + + If we are booting from a volume, we need to get the volume details from + Cinder and make sure we pass the metadata back accordingly. + + :param context: request context + :param image_api: Image API + :param volume_api: Volume API + :param block_device_mapping: + :param legacy_bdm: + """ + if not block_device_mapping: + return {} + + for bdm in block_device_mapping: + if (legacy_bdm and + block_device.get_device_letter( + bdm.get('device_name', '')) != 'a'): + continue + elif not legacy_bdm and bdm.get('boot_index') != 0: + continue + + volume_id = bdm.get('volume_id') + snapshot_id = bdm.get('snapshot_id') + if snapshot_id: + # NOTE(alaski): A volume snapshot inherits metadata from the + # originating volume, but the API does not expose metadata + # on the snapshot itself. So we query the volume for it below. + snapshot = volume_api.get_snapshot(context, snapshot_id) + volume_id = snapshot['volume_id'] + + if bdm.get('image_id'): + try: + image_id = bdm['image_id'] + image_meta = image_api.get(context, image_id) + return image_meta + except Exception: + raise exception.InvalidBDMImage(id=image_id) + elif volume_id: + try: + volume = volume_api.get(context, volume_id) + except exception.CinderConnectionFailed: + raise + except Exception: + raise exception.InvalidBDMVolume(id=volume_id) + + if not volume.get('bootable', True): + raise exception.InvalidBDMVolumeNotBootable(id=volume_id) + + return get_image_metadata_from_volume(volume) + return {} + + def get_image_metadata_from_volume(volume): properties = copy.copy(volume.get('volume_image_metadata', {})) image_meta = {'properties': properties}