Merge branch 'master' into hyperdisk_slo_sli

Author: Sneha-at

Makefile

@@ -126,7 +126,7 @@ init-buildx:
 	$(DOCKER) run --rm --privileged multiarch/qemu-user-static --reset --credential yes --persistent yes
 	# Ensure we use a builder that can leverage it (the default on linux will not)
 	-$(DOCKER) buildx rm multiarch-multiplatform-builder
-	$(DOCKER) buildx create --use --name=multiarch-multiplatform-builder --driver-opt network=host --driver-opt image=moby/buildkit:v0.10.6
+	$(DOCKER) buildx create --use --name=multiarch-multiplatform-builder --driver-opt network=host --driver-opt image=moby/buildkit:v0.11.3
 	# Register gcloud as a Docker credential helper.
 	# Required for "docker buildx build --push".
 	gcloud auth configure-docker --quiet

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 ConvertMiStringToInt64(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
+}

pkg/common/utils_test.go

@@ -799,3 +799,57 @@ func TestConvertMiStringToInt64(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)
+			}
+		})
+	}
+}

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
 }

pkg/gce-pd-csi-driver/controller.go

@@ -16,6 +16,7 @@ package gceGCEDriver
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"regexp"
@@ -515,6 +516,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)
 
@@ -590,7 +600,14 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
 	}
 	err = gceCS.CloudProvider.AttachDisk(ctx, project, volKey, readWrite, attachableDiskTypePersistent, instanceZone, instanceName)
 	if err != nil {
-		// Emit metric for error
+		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)
+		}
+ 		// Emit metric for error
 		gceCS.Metrics.RecordOperationErrorMetrics("ControllerPublishVolume", err, diskToPublish.GetPDType())
 		return nil, status.Errorf(codes.Internal, "unknown Attach error: %v", err.Error())
 	}