Upgrade to Ussuri fails if deleted volumes exist prior to Train

[Mohammed Naser]

The following introduced going towards the __DEFAULT__ volume type:

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

This code also included an online database migration which updated all volumes and snapshots to the default volume_type. However, it does not send `read_deleted=yes` which means that all deleted volumes don't get migrated and then the follow-up migration in Ussuri which makes that field non-nullable:

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

always fails.. the online db migration should be fixed and backported.

[OpenStack Infra (hudson-openstack)]

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

[Brian Rosmaita (brian-rosmaita)]

Thanks for the detailed analysis.

This is complicated by another bug, namely that the code in question shouldn't be present in master and Ussuri (Bug #1893114). But:

(1) We definitely need mnaser's fix in Train, so that the migration is performed correctly the first time for people just now upgrading to Train.

(2) We also need mnaser's fix in Ussuri, because that online migration needs to be run again and catch any untyped deleted volumes/snapshots before the Ussuri migration making the column non-nullable is applied. So we should *not* remove the online migration from Ussuri.

(3) Since the column is non-nullable in Ussuri, there can be no untyped volumes in a Ussuri installation (if it was an upgrade from T, either they didn't have any deleted untyped volumes/snapshots, or they encountered this bug and couldn't do the upgrade to U). Hence we don't need the untyped volume/snapshot online migrations in master (Victoria) and they don't need to be fixed there.

[Mohammed Naser (mnaser)]

IMHO the best thing would be to land my fix, backport to stable/train, and then land fix for Bug #1893114 and backport the 'removal' in master

The online data migrations are fairly noop so they can probably live in Ussuri happily (and would give the operator a nice way of 'bailing out' on an upgrade by running online data migrations in Ussuri to fix the db sync)

[Brian Rosmaita (brian-rosmaita)]

Sorry about the confusion. What I'd like to do is:

- don't put this into master (the code is being removed and master is unreleased).
- do put it into Ussuri, because it is relevant there
- backport from ussuri -> train

I finally figured out how to write tests that will work in both Ussuri and Train; they're attached to this bug as a patch. You can add them to your Ussuri patch.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/748481

[OpenStack Infra (hudson-openstack)]

Change abandoned by Brian Rosmaita (<email address hidden>) on branch: master
Review: https://review.opendev.org/748276
Reason: This is superseded by https://review.opendev.org/#/c/748300/ for master and https://review.opendev.org/748481 for ussuri.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/train
Review: https://review.opendev.org/748482

[Brian Rosmaita (brian-rosmaita)]

I put a hold on the ussuri patch and the backport to train. I think we're going to have to take a slightly different approach and propose the train patch directly to train (no backport) and fix this differently in ussuri.

Analysis and proposal for fixing: https://etherpad.opendev.org/p/ussuri-upgrade-issue

Will discuss with the Cinder team at tomorrow's (2 September) weekly meeting.

[Brian Rosmaita (brian-rosmaita)]

After considering some alternatives, the original code changes to ussuri and train look like the best way to do this. Enhanced the release notes to explain how to handle a corner case.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/748481
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=f29e884b2469e360b891e7168a1372bdaa110fcc
Submitter: Zuul
Branch: stable/ussuri

commit f29e884b2469e360b891e7168a1372bdaa110fcc
Author: Brian Rosmaita <email address hidden>
Date: Wed Aug 26 22:00:12 2020 -0400

    Include deleted volumes for online data migrations

    The two online data migrations which moved volumes to the __DEFAULT__
    volume type did not include deleted volumes, which meant that the follow
    up migration which makes it a nonnullable value does not work because
    the deleted volumes will continue to have `null` as their
    volume_type_id.

    This patch fixes that by including deleted volumes when running the
    online database migration. Also adds tests.

    (Patch is proposed directly to stable/ussuri because the online migration
    code is removed from master by change
    I78681ea89762790f544178725999903a41d75ad1)

    Co-authored-by: Mohammed Naser <email address hidden>

    Change-Id: I3dc5ab78266dd895829e827066f78c6bebf67d0d
    Closes-Bug: #1893107

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/748482
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=00ac80bbadeaba9c42d2453d49410c48269a2e15
Submitter: Zuul
Branch: stable/train

commit 00ac80bbadeaba9c42d2453d49410c48269a2e15
Author: Brian Rosmaita <email address hidden>
Date: Wed Aug 26 22:00:12 2020 -0400

    Include deleted volumes for online data migrations

    The two online data migrations which moved volumes to the __DEFAULT__
    volume type did not include deleted volumes, which meant that the follow
    up migration which makes it a nonnullable value does not work because
    the deleted volumes will continue to have `null` as their
    volume_type_id.

    This patch fixes that by including deleted volumes when running the
    online database migration. Also adds tests.

    (Patch is proposed directly to stable/ussuri because the online migration
    code is removed from master by change
    I78681ea89762790f544178725999903a41d75ad1)

    Co-authored-by: Mohammed Naser <email address hidden>

    Change-Id: I3dc5ab78266dd895829e827066f78c6bebf67d0d
    Closes-Bug: #1893107
    (cherry picked from commit 2440cb66e31fe54b2624550e2ac636a4b9a674fe)
    conflicts:
    - replaced the Ussuri release note with one specific to Train