From 4040f8980f813173b80ba36c29ef31a9b304ef81 Mon Sep 17 00:00:00 2001 From: Tetsuro Nakamura Date: Thu, 28 Mar 2019 09:46:36 +0000 Subject: [PATCH] Refactor aggregate _get_trees_matching_all() This patch refactors _get_trees_matching_all() not to return orphaned resource provider due to aggregate filter. Change-Id: I4d23ecdca67ca0864b24e25902df53c64e7329a7 --- placement/objects/resource_provider.py | 63 +++++++++++-------- .../db/test_allocation_candidates.py | 9 +-- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/placement/objects/resource_provider.py b/placement/objects/resource_provider.py index 64084de88..619ffcb73 100644 --- a/placement/objects/resource_provider.py +++ b/placement/objects/resource_provider.py @@ -1863,6 +1863,19 @@ def get_trees_matching_all(ctx, resources, required_traits, forbidden_traits, are limited to the resource providers under the given root resource provider. """ + # If 'member_of' has values, do a separate lookup to identify the + # resource providers that meet the member_of constraints. + if member_of: + rps_in_aggs = provider_ids_matching_aggregates(ctx, member_of) + if not rps_in_aggs: + # Short-circuit. The user either asked for a non-existing + # aggregate or there were no resource providers that matched + # the requirements... + return rp_candidates.RPCandidateList() + + if forbidden_aggs: + rps_bad_aggs = provider_ids_matching_aggregates(ctx, [forbidden_aggs]) + # To get all trees that collectively have all required resource, # aggregates and traits, we use `RPCandidateList` which has a list of # three-tuples with the first element being resource provider ID, the @@ -1902,6 +1915,29 @@ def get_trees_matching_all(ctx, resources, required_traits, forbidden_traits, len(sharing_providers), amount, rc_name, len(provs_with_inv_rc.trees)) + if member_of: + # Aggregate on root spans the whole tree, so the rp itself + # *or its root* should be in the aggregate + provs_with_inv_rc.filter_by_rp_or_tree(rps_in_aggs) + LOG.debug("found %d providers under %d trees after applying " + "aggregate filter %s", + len(provs_with_inv_rc.rps), len(provs_with_inv_rc.trees), + member_of) + if not provs_with_inv_rc: + # Short-circuit returning an empty RPCandidateList + return rp_candidates.RPCandidateList() + if forbidden_aggs: + # Aggregate on root spans the whole tree, so the rp itself + # *and its root* should be outside the aggregate + provs_with_inv_rc.filter_by_rp_nor_tree(rps_bad_aggs) + LOG.debug("found %d providers under %d trees after applying " + "negative aggregate filter %s", + len(provs_with_inv_rc.rps), len(provs_with_inv_rc.trees), + forbidden_aggs) + if not provs_with_inv_rc: + # Short-circuit returning an empty RPCandidateList + return rp_candidates.RPCandidateList() + # Adding the resource providers we've got for this resource class, # filter provs_with_inv to have only trees with enough inventories # for this resource class. Here "tree" includes sharing providers @@ -1914,33 +1950,6 @@ def get_trees_matching_all(ctx, resources, required_traits, forbidden_traits, if not provs_with_inv: return rp_candidates.RPCandidateList() - # If 'member_of' has values, do a separate lookup to identify the - # resource providers that meet the member_of constraints. - if member_of: - rps_in_aggs = provider_ids_matching_aggregates( - ctx, member_of, rp_ids=provs_with_inv.all_rps) - if not rps_in_aggs: - # Short-circuit. The user either asked for a non-existing - # aggregate or there were no resource providers that matched - # the requirements... - return rp_candidates.RPCandidateList() - # Aggregate on root spans the whole tree, so the rp itself - # *or its root* should be in the aggregate - provs_with_inv.filter_by_rp_or_tree(rps_in_aggs) - LOG.debug("found %d providers under %d trees after applying " - "aggregate filter %s", - len(provs_with_inv.rps), len(provs_with_inv.trees), - member_of) - - if forbidden_aggs: - rps_bad_aggs = provider_ids_matching_aggregates( - ctx, [forbidden_aggs], rp_ids=provs_with_inv.all_rps) - provs_with_inv.filter_by_rp_nor_tree(rps_bad_aggs) - LOG.debug("found %d providers under %d trees after applying " - "negative aggregate filter %s", - len(provs_with_inv.rps), len(provs_with_inv.trees), - forbidden_aggs) - if (not required_traits and not forbidden_traits) or ( any(sharing.values())): # If there were no traits required, there's no difference in how we diff --git a/placement/tests/functional/db/test_allocation_candidates.py b/placement/tests/functional/db/test_allocation_candidates.py index 4c2a43e69..0b9dcfa9f 100644 --- a/placement/tests/functional/db/test_allocation_candidates.py +++ b/placement/tests/functional/db/test_allocation_candidates.py @@ -492,15 +492,8 @@ class ProviderTreeDBHelperTestCase(tb.PlacementDbBaseTestCase): # non-root does NOT span the whole tree. Thus cn2 can't provide VCPU # or MEMORY_MB resource member_of = [[uuids.agg2]] - # TODO(tetsuro): "cn2_numa1_pf1" comes up. This will be filtered out - # later in _alloc_candidates_multiple_providers(), so this is not a - # bug that operator faces, but filtering this out here is cleaner. - # expected_trees = ['cn1', 'cn3'] - # expected_rps = ['cn1', 'cn1_numa0_pf0', 'cn1_numa1_pf1', - # 'cn3', 'cn3_numa0_pf0', 'cn3_numa1_pf1'] - expected_trees = ['cn1', 'cn2', 'cn3'] + expected_trees = ['cn1', 'cn3'] expected_rps = ['cn1', 'cn1_numa0_pf0', 'cn1_numa1_pf1', - 'cn2_numa1_pf1', 'cn3', 'cn3_numa0_pf0', 'cn3_numa1_pf1'] _run_test(expected_trees, expected_rps, member_of=member_of)