From 8aae3e39e58a05ed50b891aeae261641f544ad1f Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Tue, 16 Apr 2019 23:53:03 -0400 Subject: [PATCH] Fix volume-backed resize with a smaller disk flavor Change I06fad233006c7bab14749a51ffa226c3801f951b in Stein started calling _validate_flavor_image_nostatus during resize but did not pass the root_bdm since the underlying volume does not change. This can, however, lead to FlavorDiskSmallerThanMinDisk being raised if the new flavor has a disk size smaller than the current flavor. This is wrong in the case of a volume-backed server because we don't care about the root disk size in the flavor in that case. This fixes the bug by splitting the pci/numa validation logic out of _validate_flavor_image_nostatus into a separate method so that the resize method can call that directly for a volume-backed server rather than deal with the complicated disk size logic in _validate_flavor_image_nostatus (see bug 1646740 for details, but tl;dr if the image min_disk is less than the flavor root_gb, the min_disk stored in the instance image system metadata is the flavor['root_gb'] which could be larger than the root volume size). To see why trying to use _validate_flavor_image_nostatus during resize for a volume-backed server is a mess, look at PS1 of this change. Change-Id: I91c9c1e88afa035c757f692c78ad72d69cc3c431 Closes-Bug: #1825020 --- nova/compute/api.py | 62 +++++++++++++++---- .../regressions/test_bug_1825020.py | 16 +++-- nova/tests/unit/compute/test_compute_api.py | 4 +- 3 files changed, 61 insertions(+), 21 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 86edde9830dd..00e3b31d1bd1 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -570,8 +570,7 @@ class API(base.Base): with each other. :param context: A context.RequestContext - :param image: a dict representation of the image including properties, - enforces the image status is active. + :param image: a dict representation of the image including properties :param instance_type: Flavor object :param root_bdm: BlockDeviceMapping for root disk. Will be None for the resize case. @@ -664,6 +663,30 @@ class API(base.Base): servers_policies.ZERO_DISK_FLAVOR, fatal=False): raise exception.BootFromVolumeRequiredForZeroDiskFlavor() + API._validate_flavor_image_numa_pci( + image, instance_type, validate_numa=validate_numa, + validate_pci=validate_pci) + + @staticmethod + def _validate_flavor_image_numa_pci(image, instance_type, + validate_numa=True, + validate_pci=False): + """Validate the flavor and image NUMA/PCI values. + + This is called from the API service to ensure that the flavor + extra-specs and image properties are self-consistent and compatible + with each other. + + :param image: a dict representation of the image including properties + :param instance_type: Flavor object + :param validate_numa: Flag to indicate whether or not to validate + the NUMA-related metadata. + :param validate_pci: Flag to indicate whether or not to validate + the PCI-related metadata. + :raises: Many different possible exceptions. See + api.openstack.compute.servers.INVALID_FLAVOR_IMAGE_EXCEPTIONS + for the full list. + """ image_meta = _get_image_meta_obj(image) # Only validate values of flavor/image so the return results of @@ -3603,6 +3626,7 @@ class API(base.Base): current_instance_type = instance.get_flavor() # If flavor_id is not provided, only migrate the instance. + volume_backed = None if not flavor_id: LOG.debug("flavor_id is None. Assuming migration.", instance=instance) @@ -3610,12 +3634,15 @@ class API(base.Base): else: new_instance_type = flavors.get_flavor_by_flavor_id( flavor_id, read_deleted="no") + # Check to see if we're resizing to a zero-disk flavor which is + # only supported with volume-backed servers. if (new_instance_type.get('root_gb') == 0 and - current_instance_type.get('root_gb') != 0 and - not compute_utils.is_volume_backed_instance(context, - instance)): - reason = _('Resize to zero disk flavor is not allowed.') - raise exception.CannotResizeDisk(reason=reason) + current_instance_type.get('root_gb') != 0): + volume_backed = compute_utils.is_volume_backed_instance( + context, instance) + if not volume_backed: + reason = _('Resize to zero disk flavor is not allowed.') + raise exception.CannotResizeDisk(reason=reason) if not new_instance_type: raise exception.FlavorNotFound(flavor_id=flavor_id) @@ -3648,10 +3675,23 @@ class API(base.Base): if not same_instance_type: image = utils.get_image_from_system_metadata( instance.system_metadata) - # Can skip root_bdm check since it will not change during resize. - self._validate_flavor_image_nostatus( - context, image, new_instance_type, root_bdm=None, - validate_pci=True) + # Figure out if the instance is volume-backed but only if we didn't + # already figure that out above (avoid the extra db hit). + if volume_backed is None: + volume_backed = compute_utils.is_volume_backed_instance( + context, instance) + # If the server is volume-backed, we still want to validate numa + # and pci information in the new flavor, but we don't call + # _validate_flavor_image_nostatus because how it handles checking + # disk size validation was not intended for a volume-backed + # resize case. + if volume_backed: + self._validate_flavor_image_numa_pci( + image, new_instance_type, validate_pci=True) + else: + self._validate_flavor_image_nostatus( + context, image, new_instance_type, root_bdm=None, + validate_pci=True) filter_properties = {'ignore_hosts': []} diff --git a/nova/tests/functional/regressions/test_bug_1825020.py b/nova/tests/functional/regressions/test_bug_1825020.py index 9811eb8e095a..eb427f1b3749 100644 --- a/nova/tests/functional/regressions/test_bug_1825020.py +++ b/nova/tests/functional/regressions/test_bug_1825020.py @@ -10,11 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. -import six - from nova import test from nova.tests import fixtures as nova_fixtures -from nova.tests.functional.api import client as api_client from nova.tests.functional import fixtures as func_fixtures from nova.tests.functional import integrated_helpers from nova.tests.unit.image import fake as fake_image @@ -73,10 +70,11 @@ class VolumeBackedResizeDiskDown(test.TestCase, self._wait_for_state_change(self.api, server, 'ACTIVE') # Now try to resize the server with the flavor that has smaller disk. + # This should be allowed since the server is volume-backed and the + # disk size in the flavor shouldn't matter. data = {'resize': {'flavorRef': flavor1['id']}} - # FIXME(mriedem): This will raise FlavorDiskSmallerThanMinDisk as a 500 - # error until bug 1825020 is fixed. - ex = self.assertRaises(api_client.OpenStackApiException, - self.api.post_server_action, server['id'], data) - self.assertEqual(500, ex.response.status_code) - self.assertIn('FlavorDiskSmallerThanMinDisk', six.text_type(ex)) + self.api.post_server_action(server['id'], data) + self._wait_for_state_change(self.api, server, 'VERIFY_RESIZE') + # Now confirm the resize just to complete the operation. + self.api.post_server_action(server['id'], {'confirmResize': None}) + self._wait_for_state_change(self.api, server, 'ACTIVE') diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index 2cbfd20125c8..c8b1efc4b133 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -1932,6 +1932,8 @@ class _ComputeAPIUnitTestMixIn(object): self.context, fake_inst['uuid'], 'finished') mock_inst_save.assert_called_once_with(expected_task_state=[None]) + @mock.patch('nova.compute.utils.is_volume_backed_instance', + return_value=False) @mock.patch('nova.compute.api.API._validate_flavor_image_nostatus') @mock.patch('nova.objects.Migration') @mock.patch.object(compute_api.API, '_record_action_start') @@ -1945,7 +1947,7 @@ class _ComputeAPIUnitTestMixIn(object): def _test_resize(self, mock_get_all_by_host, mock_get_by_instance_uuid, mock_get_flavor, mock_upsize, mock_inst_save, mock_count, mock_limit, mock_record, - mock_migration, mock_validate, + mock_migration, mock_validate, mock_is_vol_backed, flavor_id_passed=True, same_host=False, allow_same_host=False, project_id=None,