diff --git a/api-ref/source/parameters.yaml b/api-ref/source/parameters.yaml index 5a2749af9..27ddda9fa 100644 --- a/api-ref/source/parameters.yaml +++ b/api-ref/source/parameters.yaml @@ -671,9 +671,18 @@ resource_provider_parent_provider_uuid_request: type: string in: body required: false - description: > - The UUID of the immediate parent of the resource provider. Once set, the - parent of a resource provider cannot be changed. + description: | + The UUID of the immediate parent of the resource provider. + + * Before version ``1.37``, once set, the parent of a resource provider + cannot be changed. + + * Since version ``1.37``, it can be set to any existing provider UUID + excepts to providers that would cause a loop. Also it can be set to null + to transform the provider to a new root provider. This operation needs + to be used carefully. Moving providers can mean that the original rules + used to create the existing resource allocations may be invalidated + by that move. min_version: 1.14 resource_provider_parent_provider_uuid_required_no_min: type: string diff --git a/placement/handlers/resource_provider.py b/placement/handlers/resource_provider.py index a2f2effa3..3ddffe1c3 100644 --- a/placement/handlers/resource_provider.py +++ b/placement/handlers/resource_provider.py @@ -293,6 +293,8 @@ def update_resource_provider(req): if want_version.matches((1, 14)): schema = rp_schema.PUT_RP_SCHEMA_V1_14 + allow_reparenting = want_version.matches((1, 37)) + data = util.extract_json(req.body, schema) for field in rp_obj.ResourceProvider.SETTABLE_FIELDS: @@ -300,7 +302,7 @@ def update_resource_provider(req): setattr(resource_provider, field, data[field]) try: - resource_provider.save() + resource_provider.save(allow_reparenting=allow_reparenting) except db_exc.DBDuplicateEntry: raise webob.exc.HTTPConflict( 'Conflicting resource provider %(name)s already exists.' % diff --git a/placement/microversion.py b/placement/microversion.py index 3328c2f68..a83e628de 100644 --- a/placement/microversion.py +++ b/placement/microversion.py @@ -88,6 +88,7 @@ VERSIONS = [ '1.35', # Add a `root_required` queryparam on `GET /allocation_candidates` '1.36', # Add a `same_subtree` parameter on GET /allocation_candidates # and allow resourceless requests for groups in `same_subtree`. + '1.37', # Allow re-parenting and un-parenting resource providers ] diff --git a/placement/objects/resource_provider.py b/placement/objects/resource_provider.py index be41adbf3..c8baff08c 100644 --- a/placement/objects/resource_provider.py +++ b/placement/objects/resource_provider.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections import copy # NOTE(cdent): The resource provider objects are designed to never be @@ -581,14 +582,21 @@ class ResourceProvider(object): def destroy(self): self._delete(self._context, self.id) - def save(self): + def save(self, allow_reparenting=False): + """Save the changes to the database + + :param allow_reparenting: If True then it allows changing the parent RP + to a different RP as well as changing it to None (un-parenting). + If False, then only changing the parent from None to an RP is allowed + the rest is rejected with ObjectActionError. + """ # These are the only fields we are willing to save with. # If there are others, ignore them. updates = { 'name': self.name, 'parent_provider_uuid': self.parent_provider_uuid, } - self._update_in_db(self._context, self.id, updates) + self._update_in_db(self._context, self.id, updates, allow_reparenting) @classmethod def get_by_uuid(cls, context, uuid): @@ -769,21 +777,14 @@ class ResourceProvider(object): raise exception.NotFound() @db_api.placement_context_manager.writer - def _update_in_db(self, context, id, updates): - # A list of resource providers in the same tree with the - # resource provider to update - same_tree = [] + def _update_in_db(self, context, id, updates, allow_reparenting): + # A list of resource providers in the subtree of resource provider to + # update + subtree_rps = [] + # The new root RP if changed + new_root_id = None + new_root_uuid = None if 'parent_provider_uuid' in updates: - # TODO(jaypipes): For now, "re-parenting" and "un-parenting" are - # not possible. If the provider already had a parent, we don't - # allow changing that parent due to various issues, including: - # - # * if the new parent is a descendant of this resource provider, we - # introduce the possibility of a loop in the graph, which would - # be very bad - # * potentially orphaning heretofore-descendants - # - # So, for now, let's just prevent re-parenting... my_ids = res_ctx.provider_ids_from_uuid(context, self.uuid) parent_uuid = updates.pop('parent_provider_uuid') if parent_uuid is not None: @@ -795,54 +796,69 @@ class ResourceProvider(object): action='create', reason='parent provider UUID does not exist.') if (my_ids.parent_id is not None and - my_ids.parent_id != parent_ids.id): + my_ids.parent_id != parent_ids.id and + not allow_reparenting): raise exception.ObjectActionError( action='update', reason='re-parenting a provider is not currently ' 'allowed.') - if my_ids.parent_uuid is None: - # So the user specifies a parent for an RP that doesn't - # have one. We have to check that by this new parent we - # don't create a loop in the tree. Basically the new parent - # cannot be the RP itself or one of its descendants. - # However as the RP's current parent is None the above - # condition is the same as "the new parent cannot be any RP - # from the current RP tree". - same_tree = get_all_by_filters( - context, filters={'in_tree': self.uuid}) - rp_uuids_in_the_same_tree = [rp.uuid for rp in same_tree] - if parent_uuid in rp_uuids_in_the_same_tree: - raise exception.ObjectActionError( - action='update', - reason='creating loop in the provider tree is ' - 'not allowed.') + # So the user specified a new parent. We have to make sure + # that the new parent is not a descendant of the + # current RP to avoid a loop in the graph. It could be + # easily checked by traversing the tree from the new parent + # up to the root and see if we ever hit the current RP + # along the way. However later we need to update every + # descendant of the current RP with a possibly new root + # so we go with the more expensive way and gather every + # descendant for the current RP and check if the new + # parent is part of that set. + subtree_rps = self.get_subtree(context) + subtree_rp_uuids = {rp.uuid for rp in subtree_rps} + if parent_uuid in subtree_rp_uuids: + raise exception.ObjectActionError( + action='update', + reason='creating loop in the provider tree is ' + 'not allowed.') updates['root_provider_id'] = parent_ids.root_id updates['parent_provider_id'] = parent_ids.id self.root_provider_uuid = parent_ids.root_uuid + new_root_id = parent_ids.root_id + new_root_uuid = parent_ids.root_uuid else: if my_ids.parent_id is not None: - raise exception.ObjectActionError( - action='update', - reason='un-parenting a provider is not currently ' - 'allowed.') + if not allow_reparenting: + raise exception.ObjectActionError( + action='update', + reason='un-parenting a provider is not currently ' + 'allowed.') + + # we don't need to do loop detection but we still need to + # collect the RPs from the subtree so that the new root + # value is updated in the whole subtree below. + subtree_rps = self.get_subtree(context) + + # this RP becomes a new root RP + updates['root_provider_id'] = my_ids.id + updates['parent_provider_id'] = None + self.root_provider_uuid = my_ids.uuid + new_root_id = my_ids.id + new_root_uuid = my_ids.uuid db_rp = context.session.query(models.ResourceProvider).filter_by( id=id).first() db_rp.update(updates) context.session.add(db_rp) - # We should also update the root providers of resource providers - # originally in the same tree. If re-parenting is supported, - # this logic should be changed to update only descendents of the - # re-parented resource providers, not all the providers in the tree. - for rp in same_tree: + # We should also update the root providers of the resource providers + # that are in our subtree + for rp in subtree_rps: # If the parent is not updated, this clause is skipped since the - # `same_tree` has no element. - rp.root_provider_uuid = parent_ids.root_uuid + # `subtree_rps` has no element. + rp.root_provider_uuid = new_root_uuid db_rp = context.session.query( models.ResourceProvider).filter_by(id=rp.id).first() - data = {'root_provider_id': parent_ids.root_id} + data = {'root_provider_id': new_root_id} db_rp.update(data) context.session.add(db_rp) @@ -865,6 +881,31 @@ class ResourceProvider(object): setattr(resource_provider, field, db_resource_provider[field]) return resource_provider + def get_subtree(self, context, rp_uuid_to_child_rps=None): + """Return every RP from the same tree that is part of the subtree + rooted at the current RP. + + :param context: the request context + :param rp_uuid_to_child_rps: a dict of list of children + ResourceProviders keyed by the UUID of their parent RP. If it is + None then this dict is calculated locally. + :return: a list of ResourceProvider objects + """ + # if we are at a start of a recursion then prepare some data structure + if rp_uuid_to_child_rps is None: + same_tree = get_all_by_filters( + context, filters={'in_tree': self.uuid}) + rp_uuid_to_child_rps = collections.defaultdict(set) + for rp in same_tree: + if rp.parent_provider_uuid: + rp_uuid_to_child_rps[rp.parent_provider_uuid].add(rp) + + subtree = [self] + for child_rp in rp_uuid_to_child_rps[self.uuid]: + subtree.extend( + child_rp.get_subtree(context, rp_uuid_to_child_rps)) + return subtree + @db_api.placement_context_manager.reader def _get_all_by_filters_from_db(context, filters): diff --git a/placement/rest_api_version_history.rst b/placement/rest_api_version_history.rst index c5e9af97e..04fef40e8 100644 --- a/placement/rest_api_version_history.rst +++ b/placement/rest_api_version_history.rst @@ -665,3 +665,16 @@ not have a resources$S. If this is provided, at least one of the resource providers satisfying a specified request group must be an ancestor of the rest. The ``same_subtree`` query parameter can be repeated and each repeat group is treated independently. + +Xena +---- + +1.37 - Allow re-parenting and un-parenting via PUT /resource_providers/{uuid} +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +.. versionadded:: Xena + +Add support for re-parenting and un-parenting a resource provider via ``PUT +/resource_providers/{uuid}`` API by allowing changing the +``parent_provider_uuid`` to any existing provider, except providers in same +subtree. Un-parenting can be achieved by setting the ``parent_provider_uuid`` +to ``null``. This means that the provider becomes a new root provider. \ No newline at end of file diff --git a/placement/tests/functional/db/test_resource_provider.py b/placement/tests/functional/db/test_resource_provider.py index 4bb19e67c..3027b0f25 100644 --- a/placement/tests/functional/db/test_resource_provider.py +++ b/placement/tests/functional/db/test_resource_provider.py @@ -133,9 +133,34 @@ class ResourceProviderTestCase(tb.PlacementDbBaseTestCase): ) self.assertEqual('new-name', retrieved_resource_provider.name) - def test_save_reparenting_fail(self): + def test_get_subtree(self): + root1 = self._create_provider('root1') + child1 = self._create_provider('child1', parent=root1.uuid) + child2 = self._create_provider('child2', parent=root1.uuid) + grandchild1 = self._create_provider('grandchild1', parent=child1.uuid) + grandchild2 = self._create_provider('grandchild2', parent=child1.uuid) + grandchild3 = self._create_provider('grandchild3', parent=child2.uuid) + grandchild4 = self._create_provider('grandchild4', parent=child2.uuid) + + self.assertEqual( + {grandchild1.uuid}, + {rp.uuid for rp in grandchild1.get_subtree(self.context)}) + self.assertEqual( + {child1.uuid, grandchild1.uuid, grandchild2.uuid}, + {rp.uuid for rp in child1.get_subtree(self.context)}) + self.assertEqual( + {child2.uuid, grandchild3.uuid, grandchild4.uuid}, + {rp.uuid for rp in child2.get_subtree(self.context)}) + self.assertEqual( + {root1.uuid, child1.uuid, child2.uuid, + grandchild1.uuid, grandchild2.uuid, grandchild3.uuid, + grandchild4.uuid}, + {rp.uuid for rp in root1.get_subtree(self.context)}) + + def test_save_reparenting_not_allowed(self): """Tests that we prevent a resource provider's parent provider UUID - from being changed from a non-NULL value to another non-NULL value. + from being changed from a non-NULL value to another non-NULL value if + not explicitly requested. """ cn1 = self._create_provider('cn1') self._create_provider('cn2') @@ -156,6 +181,161 @@ class ResourceProviderTestCase(tb.PlacementDbBaseTestCase): exc = self.assertRaises(exception.ObjectActionError, cn1.save) self.assertIn('un-parenting a provider is not currently', str(exc)) + def test_save_reparent_same_tree(self): + root1 = self._create_provider('root1') + child1 = self._create_provider('child1', parent=root1.uuid) + child2 = self._create_provider('child2', parent=root1.uuid) + self._create_provider('grandchild1', parent=child1.uuid) + self._create_provider('grandchild2', parent=child1.uuid) + self._create_provider('grandchild3', parent=child2.uuid) + self._create_provider('grandchild4', parent=child2.uuid) + + test_rp = self._create_provider('test_rp', parent=child1.uuid) + test_rp_child = self._create_provider( + 'test_rp_child', parent=test_rp.uuid) + + # move test_rp RP upwards + test_rp.parent_provider_uuid = root1.uuid + test_rp.save(allow_reparenting=True) + + # to make sure that this re-parenting does not effect the child test RP + # in the db we need to reload it before we assert any change + test_rp_child = rp_obj.ResourceProvider.get_by_uuid( + self.ctx, test_rp_child.uuid) + + self.assertEqual(root1.uuid, test_rp.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp.root_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp_child.root_provider_uuid) + + # move downwards + test_rp.parent_provider_uuid = child1.uuid + test_rp.save(allow_reparenting=True) + + # to make sure that this re-parenting does not effect the child test RP + # in the db we need to reload it before we assert any change + test_rp_child = rp_obj.ResourceProvider.get_by_uuid( + self.ctx, test_rp_child.uuid) + + self.assertEqual(child1.uuid, test_rp.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp.root_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp_child.root_provider_uuid) + + # move sideways + test_rp.parent_provider_uuid = child2.uuid + test_rp.save(allow_reparenting=True) + + # to make sure that this re-parenting does not effect the child test RP + # in the db we need to reload it before we assert any change + test_rp_child = rp_obj.ResourceProvider.get_by_uuid( + self.ctx, test_rp_child.uuid) + + self.assertEqual(child2.uuid, test_rp.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp.root_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp_child.root_provider_uuid) + + def test_save_reparent_another_tree(self): + root1 = self._create_provider('root1') + child1 = self._create_provider('child1', parent=root1.uuid) + self._create_provider('child2', parent=root1.uuid) + + root2 = self._create_provider('root2') + self._create_provider('child3', parent=root2.uuid) + child4 = self._create_provider('child4', parent=root2.uuid) + + test_rp = self._create_provider('test_rp', parent=child1.uuid) + test_rp_child = self._create_provider( + 'test_rp_child', parent=test_rp.uuid) + + test_rp.parent_provider_uuid = child4.uuid + test_rp.save(allow_reparenting=True) + + # the re-parenting affected the the child test RP in the db so we + # have to reload it and assert the change + test_rp_child = rp_obj.ResourceProvider.get_by_uuid( + self.ctx, test_rp_child.uuid) + + self.assertEqual(child4.uuid, test_rp.parent_provider_uuid) + self.assertEqual(root2.uuid, test_rp.root_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.parent_provider_uuid) + self.assertEqual(root2.uuid, test_rp_child.root_provider_uuid) + + def test_save_reparent_to_new_root(self): + root1 = self._create_provider('root1') + child1 = self._create_provider('child1', parent=root1.uuid) + + test_rp = self._create_provider('test_rp', parent=child1.uuid) + test_rp_child = self._create_provider( + 'test_rp_child', parent=test_rp.uuid) + + # we are creating a new root from a subtree, a.k.a un-parenting + test_rp.parent_provider_uuid = None + test_rp.save(allow_reparenting=True) + + # the un-parenting affected the the child test RP in the db so we + # have to reload it and assert the change + test_rp_child = rp_obj.ResourceProvider.get_by_uuid( + self.ctx, test_rp_child.uuid) + + self.assertIsNone(test_rp.parent_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp.root_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.parent_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.root_provider_uuid) + + def test_save_reparent_the_root(self): + root1 = self._create_provider('root1') + child1 = self._create_provider('child1', parent=root1.uuid) + + # now the test_rp is also a root RP + test_rp = self._create_provider('test_rp') + test_rp_child = self._create_provider( + 'test_rp_child', parent=test_rp.uuid) + + test_rp.parent_provider_uuid = child1.uuid + test_rp.save(allow_reparenting=True) + + # the re-parenting affected the the child test RP in the db so we + # have to reload it and assert the change + test_rp_child = rp_obj.ResourceProvider.get_by_uuid( + self.ctx, test_rp_child.uuid) + + self.assertEqual(child1.uuid, test_rp.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp.root_provider_uuid) + self.assertEqual(test_rp.uuid, test_rp_child.parent_provider_uuid) + self.assertEqual(root1.uuid, test_rp_child.root_provider_uuid) + + def test_save_reparent_loop_fail(self): + root1 = self._create_provider('root1') + + test_rp = self._create_provider('test_rp', parent=root1.uuid) + test_rp_child = self._create_provider( + 'test_rp_child', parent=test_rp.uuid) + test_rp_grandchild = self._create_provider( + 'test_rp_grandchild', parent=test_rp_child.uuid) + + # self loop, i.e. we are our parents + test_rp.parent_provider_uuid = test_rp.uuid + exc = self.assertRaises( + exception.ObjectActionError, test_rp.save, allow_reparenting=True) + self.assertIn( + 'creating loop in the provider tree is not allowed.', str(exc)) + + # direct loop, i.e. our child is our parent + test_rp.parent_provider_uuid = test_rp_child.uuid + exc = self.assertRaises( + exception.ObjectActionError, test_rp.save, allow_reparenting=True) + self.assertIn( + 'creating loop in the provider tree is not allowed.', str(exc)) + + # indirect loop, i.e. our grandchild is our parent + test_rp.parent_provider_uuid = test_rp_grandchild.uuid + exc = self.assertRaises( + exception.ObjectActionError, test_rp.save, allow_reparenting=True) + self.assertIn( + 'creating loop in the provider tree is not allowed.', str(exc)) + def test_nested_providers(self): """Create a hierarchy of resource providers and run through a series of tests that ensure one cannot delete a resource provider that has no diff --git a/placement/tests/functional/gabbits/microversion.yaml b/placement/tests/functional/gabbits/microversion.yaml index 22bf589cb..f3d500766 100644 --- a/placement/tests/functional/gabbits/microversion.yaml +++ b/placement/tests/functional/gabbits/microversion.yaml @@ -41,13 +41,13 @@ tests: response_json_paths: $.errors[0].title: Not Acceptable -- name: latest microversion is 1.36 +- name: latest microversion is 1.37 GET: / request_headers: openstack-api-version: placement latest response_headers: vary: /openstack-api-version/ - openstack-api-version: placement 1.36 + openstack-api-version: placement 1.37 - name: other accept header bad version GET: / diff --git a/placement/tests/functional/gabbits/resource-provider.yaml b/placement/tests/functional/gabbits/resource-provider.yaml index c550e80dc..aa02c831c 100644 --- a/placement/tests/functional/gabbits/resource-provider.yaml +++ b/placement/tests/functional/gabbits/resource-provider.yaml @@ -378,10 +378,11 @@ tests: parent_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] status: 200 -- name: fail trying to un-parent +- name: fail trying to un-parent with old microversion PUT: /resource_providers/$ENVIRON['RP_UUID'] request_headers: content-type: application/json + openstack-api-version: placement 1.36 data: name: child parent_provider_uuid: null @@ -389,6 +390,36 @@ tests: response_strings: - 'un-parenting a provider is not currently allowed' +- name: un-parent provider + PUT: /resource_providers/$ENVIRON['RP_UUID'] + request_headers: + content-type: application/json + openstack-api-version: placement 1.37 + data: + name: child + parent_provider_uuid: null + status: 200 + response_json_paths: + $.uuid: $ENVIRON['RP_UUID'] + $.name: 'child' + $.parent_provider_uuid: null + $.root_provider_uuid: $ENVIRON['RP_UUID'] + +- name: re-parent back to its original parent after un-parent + PUT: /resource_providers/$ENVIRON['RP_UUID'] + request_headers: + content-type: application/json + openstack-api-version: placement 1.37 + data: + name: child + parent_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] + status: 200 + response_json_paths: + $.uuid: $ENVIRON['RP_UUID'] + $.name: child + $.parent_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] + $.root_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] + - name: 409 conflict while trying to delete parent with existing child DELETE: /resource_providers/$ENVIRON['PARENT_PROVIDER_UUID'] status: 409 @@ -595,10 +626,11 @@ tests: response_json_paths: $.errors[0].title: Bad Request -- name: fail trying to re-parent to a different provider +- name: fail trying to re-parent to a different provider with old microversion PUT: /resource_providers/$ENVIRON['RP_UUID'] request_headers: content-type: application/json + openstack-api-version: placement 1.36 data: name: child parent_provider_uuid: $ENVIRON['ALT_PARENT_PROVIDER_UUID'] @@ -606,6 +638,36 @@ tests: response_strings: - 're-parenting a provider is not currently allowed' +- name: re-parent to a different provider + PUT: /resource_providers/$ENVIRON['RP_UUID'] + request_headers: + content-type: application/json + openstack-api-version: placement 1.37 + data: + name: child + parent_provider_uuid: $ENVIRON['ALT_PARENT_PROVIDER_UUID'] + status: 200 + response_json_paths: + $.uuid: $ENVIRON['RP_UUID'] + $.name: 'child' + $.parent_provider_uuid: $ENVIRON['ALT_PARENT_PROVIDER_UUID'] + $.root_provider_uuid: $ENVIRON['ALT_PARENT_PROVIDER_UUID'] + +- name: re-parent back to its original parent + PUT: /resource_providers/$ENVIRON['RP_UUID'] + request_headers: + content-type: application/json + openstack-api-version: placement 1.37 + data: + name: child + parent_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] + status: 200 + response_json_paths: + $.uuid: $ENVIRON['RP_UUID'] + $.name: child + $.parent_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] + $.root_provider_uuid: $ENVIRON['PARENT_PROVIDER_UUID'] + - name: create a new provider POST: /resource_providers request_headers: diff --git a/releasenotes/notes/re-parenting-providers-94dcedff45b35bf7.yaml b/releasenotes/notes/re-parenting-providers-94dcedff45b35bf7.yaml new file mode 100644 index 000000000..cdef59ba0 --- /dev/null +++ b/releasenotes/notes/re-parenting-providers-94dcedff45b35bf7.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + With the new microversion ``1.37`` placement now supports re-parenting and + un-parenting resource providers via ``PUT /resource_providers/{uuid}`` API.