Defaults missing group_policy to 'none'

If more than one numbered request group is in the placement a_c query
then the group_policy is mandatory. Based on the PTG discussion [1]
'none' seems to be a good default policy from nova perspective. So this
patch makes sure that if the group_policy is not provided in the flavor
extra_spec and there are more than one numbered group in the request and
the flavor only provide one or zero groups (so groups are coming from
other sources like neutron ports) then the group_policy is defaulted to
'none'.

The reasoning behind this change: If more than one numbered request
group is coming from the flavor extra_spec then the creator of the
flavor is responsible to add a group_policy to the flavor. So in this
nova only warns but let the request fail in placement to force the
fixing of the flavor. However when numbered groups are coming from
other sources (like neutron ports) then the creator of the flavor
cannot know if additional group will be included so we don't want to
force the flavor creator but simply default the group_policy.

[1] http://lists.openstack.org/pipermail/openstack-discuss/2019-May/005807.html

Change-Id: I0681de217ed9f5d77dae0d9555632b8d160bb179

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

nova/scheduler/utils.py

@@ -66,6 +66,14 @@ class ResourceRequest(object):
         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 @@ class ResourceRequest(object):
         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 @@ class ResourceRequest(object):
             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:
@@ -548,6 +546,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
 
 

nova/tests/unit/scheduler/test_utils.py

@@ -362,7 +362,8 @@ class TestUtils(test.NoDBTestCase):
                 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 @@ class TestUtils(test.NoDBTestCase):
         )
         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 @@ class TestUtils(test.NoDBTestCase):
             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())

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