Do not set allocation.id in AllocationList.create_all()
The _set_allocations method used by AllocationList.create_all is side-effecty: it sets the 'id' attribute on the list of Allocation objects that is passed to it. At the start of the method the incoming Allocation objects are checked to see if they have already been created, by checking for an 'id' field. Meanwhile, _set_allocations is also configured to retry on db deadlock. The deadlock can happen for a variety of reasons within the transaction. The original theory, discussed in the original fix, I2c276dc0125b5b9f7a54a1cd431b1b2f5239e93a, is that it is during resource provider generation checks. In the associated bug it looks like it may happen sometimes while inserting allocations. In either case, if we have gone through the 'for alloc in allocs' loop at least once, the contents of the 'allocs' list has been modified to have at least one of the alloc.id fields set. Upon retry, the 'id' field check at the start of the method will fail, leading to an ObjectActionError and an eventual 500 at the placement API level. This change takes the simplest approach and simply removes the setting of the 'id' attribute on the allocations in the 'allocs' list. There are other ways to deal with this, this is the least intrusive. It works because: * create_all is only called from the allocation handler in placement, and the objects are not used (the response bodies are empty) * other than the 'id' change, the alloc members in the allocs list are otherwise unchanged * this kind of side-effecty business is dangerous, so let's not rely on it Tests which were relying on the side-effecty business have been adjusted accordingly. Change-Id: I3c7aea7d8959a20c3c404bc6616b47336ff40b67 Closes-Bug: #1739453
This commit is contained in:
parent
609ddc2244
commit
3491f3d6f2
@ -2082,8 +2082,7 @@ class AllocationList(base.ObjectListBase, base.NovaObject):
|
||||
resource_class_id=rc_id,
|
||||
consumer_id=consumer_id,
|
||||
used=alloc.used)
|
||||
result = context.session.execute(ins_stmt)
|
||||
alloc.id = result.lastrowid
|
||||
context.session.execute(ins_stmt)
|
||||
|
||||
# Generation checking happens here. If the inventory for this resource
|
||||
# provider changed out from under us, this will raise a
|
||||
|
@ -1217,7 +1217,6 @@ class TestAllocation(ResourceProviderBaseCase):
|
||||
disk_allocation.used)
|
||||
self.assertEqual(DISK_ALLOCATION['consumer_id'],
|
||||
disk_allocation.consumer_id)
|
||||
self.assertIsInstance(disk_allocation.id, int)
|
||||
|
||||
allocations = rp_obj.AllocationList.get_all_by_resource_provider(
|
||||
self.ctx, resource_provider)
|
||||
|
@ -352,9 +352,13 @@ class TestAllocation(test_objects._LocalTest):
|
||||
used=8)
|
||||
alloc_list = resource_provider.AllocationList(self.context,
|
||||
objects=[obj])
|
||||
self.assertNotIn("id", obj)
|
||||
alloc_list.create_all()
|
||||
self.assertIn("id", obj)
|
||||
|
||||
rp_al = resource_provider.AllocationList
|
||||
saved_allocations = rp_al.get_all_by_resource_provider(
|
||||
self.context, rp)
|
||||
self.assertEqual(1, len(saved_allocations))
|
||||
self.assertEqual(obj.used, saved_allocations[0].used)
|
||||
|
||||
def test_create_with_id_fails(self):
|
||||
rp = resource_provider.ResourceProvider(context=self.context,
|
||||
|
Loading…
Reference in New Issue
Block a user