From 77d3fc3838dcd8efcb41a1e30c879f7bf12d4d20 Mon Sep 17 00:00:00 2001 From: Ed Leafe Date: Mon, 19 Mar 2018 20:07:12 +0000 Subject: [PATCH] Address issues raised in adding member_of to GET /a-c In an earlier patch [0], there were some valid criticisms noted. They were not critical enough to require holding off on that patch, so they are being addressed here in a follow-up patch. [0] I5857e927a830914c96e040936804e322baccc24c Blueprint: alloc-candidates-member-of Change-Id: I762dc4a70613056f1bd9ba7bf11c3a4588bdac70 --- .../placement/handlers/resource_provider.py | 16 +------ nova/api/openstack/placement/microversion.py | 2 +- .../placement/objects/resource_provider.py | 4 +- .../placement/rest_api_version_history.rst | 7 +-- nova/api/openstack/placement/util.py | 45 ++++++++++--------- .../allocation-candidates-member-of.yaml | 30 +++++++++++-- .../gabbits/resource-provider-aggregates.yaml | 2 +- .../source/allocation_candidates.inc | 2 +- placement-api-ref/source/parameters.yaml | 5 ++- 9 files changed, 65 insertions(+), 48 deletions(-) diff --git a/nova/api/openstack/placement/handlers/resource_provider.py b/nova/api/openstack/placement/handlers/resource_provider.py index 6cdbd069fa..57deb0e7bf 100644 --- a/nova/api/openstack/placement/handlers/resource_provider.py +++ b/nova/api/openstack/placement/handlers/resource_provider.py @@ -200,22 +200,8 @@ def list_resource_providers(req): for attr in qpkeys: if attr in req.GET: value = req.GET[attr] - # special case member_of to always make its value a - # list, either by accepting the single value, or if it - # starts with 'in:' splitting on ','. - # NOTE(cdent): This will all change when we start using - # JSONSchema validation of query params. if attr == 'member_of': - if value.startswith('in:'): - value = value[3:].split(',') - else: - value = [value] - # Make sure the values are actually UUIDs. - for aggr_uuid in value: - if not uuidutils.is_uuid_like(aggr_uuid): - raise webob.exc.HTTPBadRequest( - _('Invalid uuid value: %(uuid)s') % - {'uuid': aggr_uuid}) + value = util.normalize_member_of_qs_param(value) elif attr == 'resources': value = util.normalize_resources_qs_param(value) elif attr == 'required': diff --git a/nova/api/openstack/placement/microversion.py b/nova/api/openstack/placement/microversion.py index 1f8391db11..0a835097d9 100644 --- a/nova/api/openstack/placement/microversion.py +++ b/nova/api/openstack/placement/microversion.py @@ -59,7 +59,7 @@ VERSIONS = [ '1.19', # Include generation and conflict detection in provider aggregates # APIs '1.20', # Return 200 with provider payload from POST /resource_providers - '1.21', # Support ?member_of= queryparam on + '1.21', # Support ?member_of=in: queryparam on # GET /allocation_candidates '1.22', # Support forbidden traits in the required parameter of # GET /resource_providers and GET /allocation_candidates diff --git a/nova/api/openstack/placement/objects/resource_provider.py b/nova/api/openstack/placement/objects/resource_provider.py index 5e5bf054f2..18fc986cc6 100644 --- a/nova/api/openstack/placement/objects/resource_provider.py +++ b/nova/api/openstack/placement/objects/resource_provider.py @@ -1229,7 +1229,9 @@ def _get_all_with_shared(ctx, resources, member_of=None): # AND sharing_disk_gb.resource_provider_id IN ($RPS_SHARING_DISK) # INNER JOIN resource_provider_aggregates AS member_aggs # ON rp.id = member_aggs.resource_provider_id - # AND member_aggs.aggregate_id IN ($MEMBER_OF) + # INNER JOIN placement_aggregates AS p_aggs + # ON member_aggs.aggregate_id = p_aggs.id + # AND p_aggs.uuid IN ($MEMBER_OF) # WHERE ( # ( # COALESCE(usage_vcpu.used, 0) + $AMOUNT_VCPU <= diff --git a/nova/api/openstack/placement/rest_api_version_history.rst b/nova/api/openstack/placement/rest_api_version_history.rst index c12c994dec..1840da2fcf 100644 --- a/nova/api/openstack/placement/rest_api_version_history.rst +++ b/nova/api/openstack/placement/rest_api_version_history.rst @@ -258,9 +258,10 @@ a subsequent GET. Add support for the `member_of` query parameter to the `GET /allocation_candidates` API. It accepts a comma-separated list of UUIDs for -aggregates. If this parameter is provided, the only resource providers returned -will be those in one of the specified aggregates that meet the other parts of -the request. +aggregates. Note that if more than one aggregate UUID is passed, the +comma-separated list must be prefixed with the "in:" operator. If this +parameter is provided, the only resource providers returned will be those in +one of the specified aggregates that meet the other parts of the request. 1.22 Support forbidden traits on resource providers and allocations candidates ------------------------------------------------------------------------------ diff --git a/nova/api/openstack/placement/util.py b/nova/api/openstack/placement/util.py index 834896cd31..46271e8492 100644 --- a/nova/api/openstack/placement/util.py +++ b/nova/api/openstack/placement/util.py @@ -347,33 +347,36 @@ def normalize_traits_qs_param(val, allow_forbidden=False): return ret -def normalize_member_of_qs_param(val): - """Parse a member_of query string parameter value. +def normalize_member_of_qs_param(value): + """We need to handle member_of as a special case to always make its value a + list, either by accepting the single value, or if it starts with 'in:' + splitting on ','. - Valid values are either a single UUID, or the prefix 'in:' followed by two - or more comma-separated UUIDs. + NOTE(cdent): This will all change when we start using + JSONSchema validation of query params. - :param val: A member_of query parameter of either a single UUID, or a - comma-separated string of two or more UUIDs. - :return: A list of UUIDs - :raises `webob.exc.HTTPBadRequest` if the val parameter is not in the + :param value: A member_of query parameter of either a single UUID, or a + comma-separated string of one or more UUIDs, prefixed with + the "in:" operator. + :return: A set of UUIDs + :raises `webob.exc.HTTPBadRequest` if the value parameter is not in the expected format. """ - # Ensure that multiple values are prefixed with "in:" - if "," in val and not val.startswith("in:"): + if "," in value and not value.startswith("in:"): msg = _("Multiple values for 'member_of' must be prefixed with the " - "'in:' keyword. Got: %s") % val + "'in:' keyword. Got: %s") % value raise webob.exc.HTTPBadRequest(msg) - if val.startswith("in:"): - ret = val[3:].split(",") + if value.startswith('in:'): + value = set(value[3:].split(',')) else: - ret = [val] - # Ensure the UUIDs are valid - if not all([uuidutils.is_uuid_like(agg) for agg in ret]): - msg = _("Invalid query string parameters: Expected 'member_of' " - "parameter to contain valid UUID(s). Got: %s") % val - raise webob.exc.HTTPBadRequest(msg) - return ret + value = set([value]) + # Make sure the values are actually UUIDs. + for aggr_uuid in value: + if not uuidutils.is_uuid_like(aggr_uuid): + msg = _("Invalid query string parameters: Expected 'member_of' " + "parameter to contain valid UUID(s). Got: %s") % value + raise webob.exc.HTTPBadRequest(msg) + return value def parse_qs_request_groups(qsdict, allow_forbidden=False): @@ -383,7 +386,7 @@ def parse_qs_request_groups(qsdict, allow_forbidden=False): The input qsdict represents a query string of the form: ?resources=$RESOURCE_CLASS_NAME:$AMOUNT,$RESOURCE_CLASS_NAME:$AMOUNT - &required=$TRAIT_NAME,$TRAIT_NAME&member_of=$AGG_UUID + &required=$TRAIT_NAME,$TRAIT_NAME&member_of=in:$AGG1_UUID,$AGG2_UUID &resources1=$RESOURCE_CLASS_NAME:$AMOUNT,RESOURCE_CLASS_NAME:$AMOUNT &required1=$TRAIT_NAME,$TRAIT_NAME&member_of1=$AGG_UUID &resources2=$RESOURCE_CLASS_NAME:$AMOUNT,RESOURCE_CLASS_NAME:$AMOUNT diff --git a/nova/tests/functional/api/openstack/placement/gabbits/allocation-candidates-member-of.yaml b/nova/tests/functional/api/openstack/placement/gabbits/allocation-candidates-member-of.yaml index a359170426..92d5e3d864 100644 --- a/nova/tests/functional/api/openstack/placement/gabbits/allocation-candidates-member-of.yaml +++ b/nova/tests/functional/api/openstack/placement/gabbits/allocation-candidates-member-of.yaml @@ -25,8 +25,7 @@ tests: GET: /allocation_candidates?resources=VCPU:1,MEMORY_MB:1024,DISK_GB:100&member_of=INVALID_UUID status: 400 response_strings: - - Invalid query string parameters - - Expected 'member_of' parameter to contain valid UUID(s) + - Expected 'member_of' parameter to contain valid UUID(s). - name: get allocation candidates no 'in:' for multiple member_of GET: /allocation_candidates?resources=VCPU:1,MEMORY_MB:1024,DISK_GB:100&member_of=$ENVIRON['AGGA_UUID'],$ENVIRON['AGGB_UUID'] @@ -38,8 +37,13 @@ tests: GET: /allocation_candidates?resources=VCPU:1,MEMORY_MB:1024,DISK_GB:100&member_of=in:$ENVIRON['AGGA_UUID'],INVALID_UUID status: 400 response_strings: - - Invalid query string parameters - - Expected 'member_of' parameter to contain valid UUID(s) + - Expected 'member_of' parameter to contain valid UUID(s). + +- name: get allocation candidates multiple member_of with 'in:' but no aggregates + GET: /allocation_candidates?&member_of=in:&resources=VCPU:1,MEMORY_MB:1024,DISK_GB:100 + status: 400 + response_strings: + - Expected 'member_of' parameter to contain valid UUID(s). - name: get allocation candidates with no match for member_of GET: /allocation_candidates?resources=VCPU:1,MEMORY_MB:1024,DISK_GB:100&member_of=$ENVIRON['AGGA_UUID'] @@ -86,3 +90,21 @@ tests: status: 200 response_json_paths: $.allocation_requests.`len`: 0 + +- name: get current compute node 1 state + GET: /resource_providers/$ENVIRON['CN1_UUID'] + +- name: now associate the first compute node with both aggA and aggC + PUT: /resource_providers/$ENVIRON['CN1_UUID']/aggregates + data: + aggregates: + - $ENVIRON['AGGA_UUID'] + - $ENVIRON['AGGC_UUID'] + resource_provider_generation: $HISTORY['get current compute node 1 state'].$RESPONSE['$.generation'] + +- name: verify that the member_of call for aggs A and B still returns 2 allocation_candidates + GET: /allocation_candidates?resources=VCPU:1,MEMORY_MB:1024,DISK_GB:100&member_of=in:$ENVIRON['AGGA_UUID'],$ENVIRON['AGGB_UUID'] + status: 200 + response_json_paths: + $.allocation_requests.`len`: 2 + status: 200 diff --git a/nova/tests/functional/api/openstack/placement/gabbits/resource-provider-aggregates.yaml b/nova/tests/functional/api/openstack/placement/gabbits/resource-provider-aggregates.yaml index 18128666c5..dbcad3e597 100644 --- a/nova/tests/functional/api/openstack/placement/gabbits/resource-provider-aggregates.yaml +++ b/nova/tests/functional/api/openstack/placement/gabbits/resource-provider-aggregates.yaml @@ -53,7 +53,7 @@ tests: GET: '/resource_providers?member_of=not+a+uuid' status: 400 response_strings: - - 'Invalid uuid value: not a uuid' + - Expected 'member_of' parameter to contain valid UUID(s). response_json_paths: $.errors[0].title: Bad Request diff --git a/placement-api-ref/source/allocation_candidates.inc b/placement-api-ref/source/allocation_candidates.inc index 38fa56805e..0d9ad1573b 100644 --- a/placement-api-ref/source/allocation_candidates.inc +++ b/placement-api-ref/source/allocation_candidates.inc @@ -32,7 +32,7 @@ Request - resources: resources_query_required - limit: allocation_candidates_limit - required: allocation_candidates_required - - member_of: member_of + - member_of: member_of_1_21 Response (microversions 1.12 - ) -------------------------------- diff --git a/placement-api-ref/source/parameters.yaml b/placement-api-ref/source/parameters.yaml index 44b7495cf9..c7f91c7ea2 100644 --- a/placement-api-ref/source/parameters.yaml +++ b/placement-api-ref/source/parameters.yaml @@ -62,7 +62,7 @@ allocation_candidates_required: *collectively* contain all of the required traits. **Starting from microversion 1.22** traits which are forbidden from any resource provider may be expressed by prefixing a trait with a ``!``. -member_of: +member_of: &member_of type: string in: query required: false @@ -75,6 +75,9 @@ member_of: member_of=5e08ea53-c4c6-448e-9334-ac4953de3cfa member_of=in:42896e0d-205d-4fe3-bd1e-100924931787,5e08ea53-c4c6-448e-9334-ac4953de3cfa min_version: 1.3 +member_of_1_21: + <<: *member_of + min_version: 1.21 project_id: &project_id type: string in: query