[DNM] [WIP] Add support for smart cloning #248
Conversation
current CSI spec says a volume can be created by a snapshot or a parent volume, This PR adds the ability to clone a volume from parent volume Signed-off-by: Madhu Rajanna <[email protected]>
|
cc @j-griffith |
| if req.VolumeContentSource != nil { | ||
| if err = cs.checkSnapshot(req, rbdVol); err != nil { | ||
| if err = cs.cloneVolumeFromSource(req, rbdVol); err != nil { |
There was a problem hiding this comment.
I think createVolumeFromExistingSource would be more apt here.
| @@ -448,6 +448,21 @@ func createSnapshot(pOpts *rbdSnapshot, adminID string, credentials map[string]s | |||
| return nil | |||
| } | |||
|
|
|||
| func unprotectAndDeleteSnapshot(rbdSnap *rbdSnapshot, credentials map[string]string) error { | |||
There was a problem hiding this comment.
Lets document this function.
|
@j-griffith @nixpanic can you give some pointer on how to test this feature? |
|
|
||
| err = restoreSnapshot(rbdVol, rbdSnap, rbdVol.AdminID, req.GetSecrets()) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
if restoring fails, should rbdSnap not be removed again?
| return snapErr | ||
| } | ||
|
|
||
| err = restoreSnapshot(rbdVol, rbdSnap, rbdVol.AdminID, req.GetSecrets()) |
There was a problem hiding this comment.
In case the requested new volume is larger than the snapshot, it needs to be resized somewhere.
There was a problem hiding this comment.
we have 2 options
- reject request of the new PVC size is not equal to parent volume
(this require user has to create a PVC with same size and then do resize ) - once the new volume is created do resize if required
i will try to implement option 2
There was a problem hiding this comment.
one question am having here is, what happens if the requested volume size is less than parent volume?
is it kind of data lose?
There was a problem hiding this comment.
I do not think would be a valid request. The new volume should have the same size or be larger.
There was a problem hiding this comment.
Size less than is invalid; in the case of the kubernetes CSI provisioner the request will be rejected at the provisioner and never even make it to the plugin.
In terms of the CSI Spec, we should expect this case and should reject the request with an error response.
There was a problem hiding this comment.
@j-griffith yes agreed.
| rbdSnap.VolName = rbdVolume.VolName | ||
| rbdSnap.SnapName = snapName | ||
| if snapID == "" { | ||
| snapID = "csi-rbd-" + rbdVolume.VolName + "-snap-" + uniqueID |
There was a problem hiding this comment.
can this be made a helper function like createSnapId(..) so that the format is sortof documented somewhere
Maybe a parseSnapId(..) function makes sense too.
There was a problem hiding this comment.
yeah, might be good to move that out even if we don't currently use it directly from anywhere else. It might be worth considering generalizing it a bit and see where we can share it. I'd also consider setting up some constants rather than raw strings in here. I'm also not sure about the looooong naming convention, but don't have a better idea off the top of my head :)
| } | ||
| klog.V(4).Infof("create volume %s from snapshot %s", req.GetName(), rbdSnap.SnapName) | ||
| // Unprotect snapshot | ||
| err = unprotectAndDeleteSnapshot(rbdSnap, req.GetSecrets()) |
There was a problem hiding this comment.
Deleting a snapshot w/ an attached clone is not permitted unless your OSDs are configured to only support Mimic-or-later clients. If that is the case, there is no need to protect/unprotect the snapshots.
I actually added the support for simplified image cloning to Mimic to simplify the in-development CSI driver (and avoid the design mistakes of the RBD Cinder driver). I'm perfectly happy w/ this only working for Mimic and later clients, but if that's the case, perhaps we can remove all the protect/unprotect ugliness.
You will also run into issues attempting to delete PVs that have attached clones, which is one of the rationales for adding the "rbd trash rm" command.
There was a problem hiding this comment.
@dillaman Thanks for the review, i have a few questions
Deleting a snapshot w/ an attached clone is not permitted unless your OSDs are configured to only support Mimic-or-later clients. If that is the case, there is no need to protect/unprotect the snapshots.
is there a way to delete a snapshot w/ an attached clone. from csi perspective, a volume can be deleted if there are snapshots associated to it.
you are suggested that we can don't really need to protect and unprotect the snapshots if osd is configured to support Mimic or latest client?
I actually added the support for simplified image cloning to Mimic to simplify the in-development CSI driver (and avoid the design mistakes of the RBD Cinder driver). I'm perfectly happy w/ this only working for Mimic and later clients, but if that's the case, perhaps we can remove all the protect/unprotect ugliness.
from CSI point of view, we don't know what is the backend version is it mimic or latest or even old one ( removing protect and unprotect will cause an issue for the older version?)
You will also run into issues attempting to delete PVs that have attached clones, which is one of the rationales for adding the "rbd trash rm" command.
yes currently we have an issue for that one i think it is #227
There was a problem hiding this comment.
It would be nice if we could just say "must use >= mimic" but that's not really feasible. For those (like me) that were curious about the special features in mimic, this seemed like good FYI: https://ceph.com/community/new-mimic-simplified-rbd-image-cloning/
@dillaman I had been looking at using flatten to disassociate volumes that were created from snapshots; and my thought was that works on clones as well (assuming clone via snap). Curious, is there a concern with that?
There was a problem hiding this comment.
is there a way to delete a snapshot w/ an attached clone. from csi perspective, a volume can be deleted if there are snapshots associated to it.
Just to clarify, the spec actually provides some flexibility here and "allows" the deletion of a volume to fail if it has snapshots associated with it. I suspect though since on RBD snapshots are the clones that's what you were referring to in this case but I just wanted to make sure. At least in that case we only have to do the flatten here on the "csi clone" or "volume from snap" case.
There was a problem hiding this comment.
@j-griffith Playing devil's advocate, given that the CSI is so new, is prohibiting cloning for < mimic really that bad? Of course, if you just immediately flatten the clones for the < mimic case, that seems like a fair compromise.
The downside is that (1) any attempts to delete the source PV will fail while the clone is still attached, (2) flattening might take too long (blocking) of an amount of time if it's blocking other PV operations, (3) you would need to prevent creation of snapshots on the clone until it has been flattened (assuming you could get concurrent PV operations against it), and (4) it's a waste of space.
There was a problem hiding this comment.
is there a way to delete a snapshot w/ an attached clone. from csi perspective, a volume can be deleted if there are snapshots associated to it.
This is only possible w/ clone format v2 (requiring a Mimic or later release)
you are suggested that we can don't really need to protect and unprotect the snapshots if osd is configured to support Mimic or latest client?
Correct -- RBD will automatically use the new clone format v2 if the configured to only support Mimic and later clients. This can be force-overridden by adding --rbd_default_clone_format=2 to the rbd clone CLI.
from CSI point of view, we don't know what is the backend version is it mimic or latest or even old one ( removing protect and unprotect will cause an issue for the older version?)
It's trivial to query from the ceph CLI.
yes currently we have an issue for that one i think it is #227
Thanks, I'll comment on that.
|
Great to see you jumped on this @Madhu-1 I think we can infer most of what we need based on the spec and then update as needed when the external-provisioner supports Volume DataSources. For any functional testing we're going to need not ony the CSI Provisioner updates, but also the Kubernetes updates; the good news is they're both in progress, the bad news is they're not moving very quickly (or even at all). I do expect them this release, so I think we should test with the PR's that exist now (or update them as needed) and then be sure to come back to this when things are official in K8s and the CSI K8s provisioner. |
|
@j-griffith can you point me to the kube PR which I can pick to test this |
So on the Kube side there's a KEP that's been rolling around: kubernetes/enhancements#642 I had a POC fork to test it out here (note it's outdated at this point but should be usable to test): And finally there's a CSI Provisioner PR here (again needs updated once the KEP is finalized): I realize it's sort of disjointed at the moment, waiting for the KEP to get everything else in line; but it should provide enough to do some good testing for you. |
|
@Madhu-1 now that cloning is merged in Kubernetes, and the PR for the CSI sidecar is wrapping up (kubernetes-csi/external-provisioner#220) , thought I'd check in with you on this. Are you still wanting to finish this up, do you need some help with anything (like decoupling snapshots etc)? |
|
@j-griffith we should be able to complete this soon , I can work with @Madhu-1 too. Thanks for offering help John! |
|
@j-griffith @humblec I would like to take this up for |
|
@Madhu-1 sure, planning to come up with a list of things for 1.1.1 , will talk to you on this and this is definitely in. |
|
suppressed by #695 |
Syncing latest changes from upstream devel for ceph-csi
current CSI spec says a volume can be created by a snapshot or a parent volume, This PR adds the ability to clone a volume from parent volume.