Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver
Pull request #1841 (2024-10-04 19:37:18)

Update logic to use SI for VACs
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug

/kind cleanup
> /kind design
> /kind documentation
> /kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:

This PR updates the logic to accept SI units for throughput for VolumeAttributesClasses. This makes the VACs consistent with SI being used in other areas, such as in StorageClasses. This also removes the duplicate definition of the command line arguments in the dev overlay.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
This feature changes VolumeAttributesClass, so throughput specifications will need "Mi".
```



Patch:
diff --git a/deploy/kubernetes/overlays/dev/driver-args.yaml b/deploy/kubernetes/overlays/dev/driver-args.yaml
deleted file mode 100644
index c8db2b93..00000000
--- a/deploy/kubernetes/overlays/dev/driver-args.yaml
+++ /dev/null
@@ -1,7 +0,0 @@
-- op: add
-  path: /spec/template/spec/containers/0/args/-
-  value: --supports-dynamic-throughput-provisioning=hyperdisk-balanced,hyperdisk-throughput,hyperdisk-ml
-
-- op: add
-  path: /spec/template/spec/containers/0/args/-
-  value: --supports-dynamic-iops-provisioning=hyperdisk-balanced,hyperdisk-extreme
\ No newline at end of file
diff --git a/deploy/kubernetes/overlays/dev/kustomization.yaml b/deploy/kubernetes/overlays/dev/kustomization.yaml
index 03861055..f6ffff39 100644
--- a/deploy/kubernetes/overlays/dev/kustomization.yaml
+++ b/deploy/kubernetes/overlays/dev/kustomization.yaml
@@ -9,14 +9,6 @@ resources:
 # Here dev overlay is using the same image as alpha
 transformers:
 - ../../images/stable-master
-# Apply patches to support dynamic provisioning for hyperdisks
-patches:
-- path: ./driver-args.yaml
-  target:
-    group: apps
-    version: v1
-    kind: Deployment
-    name: csi-gce-pd-controller
 # To change the dev image, add something like the following.
 #images:
 #- name: gke.gcr.io/gcp-compute-persistent-disk-csi-driver
diff --git a/examples/kubernetes/demo-vol-create.yaml b/examples/kubernetes/demo-vol-create.yaml
index 1ca0e055..5fa53604 100644
--- a/examples/kubernetes/demo-vol-create.yaml
+++ b/examples/kubernetes/demo-vol-create.yaml
@@ -16,7 +16,7 @@ metadata:
 driverName: pd.csi.storage.gke.io
 parameters:
   iops: "3000"
-  throughput: "150"
+  throughput: "150Mi"
 ---
 apiVersion: storage.k8s.io/v1beta1
 kind: VolumeAttributesClass
@@ -25,7 +25,7 @@ metadata:
 driverName: pd.csi.storage.gke.io
 parameters:
   iops: "3013"
-  throughput: "151"
+  throughput: "151Mi"
 ---
 apiVersion: v1
 kind: PersistentVolumeClaim
diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go
index e80346ed..87283719 100644
--- a/pkg/common/parameters.go
+++ b/pkg/common/parameters.go
@@ -343,7 +343,7 @@ func ExtractModifyVolumeParameters(parameters map[string]string) (ModifyVolumePa
 			}
 			modifyVolumeParams.IOPS = &iops
 		case "throughput":
-			throughput, err := ConvertStringToInt64(value)
+			throughput, err := ConvertMiStringToInt64(value)
 			if err != nil {
 				return ModifyVolumeParameters{}, fmt.Errorf("parameters contain invalid throughput parameter: %w", err)
 			}


Test patch:
diff --git a/pkg/common/parameters_test.go b/pkg/common/parameters_test.go
index cb60ecff..35a11632 100644
--- a/pkg/common/parameters_test.go
+++ b/pkg/common/parameters_test.go
@@ -485,7 +485,7 @@ func TestSnapshotParameters(t *testing.T) {
 func TestExtractModifyVolumeParameters(t *testing.T) {
 	parameters := map[string]string{
 		"iops":       "1000",
-		"throughput": "500",
+		"throughput": "500Mi",
 	}
 
 	iops := int64(1000)
diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go
index 0e79a366..06db7222 100644
--- a/pkg/gce-pd-csi-driver/controller_test.go
+++ b/pkg/gce-pd-csi-driver/controller_test.go
@@ -1789,7 +1789,7 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) {
 						},
 					},
 				},
-				MutableParameters: map[string]string{"iops": "20000", "throughput": "600"},
+				MutableParameters: map[string]string{"iops": "20000", "throughput": "600Mi"},
 			},
 			expIops:       20000,
 			expThroughput: 600,
@@ -1822,7 +1822,7 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) {
 						},
 					},
 				},
-				MutableParameters: map[string]string{"iops": "20000", "throughput": "600"},
+				MutableParameters: map[string]string{"iops": "20000", "throughput": "600Mi"},
 			},
 			expIops:       0,
 			expThroughput: 0,
@@ -1890,7 +1890,7 @@ func TestVolumeModifyOperation(t *testing.T) {
 			name: "Update volume with valid parameters",
 			req: &csi.ControllerModifyVolumeRequest{
 				VolumeId:          testVolumeID,
-				MutableParameters: map[string]string{"iops": "20000", "throughput": "600"},
+				MutableParameters: map[string]string{"iops": "20000", "throughput": "600Mi"},
 			},
 			diskType: "hyperdisk-balanced",
 			params: &common.DiskParameters{
@@ -1906,7 +1906,7 @@ func TestVolumeModifyOperation(t *testing.T) {
 			name: "Update volume with invalid parameters",
 			req: &csi.ControllerModifyVolumeRequest{
 				VolumeId:          testVolumeID,
-				MutableParameters: map[string]string{"iops": "0", "throughput": "0"},
+				MutableParameters: map[string]string{"iops": "0", "throughput": "0Mi"},
 			},
 			diskType: "hyperdisk-balanced",
 			params: &common.DiskParameters{
@@ -1922,7 +1922,7 @@ func TestVolumeModifyOperation(t *testing.T) {
 			name: "Update volume with valid parameters but invalid disk type",
 			req: &csi.ControllerModifyVolumeRequest{
 				VolumeId:          testVolumeID,
-				MutableParameters: map[string]string{"iops": "20000", "throughput": "600"},
+				MutableParameters: map[string]string{"iops": "20000", "throughput": "600Mi"},
 			},
 			diskType: "pd-ssd",
 			params: &common.DiskParameters{
@@ -2053,7 +2053,7 @@ func TestVolumeModifyErrorHandling(t *testing.T) {
 				},
 			},
 			modifyReq: &csi.ControllerModifyVolumeRequest{
-				MutableParameters: map[string]string{"iops": "3001", "throughput": "151"},
+				MutableParameters: map[string]string{"iops": "3001", "throughput": "151Mi"},
 			},
 			modifyVolumeErrors: map[*meta.Key]error{
 				meta.ZonalKey(name, "us-central1-a"): &googleapi.Error{
@@ -2089,7 +2089,7 @@ func TestVolumeModifyErrorHandling(t *testing.T) {
 				},
 			},
 			modifyReq: &csi.ControllerModifyVolumeRequest{
-				MutableParameters: map[string]string{"iops": "10000", "throughput": "2400"},
+				MutableParameters: map[string]string{"iops": "10000", "throughput": "2400Mi"},
 			},
 			modifyVolumeErrors: map[*meta.Key]error{
 				meta.ZonalKey(name, "us-central1-a"): &googleapi.Error{Code: int(codes.InvalidArgument), Message: "InvalidArgument"},
diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go
index cd60d8b7..c77c7b91 100644
--- a/test/e2e/tests/single_zone_e2e_test.go
+++ b/test/e2e/tests/single_zone_e2e_test.go
@@ -1622,7 +1622,7 @@ var _ = Describe("GCE PD CSI Driver", func() {
 			stringPtr(provisionedIOPSOnCreateHdb),
 			stringPtr(provisionedThroughputOnCreateHdb),
 			stringPtr("3013"),
-			stringPtr("181"),
+			stringPtr("181Mi"),
 		),
 		Entry(
 			"for hyperdisk-extreme",
@@ -1640,7 +1640,7 @@ var _ = Describe("GCE PD CSI Driver", func() {
 			nil,
 			stringPtr(provisionedThroughputOnCreate),
 			nil,
-			stringPtr("70"),
+			stringPtr("70Mi"),
 		),
 	)
 })
diff --git a/test/k8s-integration/config/hdb-volumeattributesclass.yaml b/test/k8s-integration/config/hdb-volumeattributesclass.yaml
index f032912d..73d5ff4f 100644
--- a/test/k8s-integration/config/hdb-volumeattributesclass.yaml
+++ b/test/k8s-integration/config/hdb-volumeattributesclass.yaml
@@ -5,4 +5,4 @@ metadata:
 driverName: pd.csi.storage.gke.io
 parameters:
   iops: "3600"
-  throughput: "290"
\ No newline at end of file
+  throughput: "290Mi"
\ No newline at end of file