Reverting migration-based allocations leaks allocations if the server is deleted

[Matt Riedemann]

This came up in the cross-cell resize review:

https://review.opendev.org/#/c/627890/60/nova/conductor/tasks/cross_cell_migrate.py@495

And I was able to recreate with a functional test here:

https://review.opendev.org/#/c/688832/

That test is doing a cross-cell cold migration but looking at the code:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/conductor/tasks/migrate.py#L461

We can hit an issue for same-cell resize/cold migrate if we have swapped the allocations so the source node allocations are held by the migration consumer and the instance holds allocations on the target node (created by the scheduler):

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/conductor/tasks/migrate.py#L328

If something fails between ^ and the cast to prep_resize, the task will rollback and revert the allocations so the target node allocations are dropped and the source node allocations are moved back to the instance:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/conductor/tasks/migrate.py#L91

Furthermore, if the instance was deleted when we perform that swap, the move_allocations method will recreate the allocations on the source node for the now-deleted instance since we don't assert consumer generations during the swap:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/scheduler/client/report.py#L1886

This results in leaking allocations for the source node since the instance is deleted.

[Matt Riedemann (mriedem)]

Note that we could have the same issue in the compute service, for example if the server is deleted during the resize claim and we get to this exception block handler:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/compute/manager.py#L4724

To revert the allocations here:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/compute/manager.py#L4574

Which calls move_allocations which has the same problem described above.

We could also have the same issue if the instance is gone during resize_instance on the source host:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/compute/manager.py#L4896

Or during finish_revert_resize I guess:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/compute/manager.py#L4454

I'm not sure about the fix yet, but we might want callers to optionally tell the move_allocations method if it should require the usage of the target consumer (instance in this case) generation so we don't use a .get() here:

https://github.com/openstack/nova/blob/1a226aaa9e8c969ddfdfe198c36f7966b1f692f3/nova/scheduler/client/report.py#L1886

Actually if the target consumer no longer exists in placement the 'consumer_generation' key won't exist in that allocations response and we'd have to handle it earlier, like this:

https://review.opendev.org/#/c/688832/2/nova/scheduler/client/report.py

[Matt Riedemann (mriedem)]

I think we can claim this as a regression going back to migration-based allocations added in Queens. Looking at where a resize would fail during prep_resize in Pike, we call this method to remove the 'doubled up' allocations or the instance:

https://github.com/openstack/nova/blob/stable/pike/nova/scheduler/client/report.py#L1028

That's to remove the dest host provider from the allocations created by the scheduler.

In this case if the instance was deleted concurrently and had no allocations, we'd get here and return False:

https://github.com/openstack/nova/blob/stable/pike/nova/scheduler/client/report.py#L1062

And we'd log an error there and here in the resource tracker:

https://github.com/openstack/nova/blob/stable/pike/nova/compute/resource_tracker.py#L1320

But we wouldn't leak any allocations for the deleted instance which is the bug we have since Queens when reverting the allocation swap and the instance is gone.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/688980

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/688994

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/689013

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/689049

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/688980
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=24318f8cd4b71f1072c6f194a6f1e04a854acb06
Submitter: Zuul
Branch: master

commit 24318f8cd4b71f1072c6f194a6f1e04a854acb06
Author: Matt Riedemann <email address hidden>
Date: Wed Oct 16 13:08:08 2019 -0400

    Add functional recreate test for bug 1848343

    This adds a functional test to recreate a bug where the
    instance is deleted after conductor has swapped allocations
    to the migration consumer but before casting to compute. In
    this case, the scheduler fails due to NoValidHost which is
    entirely reasonable. The bug is that the conductor task rollback
    code re-creates the allocations on the source node for the
    now-deleted instance and as such those allocations get leaked.

    Note that we have similar exposures in the live migration
    task and reverting allocations when resize fails in the
    compute service. A TODO is left inline to add tests for those
    separately.

    Change-Id: Ifd156ac8789d3fc84d56d400cf1e160e2cd2fbee
    Related-Bug: #1848343

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/688994
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=252ee93086a4854c5a5922eec826ffb908f9b283
Submitter: Zuul
Branch: master

commit 252ee93086a4854c5a5922eec826ffb908f9b283
Author: Matt Riedemann <email address hidden>
Date: Wed Oct 16 14:03:59 2019 -0400

    Add live migration recreate test for bug 1848343

    This adds a live migration functional recreate test
    like Ifd156ac8789d3fc84d56d400cf1e160e2cd2fbee is
    for cold migrate/resize.

    Change-Id: I856db36d63779d521fe26b27ef5a12b7a4d3bd91
    Related-Bug: #1848343

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/689013
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=760ccb32dbbb0baf40ce4ea01977f9886d0d3a3b
Submitter: Zuul
Branch: master

commit 760ccb32dbbb0baf40ce4ea01977f9886d0d3a3b
Author: Matt Riedemann <email address hidden>
Date: Wed Oct 16 15:23:50 2019 -0400

    Add compute side revert allocation test for bug 1848343

    This adds a functional recreate test for a scenario where
    reverting migration-based allocations during resize failure
    in the compute service results in leaking allocations for a
    deleted server.

    Change-Id: Iac4dd9feebb1a405826c95cb6b046b82c61140a2
    Related-Bug: #1848343

[OpenStack Infra (hudson-openstack)]

Change abandoned by Matt Riedemann (<email address hidden>) on branch: master
Review: https://review.opendev.org/689049
Reason: I'm not working on this.