From 661abc8a7c8dc2b2bb68f930e532327aa4332110 Mon Sep 17 00:00:00 2001 From: Peter Schuurman Date: Mon, 24 Apr 2023 23:27:44 -0700 Subject: [PATCH 1/4] Rewrite PD CSI error message when encountering diskType unsupported error --- pkg/common/utils.go | 20 +++++++ pkg/common/utils_test.go | 54 +++++++++++++++++++ pkg/gce-cloud-provider/compute/gce-compute.go | 27 +++++++++- pkg/gce-pd-csi-driver/controller.go | 17 ++++++ 4 files changed, 117 insertions(+), 1 deletion(-) diff --git a/pkg/common/utils.go b/pkg/common/utils.go index d5842ea36..3041f1817 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) } } + +// ParseMachineTypeFromUrl 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 ParseMachineFamily(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 strings.Split(machineType[1], "-")[0], nil +} diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index ca435caf3..137b47ea3 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -577,3 +577,57 @@ func TestSnapshotStorageLocations(t *testing.T) { }) } } + +func TestParseMachineFamily(t *testing.T) { + tests := []struct { + desc string + inputMachineTypeUrl string + expectedMachineFamily string + expectError bool + }{ + { + desc: "full URL machine family", + inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/machineTypes/c3-highcpu-4", + expectedMachineFamily: "c3", + }, + { + desc: "partial URL machine family", + inputMachineTypeUrl: "zones/us-central1-c/machineTypes/n2-standard-4", + expectedMachineFamily: "n2", + }, + { + desc: "custom partial URL machine family", + inputMachineTypeUrl: "zones/us-central1-c/machineTypes/e2-custom-2-4096", + expectedMachineFamily: "e2", + }, + { + 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 := ParseMachineFamily(tc.inputMachineTypeUrl) + if err != nil && !tc.expectError { + t.Errorf("Got error %v parsing machine family %s; expect no error", err, tc.inputMachineTypeUrl) + } + if err == nil && tc.expectError { + t.Errorf("Got no error parsing machine family %s; expect an error", tc.inputMachineTypeUrl) + } + if err == nil && actualMachineFamily != tc.expectedMachineFamily { + t.Errorf("Got %s parsing machine family; expect %s", actualMachineFamily, tc.expectedMachineFamily) + } + }) + } +} diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index 6cbb6df0b..6e822fe83 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..05eaca04c 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 parseMachineFamily(machineTypeUrl string) string { + machineFamily, parseErr := common.ParseMachineFamily(machineTypeUrl) + if parseErr != nil { + // Parse errors represent an unexpected API change with instance.MachineType; log a warning. + klog.Warningf("ParseMachineFamily(%v): %v", machineTypeUrl, parseErr) + } + return machineFamily +} + 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. + machineFamily := parseMachineFamily(instance.MachineType) + return nil, status.Errorf(codes.Internal, "'%s' is not a compatible disk type with the machine family '%s', please review the GKE online documentation for available persistent disk options", udErr.DiskType, machineFamily) + } return nil, status.Errorf(codes.Internal, "unknown Attach error: %v", err.Error()) } From 6a3d0ac1d6c8935c49a688f205844eb157f93fb0 Mon Sep 17 00:00:00 2001 From: Peter Schuurman Date: Tue, 25 Apr 2023 11:49:54 -0700 Subject: [PATCH 2/4] Update error message for disk compatibility with machine family --- pkg/gce-pd-csi-driver/controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 05eaca04c..8bb406b25 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -540,7 +540,7 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con // 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. machineFamily := parseMachineFamily(instance.MachineType) - return nil, status.Errorf(codes.Internal, "'%s' is not a compatible disk type with the machine family '%s', please review the GKE online documentation for available persistent disk options", udErr.DiskType, machineFamily) + return nil, status.Errorf(codes.InvalidArgument, "'%s' is not a compatible disk type with the machine family '%s', please review the GCP online documentation for available persistent disk options", udErr.DiskType, machineFamily) } return nil, status.Errorf(codes.Internal, "unknown Attach error: %v", err.Error()) } From f3a9051d6e4dbbfec52658cbc024c430b08d07a8 Mon Sep 17 00:00:00 2001 From: Peter Schuurman Date: Tue, 25 Apr 2023 16:20:16 -0700 Subject: [PATCH 3/4] Fix linter warnings in gce-compute.go --- pkg/gce-cloud-provider/compute/gce-compute.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index 6e822fe83..c2193ae18 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -40,7 +40,7 @@ const ( 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` + pdDiskTypeUnsupportedPattern = `\[([a-z-]+)\] features are not compatible for creating instance` ) var pdDiskTypeUnsupportedRegex = regexp.MustCompile(pdDiskTypeUnsupportedPattern) From bfb51399b9c4c9062e4c409dcaa66ea13b206e26 Mon Sep 17 00:00:00 2001 From: Peter Schuurman Date: Wed, 10 May 2023 09:45:11 -0700 Subject: [PATCH 4/4] Print machineType, rather than just the machine family when encountering an incompatible disk attach --- pkg/common/utils.go | 8 +++--- pkg/common/utils_test.go | 38 ++++++++++++++--------------- pkg/gce-pd-csi-driver/controller.go | 12 ++++----- 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/pkg/common/utils.go b/pkg/common/utils.go index 3041f1817..18b7baf55 100644 --- a/pkg/common/utils.go +++ b/pkg/common/utils.go @@ -257,14 +257,14 @@ func ValidateSnapshotType(snapshotType string) error { } } -// ParseMachineTypeFromUrl returns an extracted machineType from a URL, or empty if not found. +// 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 ParseMachineFamily(machineTypeUrl string) (string, error) { +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 "", fmt.Errorf("failed to parse machineTypeUrl. Expected suffix: zones/{zone}/machineTypes/{machine-type}. Got: %s", machineTypeUrl) } - return strings.Split(machineType[1], "-")[0], nil + return machineType[1], nil } diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index 137b47ea3..bee84ccec 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -578,27 +578,27 @@ func TestSnapshotStorageLocations(t *testing.T) { } } -func TestParseMachineFamily(t *testing.T) { +func TestParseMachineType(t *testing.T) { tests := []struct { - desc string - inputMachineTypeUrl string - expectedMachineFamily string - expectError bool + desc string + inputMachineTypeUrl string + expectedMachineType string + expectError bool }{ { - desc: "full URL machine family", - inputMachineTypeUrl: "https://www.googleapis.com/compute/v1/projects/my-project/zones/us-central1-c/machineTypes/c3-highcpu-4", - expectedMachineFamily: "c3", + 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 family", - inputMachineTypeUrl: "zones/us-central1-c/machineTypes/n2-standard-4", - expectedMachineFamily: "n2", + desc: "partial URL machine type", + inputMachineTypeUrl: "zones/us-central1-c/machineTypes/n2-standard-4", + expectedMachineType: "n2-standard-4", }, { - desc: "custom partial URL machine family", - inputMachineTypeUrl: "zones/us-central1-c/machineTypes/e2-custom-2-4096", - expectedMachineFamily: "e2", + desc: "custom partial URL machine type", + inputMachineTypeUrl: "zones/us-central1-c/machineTypes/e2-custom-2-4096", + expectedMachineType: "e2-custom-2-4096", }, { desc: "incorrect URL", @@ -618,15 +618,15 @@ func TestParseMachineFamily(t *testing.T) { } for _, tc := range tests { t.Run(tc.desc, func(t *testing.T) { - actualMachineFamily, err := ParseMachineFamily(tc.inputMachineTypeUrl) + actualMachineFamily, err := ParseMachineType(tc.inputMachineTypeUrl) if err != nil && !tc.expectError { - t.Errorf("Got error %v parsing machine family %s; expect no error", err, tc.inputMachineTypeUrl) + 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 family %s; expect an error", tc.inputMachineTypeUrl) + t.Errorf("Got no error parsing machine type %s; expect an error", tc.inputMachineTypeUrl) } - if err == nil && actualMachineFamily != tc.expectedMachineFamily { - t.Errorf("Got %s parsing machine family; expect %s", actualMachineFamily, tc.expectedMachineFamily) + if err == nil && actualMachineFamily != tc.expectedMachineType { + t.Errorf("Got %s parsing machine type; expect %s", actualMachineFamily, tc.expectedMachineType) } }) } diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 8bb406b25..5d6b9579b 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -450,13 +450,13 @@ func (gceCS *GCEControllerServer) validateControllerPublishVolumeRequest(ctx con return project, volKey, nil } -func parseMachineFamily(machineTypeUrl string) string { - machineFamily, parseErr := common.ParseMachineFamily(machineTypeUrl) +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("ParseMachineFamily(%v): %v", machineTypeUrl, parseErr) + klog.Warningf("ParseMachineType(%v): %v", machineTypeUrl, parseErr) } - return machineFamily + return machineType } func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Context, req *csi.ControllerPublishVolumeRequest) (*csi.ControllerPublishVolumeResponse, error) { @@ -539,8 +539,8 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con 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. - machineFamily := parseMachineFamily(instance.MachineType) - return nil, status.Errorf(codes.InvalidArgument, "'%s' is not a compatible disk type with the machine family '%s', please review the GCP online documentation for available persistent disk options", udErr.DiskType, machineFamily) + 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()) }