Comment 6 for bug 1739453

Reviewed: https://review.openstack.org/529397
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=3491f3d6f2334b80c364deeb21d65004262c2846
Submitter: Zuul
Branch: master

commit 3491f3d6f2334b80c364deeb21d65004262c2846
Author: Chris Dent <email address hidden>
Date: Wed Dec 20 18:35:53 2017 +0000

    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