Repository: openebs/zfs-localpv

Issue:
REFQUOTA is updated even when volume expansion fails due to insufficient space
**What steps did you take and what happened:**
When attempting to expand a PVC using ZFS LocalPV, I've observed that the REFQUOTA value gets updated to the new desired size even when the actual volume expansion fails due to insufficient space in the zpool.


**What did you expect to happen:**
The REFQUOTA value should only be updated after a successful volume expansion. If the expansion fails due to insufficient space, the REFQUOTA should remain at its original value or be reverted back.


**The output of the following commands will help us better understand what's going on**:
```
zpool list
NAME         SIZE  ALLOC   FREE  CKPOINT  EXPANDSZ   FRAG    CAP  DEDUP    HEALTH  ALTROOT
zfspv-pool  99.5G   192M  99.3G        -         -     4%     0%  1.00x    ONLINE  -
```
```
 zfs list -o name,quota,reservation,refquota,refreservation,used,available
NAME                                                 QUOTA  RESERV  REFQUOTA  REFRESERV   USED  AVAIL
zfspv-pool                                            none    none      none         3G  64.3G  35.1G
zfspv-pool/pvc-4245caa5-cab9-40b1-aac8-431569fb195a   none    none      200G         5G     5G  37.1G
```

**Environment:**
- LocalPV-ZFS version 2.7.1


Discussion:
@Abhinandan-Purkait: Hey @donch, would you be interested in taking this up?@lcaflc: I confirm this bug, got exactly the same issue today.

The issue is that the provisionner first update the `refquota`, as `refquota` are not limited everything is fine and volume in the pod see the new size.

However this is not the same for `refreservation`. Space must be available to edit it. So if your pool cannot handle this extra space it "silently" failed.

here are the logs:
```
│ openebs-zfs-plugin I0507 09:02:20.631101       1 zfsnode.go:114] zfs node controller: updated node object openebs/k8s-prod-dc5-worker-es-2                                                                                                                                                                                                                                                                                               │
│ openebs-zfs-plugin I0507 09:03:00.689187       1 grpc.go:72] GRPC call: /csi.v1.Node/NodeExpandVolume requests {"capacity_range":{"required_bytes":1539745775616},"staging_target_path":"/var/lib/kubelet/plugins/kubernetes.io/csi/zfs.csi.openebs.io/ecdb1bc52fa0ff610ef6c2c247ee893dca8e4dc94f9aafbcff0fa7d1951bcd54/globalmount","volume_capability":{"AccessType":{"Mount":{"fs_type":"zfs"}},"access_mode":{"mode":1}},"volume_id" │
│ openebs-zfs-plugin E0507 09:03:00.748036       1 zfs_util.go:770] zfs: could not resize the volume zfspv-pool/pvc-05ecee57-715e-4d58-ba43-e98bb5b85460 cmd [set refquota=1539745775616 refreservation=1539745775616 zfspv-pool/pvc-05ecee57-715e-4d58-ba43-e98bb5b85460] error: cannot set property for 'zfspv-pool/pvc-05ecee57-715e-4d58-ba43-e98bb5b85460': size is greater than available space                                      │
│ openebs-zfs-plugin E0507 09:03:00.748094       1 grpc.go:79] GRPC error: rpc error: code = Internal desc = failed to handle NodeExpandVolume Request for pvc-05ecee57-715e-4d58-ba43-e98bb5b85460, {exit status 255}                                                                                                                                                                                                                     │
```

Provisionner should first try to do update `refreservation` and then `refquota`.  The current status is dangerous because pod that think it can eat all new datas, will in fact fail after writing old volume size.@Abhinandan-Purkait: @lcaflc Please feel free to raise a fix if you want to. @plaffitt: Hello, I confirm I can reproduce this bug too, I will start working on a fix.@Abhinandan-Purkait: Thanks @plaffitt

Solution patch:
diff --git a/pkg/driver/agent.go b/pkg/driver/agent.go
index e45b1ef..1e5035f 100644
--- a/pkg/driver/agent.go
+++ b/pkg/driver/agent.go
@@ -380,8 +380,15 @@ func (ns *node) NodeExpandVolume(
 	}
 
 	if err = zfs.ResizeZFSVolume(vol, req.GetVolumePath(), resizefs); err != nil {
+		code := codes.Internal
+
+		// error defined in zfs source code: https://github.com/openzfs/zfs/blob/zfs-2.3.99/lib/libzfs/libzfs_util.c#L551
+		if strings.Contains(err.Error(), "size is greater than available space") {
+			code = codes.ResourceExhausted
+		}
+
 		return nil, status.Errorf(
-			codes.Internal,
+			code,
 			"failed to handle NodeExpandVolume Request for %s, {%s}",
 			req.VolumeId,
 			err.Error(),
diff --git a/pkg/zfs/zfs_util.go b/pkg/zfs/zfs_util.go
index b7efb60..6ab4af0 100644
--- a/pkg/zfs/zfs_util.go
+++ b/pkg/zfs/zfs_util.go
@@ -89,7 +89,7 @@ func GetVolumeType(fstype string) string {
 	}
 }
 
-// builldZvolCreateArgs returns zfs create command for zvol along with attributes as a string array
+// buildZvolCreateArgs returns zfs create command for zvol along with attributes as a string array
 func buildZvolCreateArgs(vol *apis.ZFSVolume) []string {
 	var ZFSVolArg []string
 
@@ -132,7 +132,7 @@ func buildZvolCreateArgs(vol *apis.ZFSVolume) []string {
 	return ZFSVolArg
 }
 
-// builldCloneCreateArgs returns zfs clone commands for zfs volume/dataset along with attributes as a string array
+// buildCloneCreateArgs returns zfs clone commands for zfs volume/dataset along with attributes as a string array
 func buildCloneCreateArgs(vol *apis.ZFSVolume) []string {
 	var ZFSVolArg []string
 
@@ -193,7 +193,7 @@ func buildZFSSnapCreateArgs(snap *apis.ZFSSnapshot) []string {
 	return ZFSSnapArg
 }
 
-// builldZFSSnapDestroyArgs returns zfs destroy command for zfs snapshot
+// buildZFSSnapDestroyArgs returns zfs destroy command for zfs snapshot
 // zfs destroy <poolname>/<volname>@<snapname>
 func buildZFSSnapDestroyArgs(snap *apis.ZFSSnapshot) []string {
 	var ZFSSnapArg []string
@@ -206,7 +206,7 @@ func buildZFSSnapDestroyArgs(snap *apis.ZFSSnapshot) []string {
 	return ZFSSnapArg
 }
 
-// builldDatasetCreateArgs returns zfs create command for dataset along with attributes as a string array
+// buildDatasetCreateArgs returns zfs create command for dataset along with attributes as a string array
 func buildDatasetCreateArgs(vol *apis.ZFSVolume) []string {
 	var ZFSVolArg []string
 
@@ -252,7 +252,7 @@ func buildDatasetCreateArgs(vol *apis.ZFSVolume) []string {
 	return ZFSVolArg
 }
 
-// builldVolumeSetArgs returns volume set command along with attributes as a string array
+// buildVolumeSetArgs returns volume set command along with attributes as a string array
 // TODO(pawan) need to find a way to identify which property has changed
 func buildVolumeSetArgs(vol *apis.ZFSVolume) []string {
 	var ZFSVolArg []string
@@ -281,7 +281,7 @@ func buildVolumeSetArgs(vol *apis.ZFSVolume) []string {
 	return ZFSVolArg
 }
 
-// builldVolumeResizeArgs returns volume set  for resizing the zfs volume
+// buildVolumeResizeArgs returns volume set command for resizing the zfs volume
 func buildVolumeResizeArgs(vol *apis.ZFSVolume) []string {
 	var ZFSVolArg []string
 
@@ -306,7 +306,7 @@ func buildVolumeResizeArgs(vol *apis.ZFSVolume) []string {
 	return ZFSVolArg
 }
 
-// builldVolumeBackupArgs returns volume send command for sending the zfs volume
+// buildVolumeBackupArgs returns volume send command for sending the zfs volume
 func buildVolumeBackupArgs(bkp *apis.ZFSBackup, vol *apis.ZFSVolume) ([]string, error) {
 	var ZFSVolArg []string
 	backupDest := bkp.Spec.BackupDest
@@ -335,7 +335,7 @@ func buildVolumeBackupArgs(bkp *apis.ZFSBackup, vol *apis.ZFSVolume) ([]string,
 	return ZFSVolArg, nil
 }
 
-// builldVolumeRestoreArgs returns volume recv command for receiving the zfs volume
+// buildVolumeRestoreArgs returns volume recv command for receiving the zfs volume
 func buildVolumeRestoreArgs(rstr *apis.ZFSRestore) ([]string, error) {
 	var ZFSVolArg []string
 	var ZFSRecvParam string
@@ -386,7 +386,7 @@ func buildVolumeRestoreArgs(rstr *apis.ZFSRestore) ([]string, error) {
 	return ZFSVolArg, nil
 }
 
-// builldVolumeDestroyArgs returns volume destroy command along with attributes as a string array
+// buildVolumeDestroyArgs returns volume destroy command along with attributes as a string array
 func buildVolumeDestroyArgs(vol *apis.ZFSVolume) []string {
 	var ZFSVolArg []string
 
@@ -760,16 +760,30 @@ func GetVolumeDevPath(vol *apis.ZFSVolume) (string, error) {
 
 // ResizeZFSVolume resize volume
 func ResizeZFSVolume(vol *apis.ZFSVolume, mountpath string, resizefs bool) error {
+	oldReservation, err := GetVolumeProperty(vol, reservationPropertyName(vol.Spec.QuotaType))
+	if err != nil {
+		return err
+	}
 
 	volume := vol.Spec.PoolName + "/" + vol.Name
 	args := buildVolumeResizeArgs(vol)
 	cmd := exec.Command(ZFSVolCmd, args...)
-	out, err := cmd.CombinedOutput()
 
+	out, err := cmd.CombinedOutput()
 	if err != nil {
-		klog.Errorf(
-			"zfs: could not resize the volume %v cmd %v error: %s", volume, args, string(out),
-		)
+		klog.Errorf("zfs: could not resize the volume %v cmd %v error: %s", volume, args, string(out))
+		klog.Infof("zfs: reverting the volume quota to %s", oldReservation)
+
+		revertedVol := vol.DeepCopy()
+		revertedVol.Spec.Capacity = oldReservation
+		args := buildVolumeResizeArgs(revertedVol)
+		cmd := exec.Command(ZFSVolCmd, args...)
+		if out, err := cmd.CombinedOutput(); err != nil {
+			klog.Errorf(
+				"zfs: could not revert the volume %v quota cmd %v error: %s", volume, args, string(out),
+			)
+		}
+
 		return err
 	}
 
@@ -973,15 +987,20 @@ func decodeListOutput(raw []byte) ([]apis.Pool, error) {
 
 // reservationProperty returns the reservation property based on the quota type.
 func reservationProperty(quotaType, capacity string) string {
+	return reservationPropertyName(quotaType) + "=" + capacity
+}
+
+// reservationPropertyName returns the reservation property name based on the quota type.
+func reservationPropertyName(quotaType string) string {
 	validQuotaType := quotaProperty(quotaType)
 
 	reservationProperties := map[string]string{
-		"quota":    "reservation=",
-		"refquota": "refreservation=",
+		"quota":    "reservation",
+		"refquota": "refreservation",
 	}
 
-	// Return the mapped property or default to "reservation="
-	return reservationProperties[validQuotaType] + capacity
+	// Return the mapped property or default to "reservation"
+	return reservationProperties[validQuotaType]
 }
 
 // quotaProperty ensures backwards compatibility for the quota property.



Test patch:
diff --git a/pkg/zfs/volume_test.go b/pkg/zfs/volume_test.go
index df0d4fb..7b8b5a5 100644
--- a/pkg/zfs/volume_test.go
+++ b/pkg/zfs/volume_test.go
@@ -62,3 +62,26 @@ func TestReservationProperty(t *testing.T) {
 		})
 	}
 }
+
+func TestQuotaProperty(t *testing.T) {
+	tests := []struct {
+		name      string
+		quotaType string
+		expected  string
+	}{
+		{"Empty quotaType defaults to quota", "", "quota"},
+		{"Valid quotaType quota", "quota", "quota"},
+		{"Valid quotaType refquota", "refquota", "refquota"},
+		{"Invalid quotaType defaults to quota", "invalid", "quota"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := quotaProperty(tt.quotaType)
+			if result != tt.expected {
+				t.Errorf("For quotaType %q, expected %q but got %q",
+					tt.quotaType, tt.expected, result)
+			}
+		})
+	}
+}