Repository: kubernetes-sigs/gcp-compute-persistent-disk-csi-driver
Pull request #1222 (2023-05-12 22:16:44)

Automated cherry pick of #1212: Rewrite PD CSI error message when encountering diskType
Cherry pick of #1212 on release-1.9.

#1212: Rewrite PD CSI error message when encountering diskType

For details on the cherry pick process, see the [cherry pick requests](https://git.k8s.io/community/contributors/devel/sig-release/cherry-picks.md) page.

```release-note
Updates error message to be more user friendly when PD CSI Driver encounters an disk type UNSUPPORTED_OPERATION on ControllerPublishVolume
```

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

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/1222#" title="Author self-approved">pwschuurman</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign msau42 for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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).

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

- **[OWNERS](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/blob/release-1.9/OWNERS)**

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":["msau42"]} -->
k8s-ci-robot: @pwschuurman: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-gcp-compute-persistent-disk-csi-driver-unit | 85f2565146c1f99bd4e219d6ad70dd0fa3ec6878 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-compute-persistent-disk-csi-driver/1222/pull-gcp-compute-persistent-disk-csi-driver-unit/1657147832206888960/) | true | `/test pull-gcp-compute-persistent-disk-csi-driver-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=gcp-compute-persistent-disk-csi-driver&pr=1222). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Apwschuurman). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @pwschuurman: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-gcp-compute-persistent-disk-csi-driver-unit | 85f2565146c1f99bd4e219d6ad70dd0fa3ec6878 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-compute-persistent-disk-csi-driver/1222/pull-gcp-compute-persistent-disk-csi-driver-unit/1657147832206888960/) | true | `/test pull-gcp-compute-persistent-disk-csi-driver-unit`
pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration | 85f2565146c1f99bd4e219d6ad70dd0fa3ec6878 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-compute-persistent-disk-csi-driver/1222/pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration/1657147832303357952/) | true | `/test pull-gcp-compute-persistent-disk-csi-driver-kubernetes-integration`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=gcp-compute-persistent-disk-csi-driver&pr=1222). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Apwschuurman). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/common/utils.go b/pkg/common/utils.go
index 86cc3f9d..3f2d9830 100644
--- a/pkg/common/utils.go
+++ b/pkg/common/utils.go
@@ -59,11 +59,19 @@ const (
 	multiRegionalLocationFmt = "^[a-z]+$"
 	// Example: us-east1
 	regionalLocationFmt = "^[a-z]+-[a-z]+[0-9]$"
+
+	// Full or partial URL of the machine type resource, in the format:
+	//   zones/zone/machineTypes/machine-type
+	machineTypePattern = "zones/[^/]+/machineTypes/([^/]+)$"
 )
 
 var (
 	multiRegionalPattern = regexp.MustCompile(multiRegionalLocationFmt)
 	regionalPattern      = regexp.MustCompile(regionalLocationFmt)
+
+	// Full or partial URL of the machine type resource, in the format:
+	//   zones/zone/machineTypes/machine-type
+	machineTypeRegex = regexp.MustCompile(machineTypePattern)
 )
 
 func BytesToGbRoundDown(bytes int64) int64 {
@@ -268,3 +276,15 @@ func ConvertMiBStringToInt64(str string) (int64, error) {
 	}
 	return volumehelpers.RoundUpToMiB(quantity)
 }
+
+// ParseMachineType returns an extracted machineType from a URL, or empty if not found.
+// machineTypeUrl: Full or partial URL of the machine type resource, in the format:
+//
+//	zones/zone/machineTypes/machine-type
+func ParseMachineType(machineTypeUrl string) (string, error) {
+	machineType := machineTypeRegex.FindStringSubmatch(machineTypeUrl)
+	if machineType == nil {
+		return "", fmt.Errorf("failed to parse machineTypeUrl. Expected suffix: zones/{zone}/machineTypes/{machine-type}. Got: %s", machineTypeUrl)
+	}
+	return machineType[1], nil
+}
diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go
index 0be8356d..6631855a 100644
--- a/pkg/gce-cloud-provider/compute/gce-compute.go
+++ b/pkg/gce-cloud-provider/compute/gce-compute.go
@@ -18,6 +18,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"regexp"
 	"strings"
 	"time"
 
@@ -38,9 +39,12 @@ const (
 	waitForImageCreationTimeOut    = 5 * time.Minute
 	diskKind                       = "compute#disk"
 	cryptoKeyVerDelimiter          = "/cryptoKeyVersions"
+	// Example message: "[pd-standard] features are not compatible for creating instance"
+	pdDiskTypeUnsupportedPattern = `\[([a-z-]+)\] features are not compatible for creating instance`
 )
 
 var hyperdiskTypes = []string{"hyperdisk-extreme", "hyperdisk-throughput"}
+var pdDiskTypeUnsupportedRegex = regexp.MustCompile(pdDiskTypeUnsupportedPattern)
 
 type GCEAPIVersion string
 
@@ -69,6 +73,15 @@ var WaitForOpBackoff = wait.Backoff{
 	Steps:    100,
 	Cap:      0}
 
+// Custom error type to propagate error messages up to clients.
+type UnsupportedDiskError struct {
+	DiskType string
+}
+
+func (udErr *UnsupportedDiskError) Error() string {
+	return ""
+}
+
 type GCECompute interface {
 	// Metadata information
 	GetDefaultProject() string
@@ -838,12 +851,23 @@ func (cloud *CloudProvider) WaitForAttach(ctx context.Context, project string, v
 	})
 }
 
+func wrapOpErr(name string, opErr *computev1.OperationErrorErrors) error {
+	if opErr.Code == "UNSUPPORTED_OPERATION" {
+		if diskType := pdDiskTypeUnsupportedRegex.FindStringSubmatch(opErr.Message); diskType != nil {
+			return &UnsupportedDiskError{
+				DiskType: diskType[1],
+			}
+		}
+	}
+	return fmt.Errorf("operation %v failed (%v): %v", name, opErr.Code, opErr.Message)
+}
+
 func opIsDone(op *computev1.Operation) (bool, error) {
 	if op == nil || op.Status != operationStatusDone {
 		return false, nil
 	}
 	if op.Error != nil && len(op.Error.Errors) > 0 && op.Error.Errors[0] != nil {
-		return true, fmt.Errorf("operation %v failed (%v): %v", op.Name, op.Error.Errors[0].Code, op.Error.Errors[0].Message)
+		return true, wrapOpErr(op.Name, op.Error.Errors[0])
 	}
 	return true, nil
 }
diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go
index c9f3fb60..4830aa23 100644
--- a/pkg/gce-pd-csi-driver/controller.go
+++ b/pkg/gce-pd-csi-driver/controller.go
@@ -16,6 +16,7 @@ package gceGCEDriver
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"regexp"
@@ -449,6 +450,15 @@ func (gceCS *GCEControllerServer) validateControllerPublishVolumeRequest(ctx con
 	return project, volKey, nil
 }
 
+func parseMachineType(machineTypeUrl string) string {
+	machineType, parseErr := common.ParseMachineType(machineTypeUrl)
+	if parseErr != nil {
+		// Parse errors represent an unexpected API change with instance.MachineType; log a warning.
+		klog.Warningf("ParseMachineType(%v): %v", machineTypeUrl, parseErr)
+	}
+	return machineType
+}
+
 func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) {
 	project, volKey, err := gceCS.validateControllerPublishVolumeRequest(ctx, req)
 
@@ -525,6 +535,13 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
 	}
 	err = gceCS.CloudProvider.AttachDisk(ctx, project, volKey, readWrite, attachableDiskTypePersistent, instanceZone, instanceName)
 	if err != nil {
+		var udErr *gce.UnsupportedDiskError
+		if errors.As(err, &udErr) {
+			// If we encountered an UnsupportedDiskError, rewrite the error message to be more user friendly.
+			// The error message from GCE is phrased around disk create on VM creation, not runtime attach.
+			machineType := parseMachineType(instance.MachineType)
+			return nil, status.Errorf(codes.InvalidArgument, "'%s' is not a compatible disk type with the machine type %s, please review the GCP online documentation for available persistent disk options", udErr.DiskType, machineType)
+		}
 		return nil, status.Errorf(codes.Internal, "unknown Attach error: %v", err.Error())
 	}
 


Test patch:
diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go
index b08aee54..a888465d 100644
--- a/pkg/common/utils_test.go
+++ b/pkg/common/utils_test.go
@@ -787,3 +787,57 @@ func TestConvertMiBStringToInt64(t *testing.T) {
 		})
 	}
 }
+
+func TestParseMachineType(t *testing.T) {
+	tests := []struct {
+		desc                string
+		inputMachineTypeUrl string
+		expectedMachineType string
+		expectError         bool
+	}{
+		{
+			desc:                "full URL machine type",
+			inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/machineTypes/c3-highcpu-4",
+			expectedMachineType: "c3-highcpu-4",
+		},
+		{
+			desc:                "partial URL machine type",
+			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/n2-standard-4",
+			expectedMachineType: "n2-standard-4",
+		},
+		{
+			desc:                "custom partial URL machine type",
+			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/e2-custom-2-4096",
+			expectedMachineType: "e2-custom-2-4096",
+		},
+		{
+			desc:                "incorrect URL",
+			inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/psch-gke-dev/zones/us-central1-c",
+			expectError:         true,
+		},
+		{
+			desc:                "incorrect partial URL",
+			inputMachineTypeUrl: "zones/us-central1-c/machineTypes/",
+			expectError:         true,
+		},
+		{
+			desc:                "missing zone",
+			inputMachineTypeUrl: "zones//machineTypes/n2-standard-4",
+			expectError:         true,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.desc, func(t *testing.T) {
+			actualMachineFamily, err := ParseMachineType(tc.inputMachineTypeUrl)
+			if err != nil && !tc.expectError {
+				t.Errorf("Got error %v parsing machine type %s; expect no error", err, tc.inputMachineTypeUrl)
+			}
+			if err == nil && tc.expectError {
+				t.Errorf("Got no error parsing machine type %s; expect an error", tc.inputMachineTypeUrl)
+			}
+			if err == nil && actualMachineFamily != tc.expectedMachineType {
+				t.Errorf("Got %s parsing machine type; expect %s", actualMachineFamily, tc.expectedMachineType)
+			}
+		})
+	}
+}