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

Author: mriedem

nova/compute/api.py

@@ -570,8 +570,7 @@ def _validate_flavor_image_nostatus(context, image, instance_type,
         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 @@ def _validate_flavor_image_nostatus(context, image, instance_type,
                         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,19 +3626,23 @@ def resize(self, context, instance, flavor_id=None, clean_shutdown=True,
         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)
             new_instance_type = current_instance_type
         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 @@ def resize(self, context, instance, flavor_id=None, clean_shutdown=True,
         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': []}
 

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 @@ def test_volume_backed_resize_disk_down(self):
         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')

nova/tests/unit/compute/test_compute_api.py

@@ -1932,6 +1932,8 @@ def test_revert_resize_concurrent_fail(self, mock_reqspec, mock_elevated,
                 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 @@ def test_revert_resize_concurrent_fail(self, mock_reqspec, mock_elevated,
     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,