Improve existing flavor and image metadata validation

This improves the existing validation of flavor extra-specs and image
properties in hardware.py in preparation for calling the validation from
more places in a follow-on patch.

get_cpu_topology_constraints() becomes public as we'll need to call it
from the API code in the next patch in the series.

If the CPU topology is not valid, raise an InvalidRequest exception with
a useful error message instead of a ValueError message without any
context.

Add checks to ensure that the CPU and CPU thread policies are valid,
and if not then raise newly-added exceptions with useful error messages.

Tweak various docstrings to more accurately reflect what exceptions
they might raise.

Change-Id: I20854134c80b8f4598f375eae137fd2920114891
blueprint: flavor-extra-spec-image-property-validation
Signed-off-by: Chris Friesen <chris.friesen@windriver.com>

nova/api/openstack/compute/servers.py

@@ -641,6 +641,8 @@ class ServersController(wsgi.Controller):
                 exception.AutoDiskConfigDisabledByImage,
                 exception.ImageCPUPinningForbidden,
                 exception.ImageCPUThreadPolicyForbidden,
+                exception.InvalidCPUAllocationPolicy,
+                exception.InvalidCPUThreadAllocationPolicy,
                 exception.ImageNUMATopologyIncomplete,
                 exception.ImageNUMATopologyForbidden,
                 exception.ImageNUMATopologyAsymmetric,

nova/exception.py

@@ -2282,6 +2282,16 @@ class InvalidEmulatorThreadsPolicy(Invalid):
                 "given: '%(requested)s', available: '%(available)s'.")
 
 
+class InvalidCPUAllocationPolicy(Invalid):
+    msg_fmt = _("CPU policy requested from '%(source)s' is invalid, "
+                "given: '%(requested)s', available: '%(available)s'.")
+
+
+class InvalidCPUThreadAllocationPolicy(Invalid):
+    msg_fmt = _("CPU thread policy requested from '%(source)s' is invalid, "
+                "given: '%(requested)s', available: '%(available)s'.")
+
+
 class BadRequirementEmulatorThreadsPolicy(Invalid):
     msg_fmt = _("An isolated CPU emulator threads option requires a dedicated "
                 "CPU policy option.")

nova/tests/unit/api/openstack/compute/test_serversV21.py

@@ -4287,12 +4287,16 @@ class ServersControllerCreateTest(test.TestCase):
     @mock.patch('nova.virt.hardware.numa_get_constraints')
     def _test_create_instance_numa_topology_wrong(self, exc,
                                                   numa_constraints_mock):
-        numa_constraints_mock.side_effect = exc(**{'name': None,
+        numa_constraints_mock.side_effect = exc(**{
-                                                   'cpunum': 0,
+            'name': None,
-                                                   'cpumax': 0,
+            'source': 'flavor',
-                                                   'cpuset': None,
+            'requested': 'dummy',
-                                                   'memsize': 0,
+            'available': str(objects.fields.CPUAllocationPolicy.ALL),
-                                                   'memtotal': 0})
+            'cpunum': 0,
+            'cpumax': 0,
+            'cpuset': None,
+            'memsize': 0,
+            'memtotal': 0})
         self.req.body = jsonutils.dump_as_bytes(self.body)
         self.assertRaises(webob.exc.HTTPBadRequest,
                           self.controller.create, self.req, body=self.body)
@@ -4304,6 +4308,8 @@ class ServersControllerCreateTest(test.TestCase):
                     exception.ImageNUMATopologyCPUOutOfRange,
                     exception.ImageNUMATopologyCPUDuplicates,
                     exception.ImageNUMATopologyCPUsUnassigned,
+                    exception.InvalidCPUAllocationPolicy,
+                    exception.InvalidCPUThreadAllocationPolicy,
                     exception.ImageNUMATopologyMemoryOutOfRange]:
             self._test_create_instance_numa_topology_wrong(exc)
 

nova/tests/unit/virt/test_hardware.py

@@ -359,7 +359,7 @@ class VCPUTopologyTest(test.NoDBTestCase):
             image_meta = objects.ImageMeta.from_dict(topo_test["image"])
             if type(topo_test["expect"]) == tuple:
                 (preferred,
-                 maximum) = hw._get_cpu_topology_constraints(
+                 maximum) = hw.get_cpu_topology_constraints(
                      topo_test["flavor"], image_meta)
 
                 self.assertEqual(topo_test["expect"][0], preferred.sockets)
@@ -370,10 +370,32 @@ class VCPUTopologyTest(test.NoDBTestCase):
                 self.assertEqual(topo_test["expect"][5], maximum.threads)
             else:
                 self.assertRaises(topo_test["expect"],
-                                  hw._get_cpu_topology_constraints,
+                                  hw.get_cpu_topology_constraints,
                                   topo_test["flavor"],
                                   image_meta)
 
+    def test_invalid_flavor_values(self):
+        extra_specs = {
+                    # test non-integer value of these extra specs
+                    "hw:cpu_sockets": "8",
+                    "hw:cpu_cores": "2",
+                    "hw:cpu_threads": "1",
+                    "hw:cpu_max_sockets": "8",
+                    "hw:cpu_max_cores": "2",
+                    "hw:cpu_max_threads": "1",
+            }
+        image_meta = objects.ImageMeta.from_dict({"properties": {}})
+
+        for key in extra_specs:
+            extra_specs_invalid = extra_specs.copy()
+            extra_specs_invalid[key] = 'foo'
+            flavor = objects.Flavor(vcpus=16, memory_mb=2048,
+                                     extra_specs=extra_specs_invalid)
+            self.assertRaises(exception.InvalidRequest,
+                              hw.get_cpu_topology_constraints,
+                              flavor,
+                              image_meta)
+
     def test_possible_topologies(self):
         testdata = [
             {
@@ -1248,7 +1270,42 @@ class NUMATopologyTest(test.NoDBTestCase):
                             cpu_policy=fields.CPUAllocationPolicy.DEDICATED,
                         )]),
             },
-
+            {
+                # Invalid CPU pinning policy
+                "flavor": objects.Flavor(vcpus=4, memory_mb=2048,
+                                         extra_specs={
+                    "hw:cpu_policy": "foo",
+                }),
+                "image": {
+                    "properties": {}
+                },
+                "expect": exception.InvalidCPUAllocationPolicy,
+            },
+            {
+                # Invalid CPU thread pinning
+                "flavor": objects.Flavor(vcpus=4, memory_mb=2048,
+                                         extra_specs={
+                    "hw:cpu_policy": fields.CPUAllocationPolicy.DEDICATED,
+                    "hw:cpu_thread_policy": "foo",
+                }),
+                "image": {
+                    "properties": {}
+                },
+                "expect": exception.InvalidCPUThreadAllocationPolicy,
+            },
+            {
+                # Invalid CPU thread pinning in image
+                "flavor": objects.Flavor(vcpus=4, memory_mb=2048,
+                                         extra_specs={
+                    "hw:cpu_policy": fields.CPUAllocationPolicy.SHARED,
+                 }),
+                "image": {
+                    "properties": {
+                        "hw_cpu_policy": fields.CPUAllocationPolicy.DEDICATED,
+                    }
+                },
+                "expect": exception.ImageCPUPinningForbidden,
+            },
         ]
 
         for testitem in testdata:

nova/virt/hardware.py

@@ -187,6 +187,10 @@ def get_number_of_serial_ports(flavor, image_meta):
     :param flavor: Flavor object to read extra specs from
     :param image_meta: nova.objects.ImageMeta object instance
 
+    :raises: exception.ImageSerialPortNumberInvalid if the serial port count
+             is not a valid integer
+    :raises: exception.ImageSerialPortNumberExceedFlavorValue if the serial
+             port count defined in image is greater than that of flavor
     :returns: number of serial ports
     """
     flavor_num_ports, image_num_ports = _get_flavor_image_meta(
@@ -251,7 +255,7 @@ def _score_cpu_topology(topology, wanttopology):
     return score
 
 
-def _get_cpu_topology_constraints(flavor, image_meta):
+def get_cpu_topology_constraints(flavor, image_meta):
     """Get the topology constraints declared in flavor or image
 
     Extracts the topology constraints from the configuration defined in
@@ -286,8 +290,8 @@ def _get_cpu_topology_constraints(flavor, image_meta):
     :raises: exception.ImageVCPUTopologyRangeExceeded if the preferred
              counts set against the image exceed the maximum counts set
              against the image or flavor
-    :raises: ValueError if one of the provided flavor properties is a
+    :raises: exception.InvalidRequest if one of the provided flavor properties
-             non-integer
+             is a non-integer
     :returns: A two-tuple of objects.VirtCPUTopology instances. The
               first element corresponds to the preferred topology,
               while the latter corresponds to the maximum topology,
@@ -300,9 +304,13 @@ def _get_cpu_topology_constraints(flavor, image_meta):
     flavor_max_threads, image_max_threads = _get_flavor_image_meta(
         'cpu_max_threads', flavor, image_meta, 0)
     # image metadata is already of the correct type
-    flavor_max_sockets = int(flavor_max_sockets)
+    try:
-    flavor_max_cores = int(flavor_max_cores)
+        flavor_max_sockets = int(flavor_max_sockets)
-    flavor_max_threads = int(flavor_max_threads)
+        flavor_max_cores = int(flavor_max_cores)
+        flavor_max_threads = int(flavor_max_threads)
+    except ValueError as e:
+        msg = _('Invalid flavor extra spec. Error: %s') % six.text_type(e)
+        raise exception.InvalidRequest(msg)
 
     LOG.debug("Flavor limits %(sockets)d:%(cores)d:%(threads)d",
               {"sockets": flavor_max_sockets,
@@ -336,9 +344,13 @@ def _get_cpu_topology_constraints(flavor, image_meta):
         'cpu_cores', flavor, image_meta, 0)
     flavor_threads, image_threads = _get_flavor_image_meta(
         'cpu_threads', flavor, image_meta, 0)
-    flavor_sockets = int(flavor_sockets)
+    try:
-    flavor_cores = int(flavor_cores)
+        flavor_sockets = int(flavor_sockets)
-    flavor_threads = int(flavor_threads)
+        flavor_cores = int(flavor_cores)
+        flavor_threads = int(flavor_threads)
+    except ValueError as e:
+        msg = _('Invalid flavor extra spec. Error: %s') % six.text_type(e)
+        raise exception.InvalidRequest(msg)
 
     LOG.debug("Flavor pref %(sockets)d:%(cores)d:%(threads)d",
               {"sockets": flavor_sockets,
@@ -561,7 +573,7 @@ def _get_desirable_cpu_topologies(flavor, image_meta, allow_threads=True,
               {"flavor": flavor, "image_meta": image_meta,
                "threads": allow_threads})
 
-    preferred, maximum = _get_cpu_topology_constraints(flavor, image_meta)
+    preferred, maximum = get_cpu_topology_constraints(flavor, image_meta)
     LOG.debug("Topology preferred %(preferred)s, maximum %(maximum)s",
               {"preferred": preferred, "maximum": maximum})
 
@@ -1289,11 +1301,25 @@ def _get_cpu_policy_constraint(flavor, image_meta):
     :param image_meta: ``nova.objects.ImageMeta`` instance
     :raises: exception.ImageCPUPinningForbidden if policy is defined on both
         image and flavor and these policies conflict.
+    :raises: exception.InvalidCPUAllocationPolicy if policy is defined with
+        invalid value in image or flavor.
     :returns: The CPU policy requested.
     """
     flavor_policy, image_policy = _get_flavor_image_meta(
         'cpu_policy', flavor, image_meta)
 
+    if flavor_policy and (flavor_policy not in fields.CPUAllocationPolicy.ALL):
+        raise exception.InvalidCPUAllocationPolicy(
+            source='flavor extra specs',
+            requested=flavor_policy,
+            available=str(fields.CPUAllocationPolicy.ALL))
+
+    if image_policy and (image_policy not in fields.CPUAllocationPolicy.ALL):
+        raise exception.InvalidCPUAllocationPolicy(
+            source='image properties',
+            requested=image_policy,
+            available=str(fields.CPUAllocationPolicy.ALL))
+
     if flavor_policy == fields.CPUAllocationPolicy.DEDICATED:
         cpu_policy = flavor_policy
     elif flavor_policy == fields.CPUAllocationPolicy.SHARED:
@@ -1316,11 +1342,27 @@ def _get_cpu_thread_policy_constraint(flavor, image_meta):
     :param image_meta: ``nova.objects.ImageMeta`` instance
     :raises: exception.ImageCPUThreadPolicyForbidden if policy is defined on
         both image and flavor and these policies conflict.
+    :raises: exception.InvalidCPUThreadAllocationPolicy if policy is defined
+        with invalid value in image or flavor.
     :returns: The CPU thread policy requested.
     """
     flavor_policy, image_policy = _get_flavor_image_meta(
         'cpu_thread_policy', flavor, image_meta)
 
+    if flavor_policy and (
+            flavor_policy not in fields.CPUThreadAllocationPolicy.ALL):
+        raise exception.InvalidCPUThreadAllocationPolicy(
+            source='flavor extra specs',
+            requested=flavor_policy,
+            available=str(fields.CPUThreadAllocationPolicy.ALL))
+
+    if image_policy and (
+            image_policy not in fields.CPUThreadAllocationPolicy.ALL):
+        raise exception.InvalidCPUThreadAllocationPolicy(
+            source='image properties',
+            requested=image_policy,
+            available=str(fields.CPUThreadAllocationPolicy.ALL))
+
     if flavor_policy in [None, fields.CPUThreadAllocationPolicy.PREFER]:
         policy = flavor_policy or image_policy
     elif image_policy and image_policy != flavor_policy:
@@ -1491,6 +1533,12 @@ def numa_get_constraints(flavor, image_meta):
              policy conflicts with CPU allocation policy
     :raises: exception.ImageCPUThreadPolicyForbidden if a CPU thread policy
              specified in a flavor conflicts with one defined in image metadata
+    :raises: exception.BadRequirementEmulatorThreadsPolicy if CPU emulator
+             threads policy conflicts with CPU allocation policy
+    :raises: exception.InvalidCPUAllocationPolicy if policy is defined with
+             invalid value in image or flavor.
+    :raises: exception.InvalidCPUThreadAllocationPolicy if policy is defined
+             with invalid value in image or flavor.
     :returns: objects.InstanceNUMATopology, or None
     """
     numa_topology = None