diff --git a/pkg/common/utils.go b/pkg/common/utils.go index d5842ea36..18b7baf55 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -57,11 +57,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 { @@ -248,3 +256,15 @@ func ValidateSnapshotType(snapshotType string) error { return fmt.Errorf("invalid snapshot type %s", snapshotType) } } + +// 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/common/utils_test.go b/pkg/common/utils_test.go index ca435caf3..bee84ccec 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -577,3 +577,57 @@ func TestSnapshotStorageLocations(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) + } + }) + } +} diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index 6cbb6df0b..c2193ae18 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,8 +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 pdDiskTypeUnsupportedRegex = regexp.MustCompile(pdDiskTypeUnsupportedPattern) + type GCEAPIVersion string const ( @@ -67,6 +72,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 @@ -830,12 +844,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 126a0d1e9..5d6b9579b 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" "sort" @@ -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()) }