fix(controller): prevent zfs volume cr deletion if snapshot exists

Signed-off-by: sinhaashish <[email protected]>

.github/workflows/branch_preparation.yml

@@ -9,7 +9,7 @@ on:
 
 jobs:
   update_release_branch_after_release:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     if: ${{ github.ref_type == 'tag' }}
     steps:
       - uses: actions/checkout@v4
@@ -49,7 +49,7 @@ jobs:
           token: ${{ secrets.GITHUB_TOKEN }}
 
   update_develop_branch_on_release_branch_creation:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     if: ${{ github.ref_type == 'branch' }}
     steps:
       - uses: actions/checkout@v4
@@ -92,7 +92,7 @@ jobs:
           token: ${{ secrets.GITHUB_TOKEN }}
 
   prepare_release_branch_on_creation:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     if: ${{ github.ref_type == 'branch' }}
     steps:
       - uses: actions/checkout@v4

.github/workflows/build_and_push.yml

@@ -14,7 +14,7 @@ on:
 
 jobs:
   lint:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     steps:
       - name: Checkout
         uses: actions/checkout@v4
@@ -38,7 +38,7 @@ jobs:
         run: make golint
 
   unit-tests:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["lint"]
     steps:
       - name: Checkout
@@ -67,7 +67,7 @@ jobs:
           flags: unittests
 
   bdd-tests:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["unit-tests"]
     strategy:
       fail-fast: true
@@ -130,7 +130,7 @@ jobs:
           flags: bddtests
 
   csi-driver:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["bdd-tests"]
     steps:
       - name: Checkout
@@ -217,7 +217,7 @@ jobs:
             BRANCH=${{ env.BRANCH }}
 
   release-chart:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["csi-driver"]
     steps:
       - uses: actions/checkout@v4

.github/workflows/pull_request.yml

@@ -14,7 +14,7 @@ on:
 
 jobs:
   lint:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     steps:
       - name: Checkout
         uses: actions/checkout@v4
@@ -46,7 +46,7 @@ jobs:
         run: make golint
 
   validate_codegen:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     env:
       GOPATH: ${{ github.workspace }}/go
       GOBIN: ${{ github.workspace }}/go/bin
@@ -77,7 +77,7 @@ jobs:
           make verify-manifests
 
   unit-tests:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["lint", "validate_codegen"]
     steps:
       - name: Checkout
@@ -106,7 +106,7 @@ jobs:
           flags: unittests
 
   bdd-tests:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["unit-tests"]
     strategy:
       fail-fast: true
@@ -169,7 +169,7 @@ jobs:
           flags: bddtests
 
   csi-driver:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["bdd-tests"]
     steps:
       - name: Checkout

.github/workflows/release.yml

@@ -7,7 +7,7 @@ on:
 
 jobs:
   lint:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     steps:
       - name: Checkout
         uses: actions/checkout@v4
@@ -29,7 +29,7 @@ jobs:
           ct lint --config ct.yaml
 
   unit-tests:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["lint"]
     steps:
       - name: Checkout
@@ -53,7 +53,7 @@ jobs:
           flags: unittests
 
   bdd-tests:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["unit-tests"]
     strategy:
       fail-fast: true
@@ -111,7 +111,7 @@ jobs:
           flags: bddtests
 
   csi-driver:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["bdd-tests"]
     steps:
       - name: Checkout
@@ -210,7 +210,7 @@ jobs:
             BRANCH=${{ env.BRANCH }}
 
   release-chart:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-22.04-8-cores
     needs: ["csi-driver"]
     steps:
       - uses: actions/checkout@v4

ci/ci-test.sh

@@ -107,7 +107,7 @@ runTestSuite() {
 
   echo "running ginkgo test case with coverage ${coverageFile}"
 
-  if ! ginkgo -v -coverprofile="${coverageFile}" --label-filter="${labelFilter}" -covermode=atomic; then
+  if ! ginkgo -p -v -coverprofile="${coverageFile}" --label-filter="${labelFilter}" -covermode=atomic; then
 
   sudo zpool status
 

pkg/driver/controller.go

@@ -502,6 +502,18 @@ func (cs *controller) DeleteVolume(
 	unlock := cs.volumeLock.LockVolume(volumeID)
 	defer unlock()
 
+	// Fetch the list of snapshot for the given volume
+	snapList, err := zfs.GetSnapshotForVolume(volumeID)
+	if err != nil {
+		return nil, status.Errorf(
+			codes.NotFound,
+			"failed to handle delete volume request for {%s}, "+
+				"validation failed checking for snapshots. Error: %s",
+			req.VolumeId,
+			err.Error(),
+		)
+	}
+
 	// verify if the volume has already been deleted
 	vol, err := zfs.GetVolume(volumeID)
 	if vol != nil && vol.DeletionTimestamp != nil {
@@ -524,14 +536,17 @@ func (cs *controller) DeleteVolume(
 		return nil, status.Error(codes.Internal, "can not delete, volume creation is in progress")
 	}
 
-	// Delete the corresponding ZV CR
-	err = zfs.DeleteVolume(volumeID)
-	if err != nil {
-		return nil, errors.Wrapf(
-			err,
-			"failed to handle delete volume request for {%s}",
-			volumeID,
-		)
+	// Delete the corresponding ZV CR only if there are no snapshots present for the volume
+
+	if len(snapList.Items) == 0 {
+		err = zfs.DeleteVolume(volumeID)
+		if err != nil {
+			return nil, errors.Wrapf(
+				err,
+				"failed to handle delete volume request for {%s}",
+				volumeID,
+			)
+		}
 	}
 
 	sendEventOrIgnore("", volumeID, vol.Spec.Capacity, analytics.VolumeDeprovision)

pkg/zfs/volume.go

@@ -441,3 +441,12 @@ func IsVolumeReady(vol *apis.ZFSVolume) bool {
 
 	return false
 }
+
+// GetSnapshotForVolume fetches all the snapshots for the given volume
+func GetSnapshotForVolume(volumeID string) (*apis.ZFSSnapshotList, error) {
+	listOptions := metav1.ListOptions{
+		LabelSelector: ZFSVolKey + "=" + volumeID,
+	}
+	snapList, err := snapbuilder.NewKubeclient().WithNamespace(OpenEBSNamespace).List(listOptions)
+	return snapList, err
+}

tests/provision_test.go

@@ -24,6 +24,7 @@ var _ = Describe("[zfspv] TEST VOLUME PROVISIONING", func() {
 	Context("App is deployed with zfs driver", func() {
 		It("Running zfs volume Creation Test", volumeCreationTest)
 		It("Running zfs volume Creation Test with custom node id", Label("custom-node-id"), volumeCreationTest)
+		It("Running zfs volume Deletion Test", volumeDeletionTest)
 	})
 })
 
@@ -41,7 +42,7 @@ func exhaustiveVolumeTests(parameters map[string]string) {
 	snapshotAndCloneCreate()
 	// btrfs does not support online resize
 	if fstype != "btrfs" {
-		By("Resizing the PVC", resizeAndVerifyPVC)
+		By("Resizing the PVC", func() { resizeAndVerifyPVC(pvcNameFS) })
 	}
 	snapshotAndCloneCleanUp()
 	cleanUp()
@@ -51,62 +52,97 @@ func exhaustiveVolumeTests(parameters map[string]string) {
 func create(parameters map[string]string) {
 	By("####### Creating the storage class : " + parameters["fstype"] + " #######")
 	createFstypeStorageClass(parameters)
-	By("creating and verifying PVC bound status", createAndVerifyPVC)
-	By("Creating and deploying app pod", createDeployVerifyApp)
+	By("creating and verifying PVC bound status", func() { createAndVerifyPVC(pvcNameFS) })
+	By("Creating and deploying app pod", func() { createDeployVerifyApp(appNameFS, pvcNameFS) })
 	By("verifying ZFSVolume object", VerifyZFSVolume)
 	By("verifying storage class parameters")
 	VerifyStorageClassParams(parameters)
 }
 
 // Creates the snapshot/clone resources
 func snapshotAndCloneCreate() {
-	createSnapshot(pvcName, snapName)
-	verifySnapshotCreated(snapName)
-	createClone(clonePvcName, snapName, scObj.Name)
-	By("Creating and deploying clone app pod", createDeployVerifyCloneApp)
+	createSnapshot(pvcNameFS, snapNameFS)
+	verifySnapshotCreated(snapNameFS)
+	createClone(clonePvcNameFS, snapNameFS, scObj.Name)
+	By("Creating and deploying clone app pod", func() { createDeployVerifyCloneApp(cloneAppNameFS, clonePvcNameFS) })
 }
 
 // Removes the snapshot/clone resources
 func snapshotAndCloneCleanUp() {
-	deleteAppDeployment(cloneAppName)
-	deletePVC(clonePvcName)
-	deleteSnapshot(pvcName, snapName)
+	deleteAppDeployment(cloneAppNameFS)
+	deletePVC(clonePvcNameFS)
+	deleteSnapshot(pvcNameFS, snapNameFS)
 }
 
 // Removes the resources
 func cleanUp() {
-	deleteAppDeployment(appName)
-	deletePVC(pvcName)
+	deleteAppDeployment(appNameFS)
+	deletePVC(pvcNameFS)
 	By("Deleting storage class", deleteStorageClass)
 }
 
 func blockVolCreationTest() {
 	By("Creating default storage class", createStorageClass)
-	By("creating and verifying PVC bound status", createAndVerifyBlockPVC)
+	By("creating and verifying PVC bound status", func() { createAndVerifyPVC(pvcNameBlock) })
 
-	By("Creating and deploying app pod", createDeployVerifyBlockApp)
+	By("Creating and deploying app pod", func() { createDeployVerifyApp(appNameBlock, pvcNameBlock) })
 	By("verifying ZFSVolume object", VerifyZFSVolume)
 	By("verifying ZFSVolume property change", VerifyZFSVolumePropEdit)
-	By("Deleting application deployment")
 
-	createSnapshot(pvcName, snapName)
-	verifySnapshotCreated(snapName)
-	createClone(clonePvcName, snapName, scObj.Name)
-	By("Creating and deploying clone app pod", createDeployVerifyCloneApp)
+	createSnapshot(pvcNameBlock, snapNameBlock)
+	verifySnapshotCreated(snapNameBlock)
+	createClone(clonePvcNameBlock, snapNameBlock, scObj.Name)
+	By("Creating and deploying clone app pod", func() { createDeployVerifyCloneApp(cloneAppNameBlock, clonePvcNameBlock) })
 
 	By("Deleting clone and main application deployment")
-	deleteAppDeployment(cloneAppName)
-	deleteAppDeployment(appName)
+	deleteAppDeployment(cloneAppNameBlock)
+	deleteAppDeployment(appNameBlock)
 
 	By("Deleting snapshot, main pvc and clone pvc")
-	deletePVC(clonePvcName)
-	deleteSnapshot(pvcName, snapName)
-	deletePVC(pvcName)
+	deletePVC(clonePvcNameBlock)
+	deleteSnapshot(pvcNameBlock, snapNameBlock)
+	deletePVC(pvcNameBlock)
+
+	By("Deleting storage class", deleteStorageClass)
+}
+
+func blockVolDeletionTest() {
+	By("Creating default storage class", createStorageClass)
+	By("creating and verifying PVC bound status", func() { createAndVerifyPVC(pvcNameAplha) })
+
+	By("Creating and deploying app pod", func() { createDeployVerifyApp(appNameAlpha, pvcNameAplha) })
+	By("verifying ZFSVolume object", VerifyZFSVolume)
+
+	createSnapshot(pvcNameAplha, snapNameAlpha)
+	verifySnapshotCreated(snapNameAlpha)
+
+	By("Deleting main application deployment")
+	deleteAppDeployment(appNameAlpha)
+
+	By("Deleting main pvc")
+	deletePVC(pvcNameAplha)
+
+	By("Verifying ZFSVolume object after pvc deletion when snapshot is present", VerifyZFSVolume)
+
+	By("Creating clone from the snapshot")
+	createClone(clonePvcNameAlpha, snapNameAlpha, scObj.Name)
+	By("Creating and deploying clone app pod", func() { createDeployVerifyCloneApp(cloneAppNameAlpha, clonePvcNameAlpha) })
+
+	By("Deleting clone application deployment, clone pvc")
+	deleteAppDeployment(cloneAppNameAlpha)
+
+	deletePVC(clonePvcNameAlpha)
+	deleteSnapshot(pvcNameAplha, snapNameAlpha)
 
 	By("Deleting storage class", deleteStorageClass)
 }
 
 func volumeCreationTest() {
 	By("Running volume creation test", fsVolCreationTest)
 	By("Running block volume creation test", blockVolCreationTest)
+
+}
+
+func volumeDeletionTest() {
+	By("Running volume deletion test", blockVolDeletionTest)
 }