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
2018-03-19 20:07:12 +00:00 · 2018-03-19 20:07:12 +00:00 · 77d3fc3838
commit 77d3fc3838
parent 17ad34ff19
9 changed files with 65 additions and 48 deletions
--- 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':
--- 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=<agg UUIDs> queryparam on
+    '1.21',  # Support ?member_of=in:<agg UUIDs> queryparam on
             # GET /allocation_candidates
    '1.22',  # Support forbidden traits in the required parameter of
             # GET /resource_providers and GET /allocation_candidates
--- 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 <=
--- 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
 ------------------------------------------------------------------------------
--- 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
--- 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
--- 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

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