Reviewed: https://review.openstack.org/582382 Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=11c29ae4702ac4f50baa9fe3a29e3e413ad69b09 Submitter: Zuul Branch: master
commit 11c29ae4702ac4f50baa9fe3a29e3e413ad69b09 Author: Jay Pipes <[email protected]> Date: Thu Jul 12 14:57:35 2018 -0400 do not assume 1 consumer in AllocList.delete_all() Ever since we introduced support for setting multiple consumers in a single POST /allocations, the AllocationList.delete_all() method has been housing a latent bad assumption and bug. The AllocationList.delete_all() method used to assume that the AllocationList's Allocation objects were only ever for a single consumer, and took a shortcut in deleting the allocation by deleting all allocations with the "first" Allocation's consumer UUID: ```python def delete_all(self): # Allocations can only have a single consumer, so take advantage of # that fact and do an efficient batch delete consumer_uuid = self.objects[0].consumer.uuid _delete_allocations_for_consumer(self._context, consumer_uuid) consumer_obj.delete_consumers_if_no_allocations( self._context, [consumer_uuid]) ``` The problem with the above is that if you get all the allocations for a single resource provider, using AllocationList.get_all_by_resource_provider() and there are more than one consumer allocating resources against that provider, then calling AllocationList.delete_all() will only delete *some* of the resource provider's allocations, not all of them. Luckily, the handler code has never used AllocationList.delete_all() after calling AllocationList.get_all_by_resource_provider(), and so we've not hit this latent bug in production. However, in the next patch in this series (the reshaper DB work), we *do* call AllocationList.delete_all() for allocation lists for each provider involved in the reshape operation, which is why this fix is important to get done correctly. Note that this patch renames AllocationList.create_all() to AllocationList.replace_all() to make it absolutely clear that all of the allocations for all consumers in the list are first *deleted* by the codebase and then re-created. We also remove the check in AllocationList.create_all() that the Allocation objects in the list must not have an 'id' field set. The reason for that is because in order to properly implement AllocationList.delete_all() to call DELETE FROM allocations WHERE id IN (<...>) we need the list of allocation record internal IDs. These id field values are now properly set on the Allocation objects when AllocationList.get_all_by_resource_provider() and AllocationList.get_all_by_consumer_id() are called. This allows that returned object to have delete_all() called on it and the DELETE statement to work properly. Change-Id: I12393b033054683bcc3e6f20da14e6243b4d5577 Closes-bug: #1781430 ** Changed in: nova Status: In Progress => Fix Released -- You received this bug notification because you are a member of Yahoo! Engineering Team, which is subscribed to OpenStack Compute (nova). https://bugs.launchpad.net/bugs/1781430 Title: AllocationList.delete_all() incorrectly assumes a single consumer Status in OpenStack Compute (nova): Fix Released Bug description: AllocationList.delete_all() looks like this: ``` def delete_all(self): # Allocations can only have a single consumer, so take advantage of # that fact and do an efficient batch delete consumer_uuid = self.objects[0].consumer.uuid _delete_allocations_for_consumer(self._context, consumer_uuid) consumer_obj.delete_consumers_if_no_allocations( self._context, [consumer_uuid]) ``` the problem with the above is that it is based on an old assumption: that a list of allocations will only ever involve a single consumer. This hasn't been the case ever since we introduced the ability to do `POST /allocations` which was 1.12 or 1.13 IIRC. The safety concern about the above is that if someone in code does this: ``` allocs = AllocationList.get_all_by_resource_provider(self.ctx, compute_node_provider) allocs.delete_all() ``` they would reasonable expect all of the allocations for a provider to be deleted. However, this is not the case. Only the allocations against the "first" consumer will be deleted. The fix is simple... check to see if there are >1 consumers in the allocation list's objects and if so, don't call _delete_allocations_for_consumer(). Instead, call _delete_allocations_by_id() and do a DELETE FROM allocations WHERE id IN (...). To manage notifications about this bug go to: https://bugs.launchpad.net/nova/+bug/1781430/+subscriptions -- Mailing list: https://launchpad.net/~yahoo-eng-team Post to : [email protected] Unsubscribe : https://launchpad.net/~yahoo-eng-team More help : https://help.launchpad.net/ListHelp
