From 2e3985c30309b8e3f855035212ff4c3770f54eb4 Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Mon, 2 Dec 2019 13:38:13 -0600 Subject: [PATCH] Tie requester_id to RequestGroup suffix Previously, request groups built from ports would have a unique integer suffix generated for them as the suffix used for the GET /allocation_candidates query to placement. Since [1] we've been using a microversion permitting string suffixes (added in 1.33). This allows us to use the port UUID (pulled from the RequestGroup.requester_id) as the suffix instead. This lays the groundwork for future patches that will: - Take advantage of the suffix-to-provider mappings returned by GET /allocation_candidates as of placement microversion 1.34 (also included in [1]) to get rid of the map_requested_resources_to_providers hack. - Seamlessly support request groups from other sources, such as cyborg device profiles. [1] I52499ff6639c1a5815a8557b22dd33106dcc386b Change-Id: I55e4d61ba1c5920c44fd71eb36a9ed2cc64c4117 --- nova/exception.py | 4 ++ nova/scheduler/utils.py | 59 ++++++++++++++++------ nova/tests/unit/scheduler/test_utils.py | 66 +++++++++++++++++-------- 3 files changed, 94 insertions(+), 35 deletions(-) diff --git a/nova/exception.py b/nova/exception.py index 52a600bf5937..965b1dd52195 100644 --- a/nova/exception.py +++ b/nova/exception.py @@ -2472,3 +2472,7 @@ class GetPMEMNamespaceFailed(NovaException): class VPMEMCleanupFailed(NovaException): msg_fmt = _("Failed to clean up the vpmem backend device %(dev)s: " "%(error)s") + + +class RequestGroupSuffixConflict(NovaException): + msg_fmt = _("Duplicate request group suffix %(suffix)s!") diff --git a/nova/scheduler/utils.py b/nova/scheduler/utils.py index df2e671b0f90..c7c429b37eb7 100644 --- a/nova/scheduler/utils.py +++ b/nova/scheduler/utils.py @@ -78,6 +78,11 @@ class ResourceRequest(object): This does *not* yet handle ``member_of[$S]``. + The string suffix is used as the RequestGroup.requester_id to + facilitate mapping of requests to allocation candidates using the + ``mappings`` piece of the response added in Placement microversion 1.34 + https://docs.openstack.org/placement/train/specs/train/implemented/placement-resource-provider-request-group-mapping-in-allocation-candidates.html # noqa + For image metadata, traits are extracted from the ``traits_required`` property, if present. @@ -294,7 +299,9 @@ class ResourceRequest(object): def get_request_group(self, ident): if ident not in self._rg_by_id: - rq_grp = objects.RequestGroup(use_same_provider=bool(ident)) + rq_grp = objects.RequestGroup( + use_same_provider=bool(ident), + requester_id=ident) self._rg_by_id[ident] = rq_grp return self._rg_by_id[ident] @@ -304,24 +311,46 @@ class ResourceRequest(object): The groups coming from the flavor can have arbitrary suffixes; those are guaranteed to be unique within the flavor. - A group coming from "outside" (ports, device profiles) must be given a - suffix that is unique in combination with suffixes from the flavor. + A group coming from "outside" (ports, device profiles) must be + associated with a requester_id, such as a port UUID. We use this + requester_id as the group suffix (but ensure that it is unique in + combination with suffixes from the flavor). - .. todo:: Tie suffixes to RequestGroup.requester_id + Groups coming from "outside" are not allowed to be no-ops. That is, + they must provide resources and/or required/forbidden traits/aggregates - :param request_group: the RequestGroup to be added + :param request_group: the RequestGroup to be added. + :raise: ValueError if request_group has no requester_id, or if it + provides no resources or (required/forbidden) traits or aggregates. + :raise: RequestGroupSuffixConflict if request_group.requester_id + already exists in this ResourceRequest. """ - # Generate a unique suffix by peeling out all the suffixes that are - # integers (respond to int()) and adding 1 to the highest one. - max_ident = 0 - for ident in self._rg_by_id: - try: - max_ident = max(int(ident), max_ident) - except (TypeError, ValueError): - # Non-numeric or None (unsuffixed) - continue + # NOTE(efried): Deliberately check False-ness rather than None-ness + # here, since both would result in the unsuffixed request group being + # used, and that's bad. + if not request_group.requester_id: + # NOTE(efried): An "outside" RequestGroup is created by a + # programmatic agent and that agent is responsible for guaranteeing + # the presence of a unique requester_id. This is in contrast to + # flavor extra_specs where a human is responsible for the group + # suffix. + raise ValueError( + _('Missing requester_id in RequestGroup! This is probably a ' + 'programmer error. %s') % request_group) - self._rg_by_id[max_ident + 1] = request_group + if request_group.is_empty(): + # NOTE(efried): It is up to the calling code to enforce a nonempty + # RequestGroup with suitable logic and exceptions. + raise ValueError( + _('Refusing to add no-op RequestGroup with requester_id=%s. ' + 'This is a probably a programmer error.') % + request_group.requester_id) + + if request_group.requester_id in self._rg_by_id: + raise exception.RequestGroupSuffixConflict( + suffix=request_group.requester_id) + + self._rg_by_id[request_group.requester_id] = request_group def _add_resource(self, groupid, rclass, amount): self.get_request_group(groupid).add_resource(rclass, amount) diff --git a/nova/tests/unit/scheduler/test_utils.py b/nova/tests/unit/scheduler/test_utils.py index e1cb9a2b1f23..d9a6327a75f7 100644 --- a/nova/tests/unit/scheduler/test_utils.py +++ b/nova/tests/unit/scheduler/test_utils.py @@ -310,12 +310,14 @@ class TestUtils(TestUtilsBase): } ) expected_resources._rg_by_id['1'] = objects.RequestGroup( + requester_id='1', resources={ 'VGPU': 1, 'VGPU_DISPLAY_HEAD': 2, } ) expected_resources._rg_by_id['3'] = objects.RequestGroup( + requester_id='3', resources={ 'VCPU': 2, }, @@ -325,11 +327,13 @@ class TestUtils(TestUtilsBase): } ) expected_resources._rg_by_id['24'] = objects.RequestGroup( + requester_id='24', resources={ 'SRIOV_NET_VF': 2, }, ) expected_resources._rg_by_id['42'] = objects.RequestGroup( + requester_id='42', resources={ 'SRIOV_NET_VF': 1, } @@ -675,16 +679,40 @@ class TestUtils(TestUtilsBase): root_gb=10, ephemeral_gb=5, swap=0) - rg1 = objects.RequestGroup(resources={'CUSTOM_FOO': 1}) + rg1 = objects.RequestGroup( + resources={'CUSTOM_FOO': 1}, requester_id='The-first-group') + + # Leave requester_id out to trigger ValueError rg2 = objects.RequestGroup(required_traits={'CUSTOM_BAR'}) + reqspec = objects.RequestSpec(flavor=flavor, requested_resources=[rg1, rg2]) + + self.assertRaises( + ValueError, + utils.resources_from_request_spec, + self.context, reqspec, self.mock_host_manager) + + # Set conflicting requester_id + rg2.requester_id = 'The-first-group' + self.assertRaises( + exception.RequestGroupSuffixConflict, + utils.resources_from_request_spec, + self.context, reqspec, self.mock_host_manager) + + # Good path: nonempty non-conflicting requester_id + rg2.requester_id = 'The-second-group' + req = utils.resources_from_request_spec( self.context, reqspec, self.mock_host_manager) self.assertEqual({'MEMORY_MB': 1024, 'DISK_GB': 15, 'VCPU': 1}, req.get_request_group(None).resources) - self.assertIs(rg1, req.get_request_group(1)) - self.assertIs(rg2, req.get_request_group(2)) + self.assertIs(rg1, req.get_request_group('The-first-group')) + self.assertIs(rg2, req.get_request_group('The-second-group')) + # Make sure those ended up as suffixes correctly + qs = req.to_querystring() + self.assertIn('resourcesThe-first-group=CUSTOM_FOO%3A1', qs) + self.assertIn('requiredThe-second-group=CUSTOM_BAR', qs) def test_resources_from_request_spec_requested_resources_unfilled(self): flavor = objects.Flavor( @@ -876,6 +904,7 @@ class TestUtils(TestUtilsBase): }, ) expected._rg_by_id['1'] = objects.RequestGroup( + requester_id='1', resources={ 'SRIOV_NET_VF': 1, 'IPV4_ADDRESS': 1, @@ -888,6 +917,7 @@ class TestUtils(TestUtilsBase): }, ) expected._rg_by_id['2'] = objects.RequestGroup( + requester_id='2', resources={ 'SRIOV_NET_VF': 1, 'IPV4_ADDRESS': 2, @@ -898,6 +928,7 @@ class TestUtils(TestUtilsBase): } ) expected._rg_by_id['3'] = objects.RequestGroup( + requester_id='3', resources={ 'DISK_GB': 5, } @@ -1093,30 +1124,25 @@ class TestUtils(TestUtilsBase): vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0) rs = objects.RequestSpec(flavor=flavor, is_bfv=False) req = utils.ResourceRequest(rs) - rg1 = objects.RequestGroup() + rg1 = objects.RequestGroup(requester_id='foo', + required_traits={'CUSTOM_FOO'}) req._add_request_group(rg1) - rg2 = objects.RequestGroup() + rg2 = objects.RequestGroup(requester_id='bar', + forbidden_traits={'CUSTOM_BAR'}) req._add_request_group(rg2) - self.assertIs(rg1, req.get_request_group(1)) - self.assertIs(rg2, req.get_request_group(2)) + self.assertIs(rg1, req.get_request_group('foo')) + self.assertIs(rg2, req.get_request_group('bar')) - def test_resource_request_add_group_inserts_the_group_implicit_group(self): + def test_empty_groups_forbidden(self): + """Not allowed to add premade RequestGroup without resources/traits/ + aggregates. + """ flavor = objects.Flavor( vcpus=1, memory_mb=1024, root_gb=10, ephemeral_gb=5, swap=0) rs = objects.RequestSpec(flavor=flavor, is_bfv=False) req = utils.ResourceRequest(rs) - - # this implicitly creates the specified group - unnumbered_rg = req.get_request_group(42) - - rg1 = objects.RequestGroup() - req._add_request_group(rg1) - rg2 = objects.RequestGroup() - req._add_request_group(rg2) - - self.assertIs(rg1, req.get_request_group(43)) - self.assertIs(rg2, req.get_request_group(44)) - self.assertIs(unnumbered_rg, req.get_request_group(42)) + rg = objects.RequestGroup(requester_id='foo') + self.assertRaises(ValueError, req._add_request_group, rg) def test_claim_resources_on_destination_no_source_allocations(self): """Tests the negative scenario where the instance does not have