openstack/placement
Code Issues Proposed changes

Manage mappings on AllocationRequest object

Browse Source 
This changes the handling required for creating the 'mappings'
output in the allocation_requests section of the GET
/allocation_candidates response.

Whereas before they were tracked as a suffix on the
AllocationRequestResource object, this became untenable when it
was realized (see the linked Story) that sometimes multiple
request groups (and thus suffixes) can consume resources from
the same provider and the original implementation did not
account for this. Change Ifb687698dd1aa434781d06c7f111d8969120bf71
provides a fix for this but it is represents some cognitive and
maintenance issues because it associates a vector value with a
scalar concept. That is: it is illogical for an
AllocationRequestResource (a representation of single consumption of
a resource) with multiple suffixes doing that consumption.

Instead, a new mappings attribute is added to AllocationRequest.
This is probably where it should have been from the start, because
the things we want to manage are:

* mappings
* in an allocation_request

All existing tests pass. An additional test, borrowed from
Ifb687698dd1aa434781d06c7f111d8969120bf71 , is also included to make
sure we cover some of the ideas raised in that patch.

Change-Id: Iba590d618fa493a4516d41f6b4ad5b5d0a8a07cb
Story: #2006068
This commit is contained in:
Chris Dent 2019-07-02 18:54:28 +01:00 committed by Tetsuro Nakamura
parent 7cadf3e82a
commit 06256d86c7
4 changed files with 131 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
placement/handlers/allocation_candidate.py
View File

@ -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

44
placement/objects/allocation_candidate.py
View File

@ -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

119
placement/tests/functional/db/test_allocation_candidates.py
View File

@ -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)

10
placement/tests/functional/gabbits/allocation-candidates-mappings-numa.yaml
View File

@ -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.