RBD: Unable to delete a volume which has snapshot/volume children

[Eric Harney]

[Impact]
Deleting a volume will fail if it has snapshot or volume children, resulting in an ImageBusy error.

[Fix]
Upstream has a patch that uses RBD flatten operations to break dependencies between volumes and snapshots, reducing failures when using RBD volume clones and snapshots.

commit 1a675c9aa178c6d9c6ed10fd98f086c46d350d3f
Author: Eric Harney <email address hidden>
CommitDate: Fri Dec 1 10:17:05 2023 -0500

    RBD: Flattening of child volumes during deletion

[Test Plan]
Here are the common steps for testing.
1. Prepare an OpenStack environment with cinder-ceph
2. Create a volume named "vol"
openstack volume create --image jammy --size 10 vol
3. Create a snapshot of the volume "vol"
openstack volume snapshot create --volume vol vol-snap
4. Create a volume named "vol-copy" from the snapshot
openstack volume create --snapshot vol-snap vol-copy
5. Delete the snapshot and then delete the volume "vol"
openstack volume snapshot delete vol-snap
openstack volume delete vol
6. Confirm that the volume "vol" is successfully deleted
openstack volume list

There are two test scenarios:
- Enable RBD flatten operations:
1. Use `juju ssh` to log in to the cinder-volume and add "enable_flatten_children_deletion = True" in the cinder-ceph section of /etc/cinder/cinder.conf.
2. Restart the cinder-volume service using the command `systemctl restart cinder-volume`.
3. Follow the common steps, and all steps should pass.
- Disable RBD flatten operations:
1. Ensure that "enable_flatten_children_deletion = True" is not present in the cinder-ceph section of /etc/cinder/cinder.conf.
2. Follow the common steps, and the process should fail at step 5 with an ImageBusy error.

[Where problems could occur]
The patch primarily modifies the workflow for volume deletion when using RBD as the backend and adds a retry mechanism for unprotecting snapshots during snapshot deletion.
To prevent any performance regressions, we have also introduced a configuration option to enable the RBD flatten operations, which defaults to false.
If the patch has any undiscovered issues, it will only affect volume deletion. Other functionalities or non-RBD backends will not be impacted.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/cinder/+/838756

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/cinder/+/838757

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Eric Harney <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/cinder/+/838756
Reason: https://review.opendev.org/c/openstack/cinder/+/835384

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Eric Harney <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/cinder/+/838757
Reason: https://review.opendev.org/c/openstack/cinder/+/835384

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/cinder/+/835384
Committed: https://opendev.org/openstack/cinder/commit/1a675c9aa178c6d9c6ed10fd98f086c46d350d3f
Submitter: "Zuul (22348)"
Branch: master

commit 1a675c9aa178c6d9c6ed10fd98f086c46d350d3f
Author: Eric Harney <email address hidden>
Date: Wed Apr 20 12:08:31 2022 -0400

    RBD: Flattening of child volumes during deletion

    This patch allows delete_volume and delete_snapshot calls
    to fail less often when using RBD volume clones and snapshots.

    RBD clone v2 support allows remove() to pass in situations
    where it would previously fail, but it still fails with an
    ImageBusy error in some situations. For example:

     volume1
       -> snapshot s1 of volume 1
         -> volume2 cloned from snapshot 1
    Deleting snapshot s1 would fail with ImageBusy.

    This is fixed by using RBD flatten operations to break
    dependencies between volumes/snapshots and other RBD volumes
    or snapshots.

    Delete now works as follows:
      1. Attempt RBD remove()
         This is the "fast path" for removing a simple volume
         that involves no extra overhead.
      2. If busy and the volume has child dependencies,
         flatten those dependencies with RBD flatten()
      3. Attempt RBD remove() again
         This will succeed in more cases than (1) would have.
      4. If remove() failed, use trash_move() instead to move
         the image to the trash instead.
         This allows Cinder deletion of a volume (volume1) to proceed
         in the scenario where volume2 was cloned from snapshot s1 of
         volume1, and snapshot s1 has been trashed and not fully
         deleted from the RBD backend. (Snapshots in the trash
         namespace are no longer visible but are still in the
         dependency chain.)

    This allows Cinder deletions to succeed in most scenarios where
    they would previously fail.

    In cases where a .clone_snap snapshot is present, we still do a
    rename to .deleted instead of deleting/trashing the volume. This
    should be worked on further in a follow-up as it is likely not
    necessary most of the time.

    A new configuration option, rbd_concurrent_flatten_operations, was
    introduced to limit how many flatten calls can be made at the same time.
    This is to prevent overloading the backend. The default value is 3.

    Co-Authored-By: Eric Harney <email address hidden>
    Co-Authored-By: Sofia Enriquez <email address hidden>

    Closes-Bug: #1969643
    Change-Id: I009d0748fdc829ca0b4f99bc9b70dadd19717d04

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/cinder-tempest-plugin/+/823719
Committed: https://opendev.org/openstack/cinder-tempest-plugin/commit/861993b5e9565a988a34087305dc5617a5693e4c
Submitter: "Zuul (22348)"
Branch: master

commit 861993b5e9565a988a34087305dc5617a5693e4c
Author: Sofia Enriquez <email address hidden>
Date: Thu Jan 6 17:00:55 2022 +0000

    Deleting volumes which are consumed as a basis for others

    This patch includes two test scenarios:

    - Create a cloned volume from another volume. Cinder should
    be able to delete the source volume.

    - Create a volume from a snapshot. Cinder should be able to
    delete the source volumes while the new volume still exists.

    Related-Bug: #1969643
    Depends-On: https://review.opendev.org/c/openstack/cinder/+/848013
    Change-Id: I8daed0824f5ac60877e839265c09217dcce6dd21

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/cinder 24.0.0.0rc1 release candidate.

[Chengen Du (chengendu)]

Debdiff for Jammy

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "lp1969643-cinder-jammy.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

[Mauricio Faria de Oliveira (mfo)]

Hi Chengen,

Thanks for your work on this bug / fix / SRU template!

Some process review notes:

In the future, please update the bug tasks for the newer/devel releases as well.

In this case, Mantic not marked as Won't Fix possibly would require you an extra round-trip during the review with the SRU team later, to clarify why it is affected but not being fixed.

I think this is because Mantic will EOL in less than a month, thus not worth fixing.
This type of information is helpful to add in the SRU template's Other Info section [1].

Please also see See SRU -> General Requirements > Newer Releases > Exceptions > "2." [2]

$ ubuntu-distro-info --series=mantic --days=eol
29

$ rmadison -a source cinder | grep -e mantic -e noble -e oracular
 cinder | 2:23.0.0-0ubuntu1 | mantic | source
 cinder | 2:23.0.0-0ubuntu1.2 | mantic-updates | source
 cinder | 2:24.0.0-0ubuntu1 | noble | source
 cinder | 2:24.0.0-0ubuntu1 | oracular | source
 cinder | 2:24.0.0-0ubuntu2 | oracular-proposed | source

$ pull-lp-source cinder noble
Found cinder 2:23.0.0-0ubuntu1.2 in mantic
...
$ grep -A2 'deployments must either' cinder-23.0.0/releasenotes/notes/rbd-flatten-child-volumes-4cb0b7fcf3a1df5e.yaml
grep: cinder-23.0.0/releasenotes/notes/rbd-flatten-child-volumes-4cb0b7fcf3a1df5e.yaml: No such file or directory

$ pull-lp-source cinder noble
Found cinder 2:24.0.0-0ubuntu1 in noble
...
$ grep -A2 'deployments must either' cinder-24.0.0/releasenotes/notes/rbd-flatten-child-volumes-4cb0b7fcf3a1df5e.yaml
    Therefore, deployments must either a) enable scheduled RBD trash purging on
    the RBD backend or b) enable the Cinder RBD driver's enable_deferred_deletion
    option to have Cinder purge the RBD trash.

[1] https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template
[2] https://wiki.ubuntu.com/StableReleaseUpdates#Newer_Releases

[Mauricio Faria de Oliveira (mfo)]

Chengen,

Now the changes review notes:

This patch is concerning for a couple of reasons, if I understand correctly:
1) behavior changes that are potentially impactful to the storage backend;
2) new requirements for the deployment configuration that might seriously
   impact the deployment.

In order to proceed, could you please discuss/confirm these with Ed (hopem),
Dan (hillpd), and if they both agree, get an ACK from James Page (james-page),
and record their feedback here?

I'll mark the bug task as Incomplete for now.

Thanks!
Mauricio

Details:

1) The behavior change (flatten dependent images on volume removal) seems
to introduce O(N) load in the cluster in storage size/consumption _and_
CPU load, as each dependent image will be flattened thus the data from
the volume being removed will be copied into _each one_, which multiplies
the storage consumption with a single delete operation.

Without the patch, this would not happen, as the operation would fail.

@ https://docs.ceph.com/en/latest/rbd/rbd-snapshot/#flattening-a-cloned-image

When you remove the reference to the parent snapshot from the clone,
you effectively “flatten” the clone by copying the data stored
in the snapshot to the clone.
The time it takes to flatten a clone increases with the size of the snapshot.

@ https://docs.ceph.com/en/latest/man/8/rbd/

If the image is a clone, copy all shared blocks from the parent snapshot [...]

@ patch

++ cfg.IntOpt('rbd_concurrent_flatten_operations', default=3, min=0,
++ help='Number of flatten operations that will run '
++ 'concurrently on this volume service.')

2) The patch doc/note says that the deployments now must enable deletion options.

It seems potentially problematic if this is missed, specially with auto upgrades,
as, if such option is not enabled, this could eventually consume a lot of storage,
and not be removed until that is noticed (when it might be too late) ?

++ Cinder now uses the RBD trash functionality to handle some volume deletions.
++ Therefore, deployments must either a) enable scheduled RBD trash purging on
++ the RBD backend or b) enable the Cinder RBD driver's enable_deferred_deletion
++ option to have Cinder purge the RBD trash.

[Edward Hope-Morley (hopem)]

Thanks @mfo i think your two concerns are valid. This patch does create the risk that a user, in deleting a single volume, could overwhelm the Ceph cluster. It is not uncommon for production Ceph clusters to be reasonably full and with this patch if you happen to delete a large volume that has a large number of clones it could tip your cluster usage over the edge without any warning. I feel somewhat that what the patch is addressing is more an issue of convenience than a bug and while it is nice to be leveraging the trash feature of rbd v2, I think that to make this patch really same some guardrails should be put in place. I would recommend at least having a (configurable) limit to the number of clones a volume is allowed to have (and perhaps taking into account size) in order for auto-flattening to be allowed.

To Mauricio's second point, given that in order to implement this properly you need to also consider how and when your trashed images are deleted, it might have been safer to make this an optional feature, defaulting to disabled so that the necessary pre-requisites/changes can be put in place before it is enabled.

I am therefore not hugely in favour of backporting this patch as-is and perhaps if we can focus on addressin some of the safety concerns raised we could reconsider it later.

[Mauricio Faria de Oliveira (mfo)]

Hi Ed and Chengen,

Ed, thanks for your review and feedback.

> I think that to make this patch really same some guardrails should be put in place.

> it might have been safer to make this an optional feature, defaulting to disabled

This also aligns with Dan's feedback (internal):

> there are significant changes in behavior that are not protected by an opt-in flag.
> In fact, there is no opt-out mechanism either.

So, probably the next steps would be to modify the patch to include an opt-in flag,
and make the behavior change disabled by default.

Chengen, is this something you could look at?

The quantity and structure of the code changes in this patch might make this more
difficult and require higher-touch, but hopefully it should be possible -- and if
you would like any assistance, please let me know.

You can find a more trivial example at [1].

Thanks!
Mauricio

[1] https://launchpadlibrarian.net/688200814/nova_2%3A21.2.4-0ubuntu2.5_2%3A21.2.4-0ubuntu2.6.diff.gz

[Chengen Du (chengendu)]

@mfo I have introduced a configuration option, 'enable_flatten_children_deletion', to enable RBD flatten operations during delete_volume/delete_snapshot calls (default is False). Please take a look and proceed with the SRU process. Thanks for your continued support.

[Mauricio Faria de Oliveira (mfo)]

Hi Chengen,

Thanks for implementing this!

Could you please confirm/describe the testing
performed with this additional patch/option?

[Chengen Du (chengendu)]

@mfo I apologize for the haste earlier and have now modified the test plan.