Merge "Defaults missing group_policy to 'none'"

Zuul · openstack-gerrit · commit cea01e5f4bcb · 2019-07-16T17:37:05.000Z
diff --git a/doc/source/user/flavors.rst b/doc/source/user/flavors.rst
@@ -799,6 +799,16 @@ Numbered groupings of resource classes and traits
     * ``none``: Different numbered request groups may be satisfied
       by different providers *or* common providers.
 
+    .. note::
+
+        If more than one group is specified then the ``group_policy`` is
+        mandatory in the request. However such groups might come from other
+        sources than flavor extra_spec (e.g. from Neutron ports with QoS
+        minimum bandwidth policy). If the flavor does not specify any groups
+        and ``group_policy`` but more than one group is coming from other
+        sources then nova will default the ``group_policy`` to ``none`` to
+        avoid scheduler failure.
+
     For example, to create a server with the following VFs:
 
     * One SR-IOV virtual function (VF) on NET1 with bandwidth 10000 bytes/sec
diff --git a/nova/scheduler/utils.py b/nova/scheduler/utils.py
@@ -66,6 +66,14 @@ def __str__(self):
         return ', '.join(sorted(
             list(str(rg) for rg in list(self._rg_by_id.values()))))
 
+    @property
+    def group_policy(self):
+        return self._group_policy
+
+    @group_policy.setter
+    def group_policy(self, value):
+        self._group_policy = value
+
     def get_request_group(self, ident):
         if ident not in self._rg_by_id:
             rq_grp = objects.RequestGroup(use_same_provider=bool(ident))
@@ -211,6 +219,10 @@ def resource_groups(self):
         for rg in self._rg_by_id.values():
             yield rg.resources
 
+    def get_num_of_numbered_groups(self):
+        return len([ident for ident in self._rg_by_id.keys()
+                    if ident is not None])
+
     def merged_resources(self, flavor_resources=None):
         """Returns a merge of {resource_class: amount} for all resource groups.
 
@@ -300,31 +312,14 @@ def to_queryparams(request_group, suffix):
             qparams = []
         if self._group_policy is not None:
             qparams.append(('group_policy', self._group_policy))
-        nr_of_numbered_groups = 0
+
         for ident, rg in self._rg_by_id.items():
             # [('resourcesN', 'rclass:amount,rclass:amount,...'),
             #  ('requiredN', 'trait_name,!trait_name,...'),
             #  ('member_ofN', 'in:uuid,uuid,...'),
             #  ('member_ofN', 'in:uuid,uuid,...')]
             qparams.extend(to_queryparams(rg, ident or ''))
-            if ident:
-                nr_of_numbered_groups += 1
-        if nr_of_numbered_groups >= 2 and not self._group_policy:
-            # we know this will fail in placement so help the troubleshooting
-            LOG.warning(
-                "There is more than one numbered request group in the "
-                "allocation candidate query but the flavor did not specify "
-                "any group policy. This query will fail in placement due to "
-                "the missing group policy. If you specified more than one "
-                "numbered request group in the flavor extra_spec or booted "
-                "with more than one neutron port that has resource request "
-                "(i.e. the port has a QoS minimum bandwidth policy rule "
-                "attached) then you have to specify the group policy in the "
-                "flavor extra_spec. If it is OK to let these groups be "
-                "satisfied by overlapping resource providers then use "
-                "'group_policy': 'None'. If you want each group to be "
-                "satisfied from a separate resource provider then use "
-                "'group_policy': 'isolate'.")
+
         return parse.urlencode(sorted(qparams))
 
 
@@ -468,9 +463,12 @@ def resources_from_request_spec(ctxt, spec_obj, host_manager):
                           if rclass not in res_req.merged_resources()}
         # Now we don't need (or want) any remaining zero entries - remove them.
         res_req.strip_zeros()
+
+        numbered_groups_from_flavor = res_req.get_num_of_numbered_groups()
     else:
         # Start with an empty one
         res_req = ResourceRequest()
+        numbered_groups_from_flavor = 0
 
     # Process any image properties
     if 'image' in spec_obj and 'properties' in spec_obj.image:
@@ -556,6 +554,28 @@ def resources_from_request_spec(ctxt, spec_obj, host_manager):
         for key in spec_obj.scheduler_hints)):
         res_req._limit = None
 
+    if res_req.get_num_of_numbered_groups() >= 2 and not res_req.group_policy:
+        LOG.warning(
+            "There is more than one numbered request group in the "
+            "allocation candidate query but the flavor did not specify "
+            "any group policy. This query would fail in placement due to "
+            "the missing group policy. If you specified more than one "
+            "numbered request group in the flavor extra_spec then you need to "
+            "specify the group policy in the flavor extra_spec. If it is OK "
+            "to let these groups be satisfied by overlapping resource "
+            "providers then use 'group_policy': 'none'. If you want each "
+            "group to be satisfied from a separate resource provider then "
+            "use 'group_policy': 'isolate'.")
+
+        if numbered_groups_from_flavor <= 1:
+            LOG.info(
+                "At least one numbered request group is defined outside of "
+                "the flavor (e.g. in a port that has a QoS minimum bandwidth "
+                "policy rule attached) but the flavor did not specify any "
+                "group policy. To avoid the placement failure nova defaults "
+                "the group policy to 'none'.")
+            res_req.group_policy = 'none'
+
     return res_req
 
 
diff --git a/nova/tests/unit/scheduler/test_utils.py b/nova/tests/unit/scheduler/test_utils.py
@@ -362,7 +362,8 @@ def test_resources_from_request_spec_no_aggregates(self):
                 self.context, reqspec, self.mock_host_manager)
         self.assertEqual([], req.get_request_group(None).aggregates)
 
-    @mock.patch("nova.scheduler.utils.ResourceRequest.from_extra_specs")
+    @mock.patch("nova.scheduler.utils.ResourceRequest.from_extra_specs",
+                return_value=utils.ResourceRequest())
     def test_process_extra_specs_granular_called(self, mock_proc):
         flavor = objects.Flavor(vcpus=1,
                                 memory_mb=1024,
@@ -794,20 +795,6 @@ def test_resource_request_from_extra_specs(self):
         )
         self.assertEqual(expected_querystring, rr.to_querystring())
 
-    def test_more_than_one_resource_request_without_group_policy_warns(self):
-        extra_specs = {
-            'resources:VCPU': '2',
-            'resources1:CUSTOM_FOO': '1'
-        }
-        rr = utils.ResourceRequest.from_extra_specs(extra_specs)
-        rr.add_request_group(objects.RequestGroup(resources={'CUSTOM_BAR': 5}))
-
-        rr.to_querystring()
-        self.assertIn(
-            "There is more than one numbered request group in the allocation "
-            "candidate query but the flavor did not specify any group policy.",
-            self.stdlog.logger.output)
-
     def test_resource_request_from_extra_specs_append_request(self):
         extra_specs = {
             'resources:VCPU': '2',
@@ -1138,3 +1125,154 @@ def test_get_weight_multiplier(self):
             1.8,
             utils.get_weight_multiplier(host1, 'cpu_weight_multiplier', 1.0)
         )
+
+
+class TestResourcesFromRequestGroupDefaultPolicy(test.NoDBTestCase):
+    """These test cases assert what happens when the group policy is missing
+    from the flavor but more than one numbered request group is requested from
+    various sources. Note that while image can provide required traits for the
+    resource request those traits are always added to the unnumbered group so
+    image cannot be a source of additional numbered groups.
+    """
+
+    def setUp(self):
+        super(TestResourcesFromRequestGroupDefaultPolicy, self).setUp()
+        self.context = nova_context.get_admin_context()
+        self.port_group1 = objects.RequestGroup.from_port_request(
+            self.context, uuids.port1,
+            port_resource_request={
+                "resources": {
+                    "NET_BW_IGR_KILOBIT_PER_SEC": 1000,
+                    "NET_BW_EGR_KILOBIT_PER_SEC": 1000},
+                "required": ["CUSTOM_PHYSNET_2",
+                             "CUSTOM_VNIC_TYPE_NORMAL"]
+            })
+        self.port_group2 = objects.RequestGroup.from_port_request(
+            self.context, uuids.port2,
+            port_resource_request={
+                "resources": {
+                    "NET_BW_IGR_KILOBIT_PER_SEC": 2000,
+                    "NET_BW_EGR_KILOBIT_PER_SEC": 2000},
+                "required": ["CUSTOM_PHYSNET_3",
+                             "CUSTOM_VNIC_TYPE_DIRECT"]
+            })
+
+    def test_one_group_from_flavor_dont_warn(self):
+        flavor = objects.Flavor(
+            vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0,
+            extra_specs={
+                'resources1:CUSTOM_BAR': '2',
+            })
+        request_spec = objects.RequestSpec(
+            flavor=flavor, image=objects.ImageMeta(), requested_resources=[])
+
+        rr = utils.resources_from_request_spec(
+            self.context, request_spec, host_manager=mock.Mock())
+
+        log = self.stdlog.logger.output
+        self.assertNotIn(
+            "There is more than one numbered request group in the allocation "
+            "candidate query but the flavor did not specify any group policy.",
+            log)
+        self.assertNotIn(
+            "To avoid the placement failure nova defaults the group policy to "
+            "'none'.",
+            log)
+        self.assertIsNone(rr.group_policy)
+        self.assertNotIn('group_policy=none', rr.to_querystring())
+
+    def test_one_group_from_port_dont_warn(self):
+        flavor = objects.Flavor(
+            vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0,
+            extra_specs={})
+        request_spec = objects.RequestSpec(
+            flavor=flavor, image=objects.ImageMeta(),
+            requested_resources=[self.port_group1])
+
+        rr = utils.resources_from_request_spec(
+            self.context, request_spec, host_manager=mock.Mock())
+
+        log = self.stdlog.logger.output
+        self.assertNotIn(
+            "There is more than one numbered request group in the allocation "
+            "candidate query but the flavor did not specify any group policy.",
+            log)
+        self.assertNotIn(
+            "To avoid the placement failure nova defaults the group policy to "
+            "'none'.",
+            log)
+        self.assertIsNone(rr.group_policy)
+        self.assertNotIn('group_policy=none', rr.to_querystring())
+
+    def test_two_groups_from_flavor_only_warns(self):
+        flavor = objects.Flavor(
+            vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0,
+            extra_specs={
+                'resources1:CUSTOM_BAR': '2',
+                'resources2:CUSTOM_FOO': '1'
+            })
+        request_spec = objects.RequestSpec(
+            flavor=flavor, image=objects.ImageMeta(), requested_resources=[])
+
+        rr = utils.resources_from_request_spec(
+            self.context, request_spec, host_manager=mock.Mock())
+
+        log = self.stdlog.logger.output
+        self.assertIn(
+            "There is more than one numbered request group in the allocation "
+            "candidate query but the flavor did not specify any group policy.",
+            log)
+        self.assertNotIn(
+            "To avoid the placement failure nova defaults the group policy to "
+            "'none'.",
+            log)
+        self.assertIsNone(rr.group_policy)
+        self.assertNotIn('group_policy', rr.to_querystring())
+
+    def test_one_group_from_flavor_one_from_port_policy_defaulted(self):
+        flavor = objects.Flavor(
+            vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0,
+            extra_specs={
+                'resources1:CUSTOM_BAR': '2',
+            })
+        request_spec = objects.RequestSpec(
+            flavor=flavor, image=objects.ImageMeta(),
+            requested_resources=[self.port_group1])
+
+        rr = utils.resources_from_request_spec(
+            self.context, request_spec, host_manager=mock.Mock())
+
+        log = self.stdlog.logger.output
+        self.assertIn(
+            "There is more than one numbered request group in the allocation "
+            "candidate query but the flavor did not specify any group policy.",
+            log)
+        self.assertIn(
+            "To avoid the placement failure nova defaults the group policy to "
+            "'none'.",
+            log)
+        self.assertEqual('none', rr.group_policy)
+        self.assertIn('group_policy=none', rr.to_querystring())
+
+    def test_two_groups_from_ports_policy_defaulted(self):
+        flavor = objects.Flavor(
+            vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0,
+            extra_specs={})
+        request_spec = objects.RequestSpec(
+            flavor=flavor, image=objects.ImageMeta(),
+            requested_resources=[self.port_group1, self.port_group2])
+
+        rr = utils.resources_from_request_spec(
+            self.context, request_spec, host_manager=mock.Mock())
+
+        log = self.stdlog.logger.output
+        self.assertIn(
+            "There is more than one numbered request group in the allocation "
+            "candidate query but the flavor did not specify any group policy.",
+            log)
+        self.assertIn(
+            "To avoid the placement failure nova defaults the group policy to "
+            "'none'.",
+            log)
+        self.assertEqual('none', rr.group_policy)
+        self.assertIn('group_policy=none', rr.to_querystring())
diff --git a/releasenotes/notes/defaulting_group_policy-36f584cd3920818c.yaml b/releasenotes/notes/defaulting_group_policy-36f584cd3920818c.yaml
@@ -0,0 +1,19 @@
+---
+other:
+  - |
+    `Numbered request groups`_ can be defined in the flavor extra_spec
+    but they can come from other sources as well (e.g. neutron ports).
+    If there is more than one numbered request group in the
+    allocation candidate query and the flavor does not specify any
+    group policy then the query will fail in placement as group_policy
+    is mandatory in this case. Nova previously printed a warning to the
+    scheduler logs but let the request fail. However the creator of
+    the flavor cannot know if the flavor later on will be used in a boot
+    request that has other numbered request groups. So nova will start
+    defaulting the group_policy to 'none' which means that the resource
+    providers fulfilling the numbered request groups can overlap.
+    Nova will only default the group_policy if it is not provided in the flavor
+    extra_spec, and there is more than one numbered request group present in
+    the final request, and the flavor only provided one or zero of such groups.
+
+    .. _`Numbered request groups`: https://docs.openstack.org/nova/latest/user/flavors.html#extra-specs-numbered-resource-groupings
