mgr/dashboard: exclude cloned-deleted RBD snaps

[epuertat]

Fixes: https://tracker.ceph.com/issues/65698

Contribution Guidelines

• To sign and title your commits, please refer to Submitting Patches to Ceph.

• If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

• When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

• Tracker (select at least one)
  • References tracker ticket
  • Very recent bug; references commit where it was introduced
  • New feature (ticket optional)
  • Doc update (no ticket needed)
  • Code cleanup (no ticket needed)
• Component impact
  • Affects Dashboard, opened tracker ticket
  • Affects Orchestrator, opened tracker ticket
  • No impact that needs to be tracked
• Documentation (select at least one)
  • Updates relevant documentation
  • No doc update is appropriate
• Tests (select at least one)
  • Includes unit test(s)
  • Includes integration test(s)
  • Includes bug reproducer
  • No tests

Show available Jenkins commands

• jenkins retest this please
• jenkins test classic perf
• jenkins test crimson perf
• jenkins test signed
• jenkins test make check
• jenkins test make check arm64
• jenkins test submodules
• jenkins test dashboard
• jenkins test dashboard cephadm
• jenkins test api
• jenkins test docs
• jenkins render docs
• jenkins test ceph-volume all
• jenkins test ceph-volume tox
• jenkins test windows
• jenkins test rook e2e

[epuertat]

Manually tested, and it works:

@idryomov I remember you said that, besides setting ceph osd set-require-min-compat-client mimic --yes-i-really-mean-it, there was another way to trigger this behaviour. But I couldn't find it: the RBD image format was v2 already, and I don't see any option in rbd snap create and rbd clone for this.

[idryomov]

Yesterday I said that

Hiding these snapshots such that there is no trace of them at all is probably not a good idea

but, going by the screenshot, this appears to do exactly that... How about:

• skipping img.is_protected_snap() call in this case, just like it's skipped in case of snapshot-based mirroring
• replacing img.set_snap(snap['name']) call with img.set_snap_by_id(snap['id'])

I'm wondering if img.list_children() would "just work" in that case -- displaying such a snapshot and its children is much better than hiding all of that.

[idryomov]

• skipping img.is_protected_snap() call in this case, just like it's skipped in case of snapshot-based mirroring

If I'm reading this right, snap['is_protected'] is left to be None in case snapshot-based mirroring. This could be improved -- since only regular user snapshots can be protected, I'd write it as follows:

snap['is_protected'] = False
if snap['namespace'] == rbd.RBD_SNAP_NAMESPACE_TYPE_USER:
    snap['is_protected'] = img.is_protected_snap(snap['name'])

This would cover all types of snapshots in a generic way.

[epuertat]

Hiding these snapshots such that there is no trace of them at all is probably not a good idea

Yup, @idryomov, I knew, but given this should be backported ASAP to all active releases I wanted:

• A minimal/portable change (no changes to the API),
• Something that matches the default output of rbd snap ls.

If I don't do this way, whenever a user deletes a snapshot with clones, they'll continue seeing that snapshot. I'd rather display the deleted snapshot in the Trash section:

But rbd trash only works for images, right?

BTW, (CC @nizamial09) IMHO the /api/block/{image} endpoint tries to do too many things (iterate through all snapshots and children, calculate the real Image usage with diff_iterate()), which results in a very heavyweight endpoint. We should probably add endpoints for the snapshots (/api/block/{image}/snapshot) and a new query param (minimal=<bool>) to retrieve only the basic RBD Image info.

[epuertat]

BTW @idryomov I'll apply this snippet you shared. Thanks!:

snap['is_protected'] = False
if snap['namespace'] == rbd.RBD_SNAP_NAMESPACE_TYPE_USER:
    snap['is_protected'] = img.is_protected_snap(snap['name'])

[idryomov]

Hiding these snapshots such that there is no trace of them at all is probably not a good idea

Yup, @idryomov, I knew, but given this should be backported ASAP to all active releases I wanted:

• A minimal/portable change (no changes to the API),

By the change to the API, are you referring to adding a parameter which would be driven by a toggle in the GUI corresponding to --all flag in the CLI?

• Something that matches the default output of rbd snap ls.

I'm not convinced that having rbd snap ls hide such snapshots by default was a good idea... So much so that I wouldn't suggest replicating it.

Also, note that the behavior with this PR as is wouldn't be equivalent to rbd snap ls output: rbd snap ls hides all but RBD_SNAP_NAMESPACE_TYPE_USER snapshots, whereas here only RBD_SNAP_NAMESPACE_TYPE_TRASH snapshots are hidden.

But rbd trash only works for images, right?

Yes -- despite the fact that the term "trash" is used in both cases, rbd trash in the Windows trash bin sense (meaning that items can be moved to the trash and later restored or removed for good) works only for images. I don't see how snapshots of an image that hasn't been moved to the trash could be displayed in the Trash section as it exists today.

[epuertat]

Thanks @idryomov! I tried implementing the listing of all snapshots, and .set_snap_by_id() worked ok, but it didn't work with the .diff_iterate() method. I'll leave it this way, in order to speed up this fix.

[idryomov]

@idryomov I remember you said that, besides setting ceph osd set-require-min-compat-client mimic --yes-i-really-mean-it, there was another way to trigger this behaviour.

The other way to do it globally is to set rbd_default_clone_format config option to 2.

There is also a way to do it on a per-clone basis, but it's currently not exposed in our Python bindings: https://tracker.ceph.com/issues/65624. I'll get that fixed ASAP.