Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver
Pull request #1534 (2023-12-08 19:15:30)

add enable_storage_pools label to OperationErrorMetric
<!--  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**:
Adds enable_storage_pools label to the OperationErrorMetric so we can track CSI operation error rates when storage pools is used. 
**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
Adds enable_storage_pools label to the OperationErrorMetric so we can track CSI operation error rates when storage pools is used. 
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1534#" title="Author self-approved">amacaskill</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fgcp-compute-persistent-disk-csi-driver).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/master/OWNERS)~~ [amacaskill]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
amacaskill: /assign @sunnylovestiramisu 

Patch:
diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go
index 5d0d4c70..fe20d0a7 100644
--- a/pkg/gce-pd-csi-driver/controller.go
+++ b/pkg/gce-pd-csi-driver/controller.go
@@ -215,8 +215,9 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("CreateVolume", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("CreateVolume", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	// Validate arguments
 	volumeCapabilities := req.GetVolumeCapabilities()
@@ -244,6 +245,8 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre
 	params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraVolumeLabels)
 	diskTypeForMetric = params.DiskType
 	enableConfidentialCompute = strconv.FormatBool(params.EnableConfidentialCompute)
+	hasStoragePools := len(params.StoragePools) > 0
+	enableStoragePools = strconv.FormatBool(hasStoragePools)
 	if err != nil {
 		return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err.Error())
 	}
@@ -457,8 +460,9 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("DeleteVolume", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("DeleteVolume", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	// Validate arguments
 	volumeID := req.GetVolumeId()
@@ -488,7 +492,7 @@ func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.Del
 	}
 	defer gceCS.volumeLocks.Release(volumeID)
 	disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
-	diskTypeForMetric, enableConfidentialCompute = metrics.GetMetricParameters(disk)
+	diskTypeForMetric, enableConfidentialCompute, enableStoragePools = metrics.GetMetricParameters(disk)
 	err = gceCS.CloudProvider.DeleteDisk(ctx, project, volKey)
 	if err != nil {
 		return nil, common.LoggedError("Failed to delete disk: ", err)
@@ -502,8 +506,9 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("ControllerPublishVolume", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("ControllerPublishVolume", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	// Only valid requests will be accepted
 	_, _, _, err = gceCS.validateControllerPublishVolumeRequest(ctx, req)
@@ -517,7 +522,7 @@ func (gceCS *GCEControllerServer) ControllerPublishVolume(ctx context.Context, r
 	}
 
 	resp, err, disk := gceCS.executeControllerPublishVolume(ctx, req)
-	diskTypeForMetric, enableConfidentialCompute = metrics.GetMetricParameters(disk)
+	diskTypeForMetric, enableConfidentialCompute, enableStoragePools = metrics.GetMetricParameters(disk)
 	if err != nil {
 		klog.Infof("For node %s adding backoff due to error for volume %s: %v", req.NodeId, req.VolumeId, err)
 		gceCS.errorBackoff.next(backoffId, common.CodeForError(err))
@@ -667,8 +672,9 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("ControllerUnpublishVolume", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("ControllerUnpublishVolume", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	_, _, err = gceCS.validateControllerUnpublishVolumeRequest(ctx, req)
 	if err != nil {
@@ -681,7 +687,7 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context,
 		return nil, status.Errorf(gceCS.errorBackoff.code(backoffId), "ControllerUnpublish not permitted on node %q due to backoff condition", req.NodeId)
 	}
 	resp, err, disk := gceCS.executeControllerUnpublishVolume(ctx, req)
-	diskTypeForMetric, enableConfidentialCompute = metrics.GetMetricParameters(disk)
+	diskTypeForMetric, enableConfidentialCompute, enableStoragePools = metrics.GetMetricParameters(disk)
 	if err != nil {
 		klog.Infof("For node %s adding backoff due to error for volume %s: %v", req.NodeId, req.VolumeId, err)
 		gceCS.errorBackoff.next(backoffId, common.CodeForError(err))
@@ -776,8 +782,9 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("ValidateVolumeCapabilities", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("ValidateVolumeCapabilities", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	if req.GetVolumeCapabilities() == nil || len(req.GetVolumeCapabilities()) == 0 {
 		return nil, status.Error(codes.InvalidArgument, "Volume Capabilities must be provided")
@@ -804,7 +811,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
 	defer gceCS.volumeLocks.Release(volumeID)
 
 	disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
-	diskTypeForMetric, enableConfidentialCompute = metrics.GetMetricParameters(disk)
+	diskTypeForMetric, enableConfidentialCompute, enableStoragePools = metrics.GetMetricParameters(disk)
 	if err != nil {
 		if gce.IsGCENotFoundError(err) {
 			return nil, status.Errorf(codes.NotFound, "Could not find disk %v: %v", volKey.Name, err.Error())
@@ -939,8 +946,9 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("CreateSnapshot", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("CreateSnapshot", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	// Validate arguments
 	volumeID := req.GetSourceVolumeId()
@@ -962,7 +970,7 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C
 
 	// Check if volume exists
 	disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
-	diskTypeForMetric, enableConfidentialCompute = metrics.GetMetricParameters(disk)
+	diskTypeForMetric, enableConfidentialCompute, enableStoragePools = metrics.GetMetricParameters(disk)
 	if err != nil {
 		if gce.IsGCENotFoundError(err) {
 			return nil, status.Errorf(codes.NotFound, "CreateSnapshot could not find disk %v: %v", volKey.String(), err.Error())
@@ -1191,8 +1199,9 @@ func (gceCS *GCEControllerServer) DeleteSnapshot(ctx context.Context, req *csi.D
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("DeleteSnapshot", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("DeleteSnapshot", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	// Validate arguments
 	snapshotID := req.GetSnapshotId()
@@ -1281,8 +1290,9 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
 	var err error
 	diskTypeForMetric := metrics.DefaultDiskTypeForMetric
 	enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute
+	enableStoragePools := metrics.DefaultEnableStoragePools
 	defer func() {
-		gceCS.Metrics.RecordOperationErrorMetrics("ControllerExpandVolume", err, diskTypeForMetric, enableConfidentialCompute)
+		gceCS.Metrics.RecordOperationErrorMetrics("ControllerExpandVolume", err, diskTypeForMetric, enableConfidentialCompute, enableStoragePools)
 	}()
 	volumeID := req.GetVolumeId()
 	if len(volumeID) == 0 {
@@ -1306,8 +1316,9 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
 		return nil, common.LoggedError("ControllerExpandVolume error repairing underspecified volume key: ", err)
 	}
 	sourceDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1)
-	diskTypeForMetric, enableConfidentialCompute = metrics.GetMetricParameters(sourceDisk)
+	diskTypeForMetric, enableConfidentialCompute, enableStoragePools = metrics.GetMetricParameters(sourceDisk)
 	resizedGb, err := gceCS.CloudProvider.ResizeDisk(ctx, project, volKey, reqBytes)
+
 	if err != nil {
 		return nil, common.LoggedError("ControllerExpandVolume failed to resize disk: ", err)
 	}
diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go
index a08f5074..d37c666f 100644
--- a/pkg/metrics/metrics.go
+++ b/pkg/metrics/metrics.go
@@ -36,6 +36,7 @@ const (
 	pdcsiDriverName                  = "pd.csi.storage.gke.io"
 	DefaultDiskTypeForMetric         = "unknownDiskType"
 	DefaultEnableConfidentialCompute = "unknownConfidentialMode"
+	DefaultEnableStoragePools        = "unknownStoragePools"
 )
 
 var (
@@ -52,7 +53,7 @@ var (
 			Help:           "CSI server side error metrics",
 			StabilityLevel: metrics.ALPHA,
 		},
-		[]string{"driver_name", "method_name", "grpc_status_code", "disk_type", "enable_confidential_storage"})
+		[]string{"driver_name", "method_name", "grpc_status_code", "disk_type", "enable_confidential_storage", "enable_storage_pools"})
 )
 
 type MetricsManager struct {
@@ -94,12 +95,13 @@ func (mm *MetricsManager) RecordOperationErrorMetrics(
 	operationName string,
 	operationErr error,
 	diskType string,
-	enableConfidentialStorage string) {
+	enableConfidentialStorage string,
+	enableStoragePools string) {
 	err := codes.OK.String()
 	if operationErr != nil {
 		err = common.CodeForError(operationErr).String()
 	}
-	pdcsiOperationErrorsMetric.WithLabelValues(pdcsiDriverName, "/csi.v1.Controller/"+operationName, err, diskType, enableConfidentialStorage).Inc()
+	pdcsiOperationErrorsMetric.WithLabelValues(pdcsiDriverName, "/csi.v1.Controller/"+operationName, err, diskType, enableConfidentialStorage, enableStoragePools).Inc()
 }
 
 func (mm *MetricsManager) EmitGKEComponentVersion() error {
@@ -156,12 +158,14 @@ func IsGKEComponentVersionAvailable() bool {
 	return true
 }
 
-func GetMetricParameters(disk *gce.CloudDisk) (string, string) {
+func GetMetricParameters(disk *gce.CloudDisk) (string, string, string) {
 	diskType := DefaultDiskTypeForMetric
 	enableConfidentialStorage := DefaultEnableConfidentialCompute
+	enableStoragePools := DefaultEnableStoragePools
 	if disk != nil {
 		diskType = disk.GetPDType()
 		enableConfidentialStorage = strconv.FormatBool(disk.GetEnableConfidentialCompute())
+		enableStoragePools = strconv.FormatBool(disk.GetEnableStoragePools())
 	}
-	return diskType, enableConfidentialStorage
+	return diskType, enableConfidentialStorage, enableStoragePools
 }


Test patch:
diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go
index dcf49092..238e02d8 100644
--- a/pkg/metrics/metrics_test.go
+++ b/pkg/metrics/metrics_test.go
@@ -20,6 +20,7 @@ package metrics
 import (
 	"testing"
 
+	computealpha "google.golang.org/api/compute/v0.alpha"
 	computebeta "google.golang.org/api/compute/v0.beta"
 	"google.golang.org/api/compute/v1"
 	gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute"
@@ -41,41 +42,76 @@ func CreateDiskWithConfidentialCompute(betaVersion bool, confidentialCompute boo
 	})
 }
 
-func TestGetEnableConfidentialCompute(t *testing.T) {
+func CreateDiskWithStoragePool(storagePool string, diskType string) *gce.CloudDisk {
+	return gce.CloudDiskFromAlpha(&computealpha.Disk{
+		StoragePool: storagePool,
+		Type:        diskType,
+	})
+}
+
+func TestGetMetricParameters(t *testing.T) {
 	testCases := []struct {
 		name                              string
 		disk                              *gce.CloudDisk
 		expectedEnableConfidentialCompute string
 		expectedDiskType                  string
+		expectedEnableStoragePools        string
 	}{
 		{
 			name:                              "test betaDisk with enableConfidentialCompute=false",
 			disk:                              CreateDiskWithConfidentialCompute(true, false, hyperdiskBalanced),
 			expectedEnableConfidentialCompute: "false",
 			expectedDiskType:                  hyperdiskBalanced,
+			expectedEnableStoragePools:        "false",
 		},
 		{
 			name:                              "test betaDisk with enableConfidentialCompute=true",
 			disk:                              CreateDiskWithConfidentialCompute(true, true, hyperdiskBalanced),
 			expectedEnableConfidentialCompute: "true",
 			expectedDiskType:                  hyperdiskBalanced,
+			expectedEnableStoragePools:        "false",
 		},
 		{
-			name:                              "test disk withpit enableConfidentialCompute",
+			name:                              "test disk without enableConfidentialCompute",
 			disk:                              CreateDiskWithConfidentialCompute(false, false, hyperdiskBalanced),
 			expectedEnableConfidentialCompute: "false",
 			expectedDiskType:                  hyperdiskBalanced,
+			expectedEnableStoragePools:        "false",
+		},
+		{
+			name:                              "test alphaDisk with storage pool projects/my-project/zone/us-central1-a/storagePools/sp1",
+			disk:                              CreateDiskWithStoragePool("projects/my-project/zone/us-central1-a/storagePools/sp1", hyperdiskBalanced),
+			expectedEnableConfidentialCompute: "false",
+			expectedDiskType:                  hyperdiskBalanced,
+			expectedEnableStoragePools:        "true",
+		},
+		{
+			name:                              "test alphaDisk with no storage pool",
+			disk:                              CreateDiskWithStoragePool("", hyperdiskBalanced),
+			expectedEnableConfidentialCompute: "false",
+			expectedDiskType:                  hyperdiskBalanced,
+			expectedEnableStoragePools:        "false",
+		},
+		{
+			name:                              "test nil disk",
+			disk:                              nil,
+			expectedEnableConfidentialCompute: DefaultEnableConfidentialCompute,
+			expectedDiskType:                  DefaultDiskTypeForMetric,
+			expectedEnableStoragePools:        DefaultEnableStoragePools,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Logf("Running test: %v", tc.name)
-		diskType, confidentialCompute := GetMetricParameters(tc.disk)
+		diskType, confidentialCompute, enableStoragePools := GetMetricParameters(tc.disk)
 		if confidentialCompute != tc.expectedEnableConfidentialCompute {
-			t.Fatalf("Got confidentialCompute value %v expected %v", confidentialCompute, tc.expectedEnableConfidentialCompute)
+			t.Fatalf("Got confidentialCompute value %q expected %q", confidentialCompute, tc.expectedEnableConfidentialCompute)
 		}
 		if diskType != tc.expectedDiskType {
-			t.Fatalf("Got confidentialCompute value %v expected %v", diskType, tc.expectedDiskType)
+			t.Fatalf("Got diskType value %q expected %q", diskType, tc.expectedDiskType)
+		}
+		if enableStoragePools != tc.expectedEnableStoragePools {
+			t.Fatalf("Got enableStoragePools value %q expected %q", enableStoragePools, tc.expectedEnableStoragePools)
 		}
 	}
 }