-
Notifications
You must be signed in to change notification settings - Fork 609
nvmeof: add volume cloning capability #6277
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,7 @@ var _ = ginkgo.Describe("nvmeof", func() { | |
| ginkgo.Context("Test NVMe CSI", ginkgo.Ordered, func() { | ||
|
|
||
| pvcPath := nvmeofExamplePath + "pvc.yaml" | ||
| pvcClonePath := nvmeofExamplePath + "pvc-clone.yaml" | ||
| appPath := nvmeofExamplePath + "pod.yaml" | ||
| rawPvcPath := nvmeofExamplePath + "raw-block-pvc.yaml" | ||
| rawAppPath := nvmeofExamplePath + "raw-block-pod.yaml" | ||
|
|
@@ -432,5 +433,70 @@ var _ = ginkgo.Describe("nvmeof", func() { | |
| validateRBDImageCount(f, 0, nvmeofPool) | ||
| validateOmapCount(f, 0, rbdType, nvmeofPool, volumesType) | ||
| }) | ||
|
|
||
| ginkgo.It("Cloning nvmeof PVC", func() { | ||
| // This test validates cloning of NVMe-oF PVCs. | ||
| // | ||
| // Test flow: | ||
| // 1. Create a source PVC and bind it to an app | ||
| // 2. Create a clone of the source PVC and bind it to an app | ||
| // 3. Delete the clone app and PVC | ||
| // 4. Delete the source app and PVC | ||
|
|
||
| ginkgo.By("Creating a source PVC") | ||
| sourcePVC, err := loadPVC(pvcPath) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| sourcePVC.Namespace = f.UniqueName | ||
| sourcePVC.Spec.StorageClassName = &nvmeofStorageClass | ||
|
|
||
| err = createPVCAndvalidatePV(f.ClientSet, sourcePVC, deployTimeout) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| ginkgo.By("Binding source PVC to an application") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't need to bind the source volume to an app, that is validated in other tests already too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohh ok I can remove it. I thought you want both to be bind to app .
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having both is ok, but not required. The cloned volume needs complete testing. The current PR is fine by me, no need to update it unless there is something else you want to improve too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I will keep it as is. |
||
| sourceApp, err := loadApp(appPath) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| sourceApp.Namespace = f.UniqueName | ||
| sourceApp.Spec.Volumes[0].PersistentVolumeClaim.ClaimName = sourcePVC.Name | ||
|
|
||
| err = createApp(f.ClientSet, sourceApp, deployTimeout) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| ginkgo.By("Creating a clone of the source PVC") | ||
| // Load clone PVC template with DataSource already configured | ||
| clonePVC, err := loadPVC(pvcClonePath) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| clonePVC.Namespace = f.UniqueName | ||
| clonePVC.Spec.StorageClassName = &nvmeofStorageClass | ||
| clonePVC.Spec.DataSource.Name = sourcePVC.Name | ||
|
|
||
| err = createPVCAndvalidatePV(f.ClientSet, clonePVC, deployTimeout) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| ginkgo.By("Binding clone PVC to an application") | ||
| cloneApp, err := loadApp(appPath) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| cloneApp.Name = sourceApp.Name + "-clone" | ||
| cloneApp.Namespace = f.UniqueName | ||
| cloneApp.Spec.Volumes[0].PersistentVolumeClaim.ClaimName = clonePVC.Name | ||
|
|
||
| err = createApp(f.ClientSet, cloneApp, deployTimeout) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| ginkgo.By("Deleting the clone application and PVC") | ||
| err = deletePVCAndApp("", f, clonePVC, cloneApp) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| ginkgo.By("Deleting the source application and PVC") | ||
| err = deletePVCAndApp("", f, sourcePVC, sourceApp) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
|
|
||
| // validate all backend rbd images are cleaned up | ||
| validateRBDImageCount(f, 0, nvmeofPool) | ||
| validateOmapCount(f, 0, rbdType, nvmeofPool, volumesType) | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| --- | ||
| apiVersion: v1 | ||
| kind: PersistentVolumeClaim | ||
| metadata: | ||
| name: nvmeof-pvc-clone | ||
| spec: | ||
| storageClassName: csi-nvmeof-sc | ||
| dataSource: | ||
| name: nvmeof-pvc | ||
| kind: PersistentVolumeClaim | ||
| accessModes: | ||
| - ReadWriteOnce | ||
| resources: | ||
| requests: | ||
| storage: 64Mi |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,7 +118,37 @@ func (cs *Server) CreateVolume( | |
| } | ||
| defer cs.volumeLocks.Release(req.GetName()) | ||
|
|
||
| // Extract clone source if present (for locking) | ||
| // Note- there is no need for snapshot source locking because snapshots are not | ||
|
gadididi marked this conversation as resolved.
|
||
| // used in the NVMe-oF gateway and do not affect the NVMe-oF resources. | ||
| var sourceVolumeID, sourceSnapshotID string | ||
| if contentSource := req.GetVolumeContentSource(); contentSource != nil { | ||
| switch contentSource.GetType().(type) { | ||
| case *csi.VolumeContentSource_Snapshot: | ||
| if snapshot := contentSource.GetSnapshot(); snapshot != nil { | ||
| sourceSnapshotID = snapshot.GetSnapshotId() | ||
| log.DebugLog(ctx, "Creating volume from snapshot: %s", sourceSnapshotID) | ||
| } | ||
| case *csi.VolumeContentSource_Volume: | ||
| if vol := contentSource.GetVolume(); vol != nil { | ||
| sourceVolumeID = vol.GetVolumeId() | ||
| log.DebugLog(ctx, "Creating volume clone from volume: %s", sourceVolumeID) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Lock source volume to prevent concurrent deletion | ||
| if sourceVolumeID != "" { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lock on snapshot is not required?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @Madhu-1 , I missed your comment here. |
||
| if acquired := cs.volumeLocks.TryAcquire(sourceVolumeID); !acquired { | ||
| log.ErrorLog(ctx, util.VolumeOperationAlreadyExistsFmt, sourceVolumeID) | ||
|
|
||
| return nil, status.Errorf(codes.Aborted, util.VolumeOperationAlreadyExistsFmt, sourceVolumeID) | ||
| } | ||
| defer cs.volumeLocks.Release(sourceVolumeID) | ||
| } | ||
|
|
||
| // Step 1: Create RBD volume through backend. if exists, it is ok. | ||
| // RBD backend automatically handles cloning when VolumeContentSource is present. | ||
| res, err := cs.backendServer.CreateVolume(ctx, req) | ||
| if err != nil { | ||
| log.ErrorLog(ctx, "failed to create RBD volume: %v", err) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,7 @@ func (d *nvmeofDriver) Run(conf *util.Config) { | |
| csi.ControllerServiceCapability_RPC_MODIFY_VOLUME, | ||
| csi.ControllerServiceCapability_RPC_EXPAND_VOLUME, | ||
| csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT, | ||
| csi.ControllerServiceCapability_RPC_CLONE_VOLUME, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add later.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please include an e2e test for cloning with this PR, thanks!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed that. I am on it!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| }) | ||
|
|
||
| cd.AddVolumeCapabilityAccessModes([]csi.VolumeCapability_AccessMode_Mode{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clone from snapshot source is not added to e2e?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gadididi is this something you still want to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nixpanic sure, I can do it, but is ok to make nvmeof e2e test longer than now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iPraveenParihar Hi! I will add snapshot tests in separated PR 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, create a tracker for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6435