diff --git a/nova/api/openstack/placement/objects/consumer.py b/nova/api/openstack/placement/objects/consumer.py index a67eea7f36db..c9a7088e566b 100644 --- a/nova/api/openstack/placement/objects/consumer.py +++ b/nova/api/openstack/placement/objects/consumer.py @@ -217,6 +217,27 @@ class Consumer(base.VersionedObject, base.TimestampedObject): _create_in_db(self._context) self.obj_reset_changes() + def update(self): + """Used to update the consumer's project and user information without + incrementing the consumer's generation. + """ + @db_api.placement_context_manager.writer + def _update_in_db(ctx): + upd_stmt = CONSUMER_TBL.update().values( + project_id=self.project.id, user_id=self.user.id) + # NOTE(jaypipes): We add the generation check to the WHERE clause + # above just for safety. We don't need to check that the statement + # actually updated a single row. If it did not, then the + # consumer.increment_generation() call that happens in + # AllocationList.create_all() will end up raising + # ConcurrentUpdateDetected anyway + upd_stmt = upd_stmt.where(sa.and_( + CONSUMER_TBL.c.id == self.id, + CONSUMER_TBL.c.generation == self.generation)) + ctx.session.execute(upd_stmt) + _update_in_db(self._context) + self.obj_reset_changes() + def increment_generation(self): """Increments the consumer's generation. diff --git a/nova/api/openstack/placement/util.py b/nova/api/openstack/placement/util.py index 095a16e27a9d..4192673e2d36 100644 --- a/nova/api/openstack/placement/util.py +++ b/nova/api/openstack/placement/util.py @@ -16,6 +16,7 @@ import re import jsonschema from oslo_config import cfg +from oslo_log import log as logging from oslo_middleware import request_id from oslo_serialization import jsonutils from oslo_utils import timeutils @@ -34,6 +35,7 @@ from nova.api.openstack.placement.objects import user as user_obj from nova.i18n import _ CONF = cfg.CONF +LOG = logging.getLogger(__name__) # Error code handling constants ENV_ERROR_CODE = 'placement.error_code' @@ -586,6 +588,11 @@ def ensure_consumer(ctx, consumer_uuid, project_id, user_id, and User sub-objects and a boolean indicating whether a new Consumer object was created (as opposed to an existing consumer record retrieved) + :note: If the supplied project or user external identifiers do not match an + existing consumer's project and user identifiers, the existing + consumer's project and user IDs are updated to reflect the supplied + ones. + :param ctx: The request context. :param consumer_uuid: The uuid of the consumer of the resources. :param project_id: The external ID of the project consuming the resources. @@ -633,6 +640,37 @@ def ensure_consumer(ctx, consumer_uuid, project_id, user_id, 'expected_gen': consumer.generation, 'got_gen': consumer_generation, }) + # NOTE(jaypipes): The user may have specified a different project and + # user external ID than the one that we had for the consumer. If this + # is the case, go ahead and modify the consumer record with the + # newly-supplied project/user information, but do not bump the consumer + # generation (since it will be bumped in the + # AllocationList.create_all() method). + # + # TODO(jaypipes): This means that there may be a partial update. + # Imagine a scenario where a user calls POST /allocations, and the + # payload references two consumers. The first consumer is a new + # consumer and is auto-created. The second consumer is an existing + # consumer, but contains a different project or user ID than the + # existing consumer's record. If the eventual call to + # AllocationList.create_all() fails for whatever reason (say, a + # resource provider generation conflict or out of resources failure), + # we will end up deleting the auto-created consumer but we MAY not undo + # the changes to the second consumer's project and user ID. I say MAY + # and not WILL NOT because I'm not sure that the exception that gets + # raised from AllocationList.create_all() will cause the context + # manager's transaction to rollback automatically. I believe that the + # same transaction context is used for both util.ensure_consumer() and + # AllocationList.create_all() within the same HTTP request, but need to + # test this to be 100% certain... + if (project_id != consumer.project.external_id or + user_id != consumer.user.external_id): + LOG.debug("Supplied project or user ID for consumer %s was " + "different than existing record. Updating consumer " + "record.", consumer_uuid) + consumer.project = proj + consumer.user = user + consumer.update() except exception.NotFound: # If we are attempting to modify or create allocations after 1.26, we # need a consumer generation specified. The user must have specified diff --git a/nova/tests/functional/api/openstack/placement/db/test_consumer.py b/nova/tests/functional/api/openstack/placement/db/test_consumer.py index 907f44c3e45a..8fe167fc46d9 100644 --- a/nova/tests/functional/api/openstack/placement/db/test_consumer.py +++ b/nova/tests/functional/api/openstack/placement/db/test_consumer.py @@ -55,6 +55,35 @@ class ConsumerTestCase(tb.PlacementDbBaseTestCase): self.assertEqual(2, c.user.id) self.assertRaises(exception.ConsumerExists, c.create) + def test_update(self): + """Tests the scenario where a user supplies a different project/user ID + for an allocation's consumer and we call Consumer.update() to save that + information to the consumers table. + """ + # First, create the consumer with the "fake-user" and "fake-project" + # user/project in the base test class's setUp + c = consumer_obj.Consumer( + self.ctx, uuid=uuids.consumer, user=self.user_obj, + project=self.project_obj) + c.create() + c = consumer_obj.Consumer.get_by_uuid(self.ctx, uuids.consumer) + self.assertEqual(self.project_obj.id, c.project.id) + self.assertEqual(self.user_obj.id, c.user.id) + + # Now change the consumer's project and user to a different project + another_user = user_obj.User(self.ctx, external_id='another-user') + another_user.create() + another_proj = project_obj.Project( + self.ctx, external_id='another-project') + another_proj.create() + + c.project = another_proj + c.user = another_user + c.update() + c = consumer_obj.Consumer.get_by_uuid(self.ctx, uuids.consumer) + self.assertEqual(another_proj.id, c.project.id) + self.assertEqual(another_user.id, c.user.id) + @db_api.placement_context_manager.reader def _get_allocs_with_no_consumer_relationship(ctx): diff --git a/nova/tests/functional/api/openstack/placement/gabbits/allocations-bug-1779717.yaml b/nova/tests/functional/api/openstack/placement/gabbits/allocations-bug-1779717.yaml new file mode 100644 index 000000000000..13de8aa41dce --- /dev/null +++ b/nova/tests/functional/api/openstack/placement/gabbits/allocations-bug-1779717.yaml @@ -0,0 +1,102 @@ +# Test that it's possible to change the project or user identifier for a +# consumer by specifying a different project_id or user_id value in the payload +# of both a PUT /allocations/{consumer_uuid} or POST /allocations + +fixtures: + - APIFixture + +defaults: + request_headers: + x-auth-token: admin + accept: application/json + content-type: application/json + openstack-api-version: placement 1.28 + +tests: + +- name: create cn1 + POST: /resource_providers + data: + name: cn1 + status: 200 + +- name: add inventory + PUT: $HISTORY['create cn1'].$RESPONSE['links[?rel = "inventories"].href'] + data: + resource_provider_generation: 0 + inventories: + VCPU: + total: 16 + MEMORY_MB: + total: 2048 + +- name: create allocations for consumer1 + PUT: /allocations/11111111-1111-1111-1111-111111111111 + data: + allocations: + $HISTORY['create cn1'].$RESPONSE['uuid']: + resources: + MEMORY_MB: 1024 + VCPU: 2 + project_id: $ENVIRON['PROJECT_ID'] + user_id: $ENVIRON['USER_ID'] + consumer_generation: null + status: 204 + +- name: get allocations for consumer1 + GET: /allocations/11111111-1111-1111-1111-111111111111 + status: 200 + response_json_paths: + $.project_id: $ENVIRON['PROJECT_ID'] + $.user_id: $ENVIRON['USER_ID'] + +- name: change the project for consumer1 + PUT: /allocations/11111111-1111-1111-1111-111111111111 + data: + allocations: + $HISTORY['create cn1'].$RESPONSE['uuid']: + resources: + MEMORY_MB: 1024 + VCPU: 2 + project_id: $ENVIRON['PROJECT_ID_ALT'] + user_id: $ENVIRON['USER_ID'] + consumer_generation: 1 + status: 204 + +- name: check consumer1's project is now the other project + GET: /allocations/11111111-1111-1111-1111-111111111111 + status: 200 + response_json_paths: + $.project_id: $ENVIRON['PROJECT_ID_ALT'] + $.user_id: $ENVIRON['USER_ID'] + +- name: create allocations for two consumers + POST: /allocations + data: + 11111111-1111-1111-1111-111111111111: + allocations: + $HISTORY['create cn1'].$RESPONSE['uuid']: + resources: + MEMORY_MB: 1024 + VCPU: 1 + consumer_generation: 2 + # Change consumer1's project back to the original PROJECT_ID + project_id: $ENVIRON['PROJECT_ID'] + user_id: $ENVIRON['USER_ID'] + 22222222-2222-2222-2222-222222222222: + allocations: + $HISTORY['create cn1'].$RESPONSE['uuid']: + resources: + MEMORY_MB: 1024 + VCPU: 1 + consumer_generation: null + project_id: $ENVIRON['PROJECT_ID_ALT'] + user_id: $ENVIRON['USER_ID_ALT'] + status: 204 + +- name: check consumer1's project is back to the original project + GET: /allocations/11111111-1111-1111-1111-111111111111 + status: 200 + response_json_paths: + $.project_id: $ENVIRON['PROJECT_ID'] + $.user_id: $ENVIRON['USER_ID']