Merge "Improve existing flavor and image metadata validation"

nova/api/openstack/compute/servers.py

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

nova/exception.py

@@ -2279,6 +2279,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

@@ -4357,12 +4357,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)
@@ -4374,6 +4378,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

@@ -188,6 +188,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(
@@ -252,7 +256,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
@@ -287,8 +291,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,
@@ -301,9 +305,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,
@@ -337,9 +345,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,
@@ -562,7 +574,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})
 
@@ -1294,11 +1306,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:
@@ -1321,11 +1347,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:
@@ -1496,6 +1538,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