VolumeSnapshotRef.UID should be cleared on volumesnapshot deletion

[ashish-amarnath]

User story

As a user of CSI volumesnapshot APIs, I would like to be able to reuse existing volumesnapshotcontents to provision new volumes pre-populated data when using DeletionPolicy: Retain on the volumesnapshotcontent.

Detailed Description

When using DeletionPolicy: Retain on the volumesnapshotcontent, deletion of volumesnapshot objects will leave a dangling reference to the deleted volumesnapshot object, preventing new volumesnapshots, with the same name in the same namespace, to attach back to the volumesnapshotcontent.

Proposal

When a volumesnapshot is deleted, and the DeletionPolicy on the underlying volumesnapshotcontent is Retain, the deletion should cascade back to the volumesnapshotcontent to clear the volumesnapshotcontent's Spec.VolumeSnapshotRef.UID to allow it to be reused by a new volumesnapshot object with the same name and in the same namespace.

It should also be considered to update the Source of the volumesnapshot content by:
1. Clearing volumesnapshotcontent.Spec.Source.volumehandle to empty (nil or "")
1. Setting volumesnapshotcontent.Spec.Source.snapshothandle to the volumesnapshotcontent.Status.snapshotHandle

cc @xing-yang

[xing-yang]

Use case: When the whole namespace is deleted by accident, we want to be able to restore from VolumeSnapshotContent with the associated physical snapshot resource.

[xing-yang]

Discussed at today's Volume Snapshot WG meeting.

• Have the snapshot controller clear up VolumeSnapshotRef.UID may be reasonable, but need to think about potential race conditions.

• Clearing up volumesnapshotcontent.Spec.Source.volumehandle and setting volumesnapshotcontent.Spec.Source.snapshothandle to the volumesnapshotcontent.Status.snapshotHandleset are not the responsibility of the snapshot controller. It is up to the higher level controller for backup and restore to do it if there is such a need.

[xing-yang]

This can be discussed as part of the Data Protection workflow.

[yuxiangqian]

"When using DeletionPolicy: Retain on the volumesnapshotcontent, deletion of VolumeSnapshot objects will leave a dangling reference to the deleted VolumeSnapshot object, preventing new VolumeSnapshots, with the same name in the same namespace, to attach back to the volumesnapshotcontent."
@ashish-amarnath I am not sure this is accurate.
Setting Retain as the DeletionPolicy implies the following: when Retain is the deletion policy of a VolumeSnapshot, deletion of a VolumeSnapshot will NOT trigger deletion of its bound VolumeSnapshotContent automatically, the content object will be dangling in the cluster. If user manually delete a dangling volume snapshot content with retain deletion policy, the snapshot sidecar will NOT delete the physical snapshot on underlying storage system, however the content object will be removed from API server.

It does not mean that you can no longer use the physical snapshot on underly storage system as long as you still have access to the snapshot handle, a VolumeSnapshot can be created to statically bind to a VolumeSnapshotContent which points back to it as well.

In current design, rebinding is NOT allowed for good reasons.
In dynamic snapshot case, a VolumeSnapshotContent will be created by the common controller following a naming scheme, and the VolumeSnapshot's UID is part of it. This is the make sure the common controller NOT to recreate snapshots if it crashed, but rather using the consistent naming scheme to identify existing VolumeSnapshotContents in API server. In this case, unset "UID" field in VolumeSnapshotContent will break the naming scheme.

w.r.t the proposal to update VolumeSnapshotContent's source fields, it's equivalent to transferring the content object from a dynamically created one to a static one. IMO, it's should be a user operation rather than controller's responsibility which could bring un-desired behavior of the system. The original PVC information from which the snapshot has been taken from will be lost as well.

LMK if you have any questions.

[ashish-amarnath]

@yuxiangqian I agree that it is a little over reaching to update the volumesnapshotcontent's source fields and I've scratched that from the description.

But on the VolumeSnapshotRef.UID side, not sure what the inaccuracy is. I am saying that if the DeletionPolicy is retain, then the volumesnapshotcontent is retained even though the volumesnapshot is deleted.
In the retained volumesnapshotcontent, however, still has a reference to the UID of the volumesnapshot that was deleted. This prevents from this volumesnapshotcontent from being reused to provision new volumes.

Is reusing volumesnapshotcontent objects to pre-populate newly created volumes, not a supported scenario? Is this what you are referring to when you say "dynamic snapshot"?

[yuxiangqian]

@ashish-amarnath The UID field is preserved on purpose ATM to not allow rebinding. It could certainly be revisited.
"Is reusing volumesnapshotcontent objects to pre-populate newly created volumes, not a supported scenario?" I meant once a VolumeSnapshotContent has been bound to a VolumeSnapshot, it's no longer bindable even if it's corresponding VolumeSnapshot has been removed from the system. One can certainly use a pre-existed snapshot to populate a volume utilizing the static snapshot functionality.

When I say dynamic snapshot, I meant when a user creates a VolumeSnapshot object with its source specified to a PVC. That asks snapshot controllers to invoke CSI "CreateSnapshot" call on-the-fly.

Maybe I was not clear enough, when I say its not accurate I explicitly referred to "preventing new VolumeSnapshots, with the same name in the same namespace, to attach back to the volumesnapshotcontent." and I meant a user can still refer to the underlying volume snapshot by using statically creation of a NEW VolumeSnapshotContent. ATM, as you suggested, reusing of an existing content is not supported.

[ashish-amarnath]

@yuxiangqian Thanks for the clarifications.
I think we would want to revisit the preservation of the UID in the volumesnapshot ref.

About

I meant once a VolumeSnapshotContent has been bound to a VolumeSnapshot, it's no longer bindable even if it's corresponding VolumeSnapshot has been removed from the system.

I am curious to know why we want to disallow volumesnapshotcontents to be made re-bindable, by removal of the UID in the volumesnapshotref, iff the associated volumesnapshot is deleted.
Adding clarifiers my previous question:
Is reusing volumesnapshotcontent objects, whose associated volumesnapshot objects are deleted, to pre-populate newly created volumes, not a supported scenario?

[yuxiangqian]

@ashish-amarnath As I stated previously:
In current design, rebinding is NOT allowed for good reasons.
In dynamic snapshot case, a VolumeSnapshotContent will be created by the common controller following a naming scheme, and the VolumeSnapshot's UID is part of it. This is the make sure the common controller NOT to recreate snapshots if it crashed, but rather using the consistent naming scheme to identify existing VolumeSnapshotContents in API server. In this case, unset "UID" field in VolumeSnapshotContent will break the naming scheme.

on top of that, one of the main things considered during the API design is to have well-defined and separated workflow for dynamically creating a snapshot from a PVC v.s. statically binding to an existing snapshot on storage system. Allowing rebinding of a dynamically created VolumeSnapshotContent breaks that semantic.

[ashish-amarnath]

Are those reasons documented some where I can refer to?
Also curious to know if the static binding flow documented anywhere?

[yuxiangqian]

this blog should give you an overview about it. The blog uses "pre-provisioned" instead of "static".

[nrb]

@yuxiangqian Thanks for that. The blog post seems to be the mechanics, but not the reasoning.

I'm still a little confused, though - if the namespace is entirely deleted, and a VolumeSnapshotContents is bound via UID, how would a user be able to use the snapshot at all? Create a new, statically bound VSC for a new VolumeSnapshot?

Relatedly, what would you envision is the right path for tools generating CSI objects to do in this scenario? Creating the statically bound objects?

Right now, here's what Velero does when creating a backup using our CSI implementation:

• When encountering a CSI-backed PV, create a (dynamic) VolumeSnapshot from it.
• Label the resulting VolumeSnapshot and VolumeSnapshotContents with the Velero backup name so we can track them as part of the Velero backup
• Store the CSI objects in the Velero backup tarball
• On restore, create the CSI objects first, editing them to appear as if they were the statically provisioned case:
  • VolumeSnapshotContents.VolumeSnapshotRef.UID field cleared
• Restore the PVC, but edit it so that the dataSource field points to the restored VolumeSnapshot

Instead, it sounds like Velero should preserve the snapshotHandle and create entirely new objects on restore, rather than trying to back up the existing objects.

Edit: The above scenario falls down in a cluster currently if the Contents object already exists, of course. We'd either need to clear the UID field, or create a new Contents object. On a new cluster in the same zone/region/data center, it shouldn't be an issue.