diff --git a/placement/handlers/allocation_candidate.py b/placement/handlers/allocation_candidate.py index 8c388aea3..cb0d1eb6f 100644 --- a/placement/handlers/allocation_candidate.py +++ b/placement/handlers/allocation_candidate.py @@ -68,17 +68,12 @@ def _transform_allocation_requests_dict(alloc_reqs, want_version): for ar in alloc_reqs: # A default dict of {$rp_uuid: "resources": {}) rp_resources = collections.defaultdict(lambda: dict(resources={})) - # A dict to map request group suffixes to the providers that provided - # solutions to that group. - mappings = collections.defaultdict(set) for rr in ar.resource_requests: - suffix = rr.suffix - mappings[suffix].add(rr.resource_provider.uuid) res_dict = rp_resources[rr.resource_provider.uuid]['resources'] res_dict[rr.resource_class] = rr.amount result = dict(allocations=rp_resources) if want_version.matches((1, 34)): - result['mappings'] = mappings + result['mappings'] = ar.mappings results.append(result) return results diff --git a/placement/objects/allocation_candidate.py b/placement/objects/allocation_candidate.py index efe7e2de8..d177150ca 100644 --- a/placement/objects/allocation_candidate.py +++ b/placement/objects/allocation_candidate.py @@ -167,7 +167,8 @@ class AllocationCandidates(object): class AllocationRequest(object): def __init__(self, anchor_root_provider_uuid=None, - use_same_provider=None, resource_requests=None): + use_same_provider=None, resource_requests=None, + mappings=None): # UUID of (the root of the tree including) the non-sharing resource # provider associated with this AllocationRequest. Internal use only, # not included when the object is serialized for output. @@ -178,6 +179,9 @@ class AllocationRequest(object): # use only, not included when the object is serialized for output. self.use_same_provider = use_same_provider self.resource_requests = resource_requests or [] + # mappings will be presented as a dict during output, so ensure we have + # a reasonable default here, despite mappings always being set. + self.mappings = mappings or dict() def __repr__(self): anchor = (self.anchor_root_provider_uuid[-8:] @@ -193,7 +197,8 @@ class AllocationRequest(object): return repr_str def __eq__(self, other): - return set(self.resource_requests) == set(other.resource_requests) + return (set(self.resource_requests) == set(other.resource_requests) + and self.mappings == other.mappings) def __hash__(self): # We need a stable sort order on the resource requests to get an @@ -207,23 +212,20 @@ class AllocationRequest(object): class AllocationRequestResource(object): def __init__(self, resource_provider=None, resource_class=None, - amount=None, suffix=''): + amount=None): self.resource_provider = resource_provider self.resource_class = resource_class self.amount = amount - self.suffix = suffix def __eq__(self, other): return ((self.resource_provider.id == other.resource_provider.id) and (self.resource_class == other.resource_class) and - (self.amount == other.amount) and - (self.suffix == other.suffix)) + (self.amount == other.amount)) def __hash__(self): return hash((self.resource_provider.id, self.resource_class, - self.amount, - self.suffix)) + self.amount)) class ProviderSummary(object): @@ -292,8 +294,7 @@ def _alloc_candidates_multiple_providers(rg_ctx, rp_candidates): AllocationRequestResource( resource_provider=rp_summary.resource_provider, resource_class=rc_cache.RC_CACHE.string_from_id(rp.rc_id), - amount=rg_ctx.resources[rp.rc_id], - suffix=rg_ctx.suffix)) + amount=rg_ctx.resources[rp.rc_id])) # Next, build up a set of allocation requests. These allocation requests # are AllocationRequest objects, containing resource provider UUIDs, @@ -333,9 +334,14 @@ def _alloc_candidates_multiple_providers(rg_ctx, rp_candidates): rg_ctx.forbidden_trait_map): # This combination doesn't satisfy trait constraints continue - root_alloc_reqs.add( - AllocationRequest(resource_requests=list(res_requests), - anchor_root_provider_uuid=root_uuid)) + + mappings = collections.defaultdict(set) + for rr in res_requests: + mappings[rg_ctx.suffix].add(rr.resource_provider.uuid) + alloc_req = AllocationRequest(resource_requests=list(res_requests), + anchor_root_provider_uuid=root_uuid, + mappings=mappings) + root_alloc_reqs.add(alloc_req) alloc_requests |= root_alloc_reqs return list(alloc_requests), list(summaries.values()) @@ -427,16 +433,18 @@ def _allocation_request_for_provider(ctx, requested_resources, provider, AllocationRequestResource( resource_provider=provider, resource_class=rc_cache.RC_CACHE.string_from_id(rc_id), - amount=amount, suffix=suffix + amount=amount ) for rc_id, amount in requested_resources.items() ] # NOTE(efried): This method only produces an AllocationRequest with its # anchor in its own tree. If the provider is a sharing provider, the # caller needs to identify the other anchors with which it might be # associated. + mappings = {suffix: set([provider.uuid])} return AllocationRequest( resource_requests=resource_requests, - anchor_root_provider_uuid=provider.root_provider_uuid) + anchor_root_provider_uuid=provider.root_provider_uuid, + mappings=mappings) def _build_provider_summaries(context, usages, prov_traits): @@ -580,6 +588,7 @@ def _consolidate_allocation_requests(areqs): # areqs must have at least one element. Save the anchor to populate the # returned AllocationRequest. anchor_rp_uuid = areqs[0].anchor_root_provider_uuid + mappings = collections.defaultdict(set) for areq in areqs: # Sanity check: the anchor should be the same for every areq if anchor_rp_uuid != areq.anchor_root_provider_uuid: @@ -594,9 +603,12 @@ def _consolidate_allocation_requests(areqs): arrs_by_rp_rc[key] = copy.copy(arr) else: arrs_by_rp_rc[key].amount += arr.amount + for suffix, providers in areq.mappings.items(): + mappings[suffix].update(providers) return AllocationRequest( resource_requests=list(arrs_by_rp_rc.values()), - anchor_root_provider_uuid=anchor_rp_uuid) + anchor_root_provider_uuid=anchor_rp_uuid, + mappings=mappings) @db_api.placement_context_manager.reader diff --git a/placement/tests/functional/db/test_allocation_candidates.py b/placement/tests/functional/db/test_allocation_candidates.py index d182744c9..8d49c501d 100644 --- a/placement/tests/functional/db/test_allocation_candidates.py +++ b/placement/tests/functional/db/test_allocation_candidates.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import collections + import os_resource_classes as orc import os_traits from oslo_utils.fixture import uuidsentinel as uuids @@ -943,8 +945,21 @@ class AllocationCandidatesTestCase(tb.PlacementDbBaseTestCase): return ac_obj.AllocationCandidates.get_by_requests( self.ctx, groups, rqparams) + def _mappings_to_suffix(self, mappings): + """Turn a dict of AllocationRequest mappings keyed on suffix to + a dict, keyed by uuid, of lists of suffixes. + """ + suffix_by_uuid = collections.defaultdict(set) + for suffix, rps in mappings.items(): + for rp_uuid in rps: + suffix_by_uuid[rp_uuid].add(suffix) + listed_sorted_suffixes = {} + for suffix, rps in suffix_by_uuid.items(): + listed_sorted_suffixes[suffix] = sorted(list(rps)) + return listed_sorted_suffixes + def _validate_allocation_requests(self, expected, candidates, - expect_suffix=False): + expect_suffixes=False): """Assert correctness of allocation requests in allocation candidates. This is set up to make it easy for the caller to specify the expected @@ -960,19 +975,22 @@ class AllocationCandidatesTestCase(tb.PlacementDbBaseTestCase): ... ] :param candidates: The result from AllocationCandidates.get_by_requests - :param expect_suffix: If True, validate the RequestGroup suffix in the - results, found as a 4th member of the tuple - described above. + :param expect_suffixes: If True, validate the AllocationRequest + mappings in the results, found as a list of + suffixes in 4th member of the tuple described + above. """ # Extract/convert allocation requests from candidates observed = [] for ar in candidates.allocation_requests: + suffix_by_uuid = self._mappings_to_suffix(ar.mappings) rrs = [] for rr in ar.resource_requests: req_tuple = (self.rp_uuid_to_name[rr.resource_provider.uuid], rr.resource_class, rr.amount) - if expect_suffix: - req_tuple = req_tuple + (rr.suffix, ) + if expect_suffixes: + req_tuple = (req_tuple + + (suffix_by_uuid[rr.resource_provider.uuid], )) rrs.append(req_tuple) rrs.sort() observed.append(rrs) @@ -1073,10 +1091,10 @@ class AllocationCandidatesTestCase(tb.PlacementDbBaseTestCase): ) expected = [ - [('cn1', orc.VCPU, 1, '')] + [('cn1', orc.VCPU, 1, [''])] ] self._validate_allocation_requests( - expected, alloc_cands, expect_suffix=True) + expected, alloc_cands, expect_suffixes=True) expected = { 'cn1': set([ @@ -3038,18 +3056,18 @@ class AllocationCandidatesTestCase(tb.PlacementDbBaseTestCase): }, rqparams=placement_lib.RequestWideParams(group_policy='isolate')) expected = [ - [('cn1', orc.VCPU, 2, ''), - ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 1, '_NET1'), - ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 1, '_NET2')], - [('cn1', orc.VCPU, 2, ''), - ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 1, '_NET2'), - ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 1, '_NET1')], - [('cn2', orc.VCPU, 2, ''), - ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 1, '_NET1'), - ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 1, '_NET2')], - [('cn2', orc.VCPU, 2, ''), - ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 1, '_NET2'), - ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 1, '_NET1')], + [('cn1', orc.VCPU, 2, ['']), + ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET1']), + ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET2'])], + [('cn1', orc.VCPU, 2, ['']), + ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET2']), + ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET1'])], + [('cn2', orc.VCPU, 2, ['']), + ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET1']), + ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET2'])], + [('cn2', orc.VCPU, 2, ['']), + ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET2']), + ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET1'])], ] # Near the end of _merge candidates we expect 4 different collections @@ -3061,4 +3079,63 @@ class AllocationCandidatesTestCase(tb.PlacementDbBaseTestCase): # amount. self.assertEqual(4, len(alloc_cands.allocation_requests)) self._validate_allocation_requests( - expected, alloc_cands, expect_suffix=True) + expected, alloc_cands, expect_suffixes=True) + + def test_nested_result_suffix_mappings_non_isolated(self): + """Confirm that paying attention to suffix mappings expands + the quantity of results and confirm those results. + """ + self._create_nested_trees() + # Make a granular request to check count and suffixes of results. + alloc_cands = self._get_allocation_candidates({ + '': placement_lib.RequestGroup( + use_same_provider=False, + resources={ + orc.VCPU: 2, + }), + '_NET1': placement_lib.RequestGroup( + use_same_provider=True, + resources={ + orc.SRIOV_NET_VF: 1, + }), + '_NET2': placement_lib.RequestGroup( + use_same_provider=True, + resources={ + orc.SRIOV_NET_VF: 1, + }), + }, rqparams=placement_lib.RequestWideParams(group_policy='none')) + + # We get four candidates from each compute node: + # [A] Two where one VF comes from each PF+RequestGroup combination. + # [B] Two where both VFs come from the same PF (which satisfies both + # RequestGroupZ). + expected = [ + # [A] (cn1) + [('cn1', orc.VCPU, 2, ['']), + ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET1']), + ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET2'])], + [('cn1', orc.VCPU, 2, ['']), + ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET2']), + ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET1'])], + # [B] (cn1) + [('cn1', orc.VCPU, 2, ['']), + ('cn1_numa0_pf0', orc.SRIOV_NET_VF, 2, ['_NET1', '_NET2'])], + [('cn1', orc.VCPU, 2, ['']), + ('cn1_numa1_pf1', orc.SRIOV_NET_VF, 2, ['_NET1', '_NET2'])], + # [A] (cn2) + [('cn2', orc.VCPU, 2, ['']), + ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET1']), + ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET2'])], + [('cn2', orc.VCPU, 2, ['']), + ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 1, ['_NET2']), + ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 1, ['_NET1'])], + # [B] (cn2) + [('cn2', orc.VCPU, 2, ['']), + ('cn2_numa0_pf0', orc.SRIOV_NET_VF, 2, ['_NET1', '_NET2'])], + [('cn2', orc.VCPU, 2, ['']), + ('cn2_numa1_pf1', orc.SRIOV_NET_VF, 2, ['_NET1', '_NET2'])], + ] + + self.assertEqual(8, len(alloc_cands.allocation_requests)) + self._validate_allocation_requests( + expected, alloc_cands, expect_suffixes=True) diff --git a/placement/tests/functional/gabbits/allocation-candidates-mappings-numa.yaml b/placement/tests/functional/gabbits/allocation-candidates-mappings-numa.yaml index 3a39391e0..d98fdc2cd 100644 --- a/placement/tests/functional/gabbits/allocation-candidates-mappings-numa.yaml +++ b/placement/tests/functional/gabbits/allocation-candidates-mappings-numa.yaml @@ -102,13 +102,11 @@ tests: response_json_paths: $.allocation_requests.`len`: 1 $.provider_summaries.`len`: 12 - # BUG! https://storyboard.openstack.org/#!/story/2006068 + # Fix for https://storyboard.openstack.org/#!/story/2006068 # We should get a mapping from each request group to ESN1: - # $.allocation_requests[0].mappings: - # _BWA: ["$ENVIRON['ESN1_UUID']"] - # _BWB: ["$ENVIRON['ESN1_UUID']"] - # But we instead get only one of them. (Which one is arbitrary, I think.) - $.allocation_requests[0].mappings.`len`: 1 + $.allocation_requests[0].mappings: + _BWA: ["$ENVIRON['ESN1_UUID']"] + _BWB: ["$ENVIRON['ESN1_UUID']"] # Confirm that a resource provider which provides two different classes # of inventory only shows up in a mapping for any suffix once.