From e8d43fb5dc4e5472d504b24dea746aff6b22adbf Mon Sep 17 00:00:00 2001 From: chrisThePattyEater Date: Fri, 27 Jun 2025 02:31:09 +0000 Subject: [PATCH 1/2] Added filtering for incorrect slo boosting logs --- pkg/gce-cloud-provider/compute/gce.go | 16 +++++++ pkg/gce-cloud-provider/compute/gce_test.go | 54 ++++++++++++++++++++++ pkg/gce-pd-csi-driver/controller.go | 10 ++++ 3 files changed, 80 insertions(+) diff --git a/pkg/gce-cloud-provider/compute/gce.go b/pkg/gce-cloud-provider/compute/gce.go index bde9b64f0..fae0c1bdc 100644 --- a/pkg/gce-cloud-provider/compute/gce.go +++ b/pkg/gce-cloud-provider/compute/gce.go @@ -21,6 +21,7 @@ import ( "net/http" "net/url" "os" + "regexp" "runtime" "sync" "time" @@ -77,6 +78,11 @@ const ( gcpTagsRequestTokenBucketSize = 8 ) +var ( + ssAlreadyExistsRegex = regexp.MustCompile("The resource [^']+ already exists") + gcpViolationRegex = regexp.MustCompile("violates constraint constraints/gcp.") +) + // ResourceType indicates the type of a compute resource. type ResourceType string @@ -441,3 +447,13 @@ func IsGCENotFoundError(err error) bool { func IsGCEInvalidError(err error) bool { return IsGCEError(err, "invalid") } + +// IsSnapshotAlreadyExistsError checks if the error is a snapshot already exists error. +func IsSnapshotAlreadyExistsError(err error) bool { + return ssAlreadyExistsRegex.MatchString(err.Error()) +} + +// IsGCPOrgViolationError checks if the error is a GCP organization policy violation error. +func IsGCPOrgViolationError(err error) bool { + return gcpViolationRegex.MatchString(err.Error()) +} diff --git a/pkg/gce-cloud-provider/compute/gce_test.go b/pkg/gce-cloud-provider/compute/gce_test.go index 94bb4fd9f..f7f2e0e6e 100644 --- a/pkg/gce-cloud-provider/compute/gce_test.go +++ b/pkg/gce-cloud-provider/compute/gce_test.go @@ -101,6 +101,60 @@ func TestIsGCEError(t *testing.T) { } } +func TestErrorIsGCPViolationRegex(t *testing.T) { + testCases := []struct { + name string + inputErr error + expectedResult bool + }{ + { + name: "is gcp org violation error", + inputErr: errors.New("Your api request violates constraint constraints/gcp.resourceLocations"), + expectedResult: true, + }, + { + name: "is not gcp org violation error", + inputErr: errors.New("Some incorrect error message"), + expectedResult: false, + }, + } + + for _, tc := range testCases { + t.Logf("Running test: %v", tc.name) + result := IsGCPOrgViolationError(tc.inputErr) + if tc.expectedResult != result { + t.Fatalf("Got '%t', expected '%t'", result, tc.expectedResult) + } + } +} + +func TestErrorIsSnapshotExistsError(t *testing.T) { + testCases := []struct { + name string + inputErr error + expectedResult bool + }{ + { + name: "is ss error", + inputErr: errors.New("The resource projects/dcme-pre-opt-mdp-rmp-00/global/snapshots/snapshot-3c208602-d815-40ae-a61e-3259e3bd29ca already exists, alreadyExists"), + expectedResult: true, + }, + { + name: "is not ss already exists error", + inputErr: errors.New("Some incorrect error message"), + expectedResult: false, + }, + } + + for _, tc := range testCases { + t.Logf("Running test: %v", tc.name) + result := IsSnapshotAlreadyExistsError(tc.inputErr) + if tc.expectedResult != result { + t.Fatalf("Got '%t', expected '%t'", result, tc.expectedResult) + } + } +} + func TestGetComputeVersion(t *testing.T) { testCases := []struct { name string diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index f0523d435..f77d0982b 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -1668,6 +1668,16 @@ func (gceCS *GCEControllerServer) createPDSnapshot(ctx context.Context, project if gce.IsGCEError(err, "notFound") { return nil, status.Errorf(codes.NotFound, "Could not find volume with ID %v: %v", volKey.String(), err.Error()) } + + // Identified as incorrect error handling + if gce.IsSnapshotAlreadyExistsError(err) { + return nil, status.Errorf(codes.AlreadyExists, "Snapshot already exists: %v", err.Error()) + } + + // Identified as incorrect error handling + if gce.IsGCPOrgViolationError(err) { + return nil, status.Errorf(codes.FailedPrecondition, "Violates GCP org policy: %v", err.Error()) + } return nil, common.LoggedError("Failed to create snapshot: ", err) } } From 7ed2e9e9ddc2e00ad2c3ae3ebaac4000d6f129b0 Mon Sep 17 00:00:00 2001 From: dannawang Date: Tue, 15 Jul 2025 22:24:39 +0000 Subject: [PATCH 2/2] Update org policy violation check --- pkg/gce-cloud-provider/compute/gce.go | 15 ++++++++++++--- pkg/gce-cloud-provider/compute/gce_test.go | 20 +++++++++++++++----- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/pkg/gce-cloud-provider/compute/gce.go b/pkg/gce-cloud-provider/compute/gce.go index fae0c1bdc..e9c4d0920 100644 --- a/pkg/gce-cloud-provider/compute/gce.go +++ b/pkg/gce-cloud-provider/compute/gce.go @@ -79,8 +79,9 @@ const ( ) var ( - ssAlreadyExistsRegex = regexp.MustCompile("The resource [^']+ already exists") - gcpViolationRegex = regexp.MustCompile("violates constraint constraints/gcp.") + ssAlreadyExistsRegex = regexp.MustCompile("The resource [^']+ already exists") + gcpErrorCodeRegex = regexp.MustCompile(`Error (\d+)`) + orgPolicyConstraintErrorCodes = []string{"400", "412"} ) // ResourceType indicates the type of a compute resource. @@ -455,5 +456,13 @@ func IsSnapshotAlreadyExistsError(err error) bool { // IsGCPOrgViolationError checks if the error is a GCP organization policy violation error. func IsGCPOrgViolationError(err error) bool { - return gcpViolationRegex.MatchString(err.Error()) + matches := gcpErrorCodeRegex.FindStringSubmatch(err.Error()) + if len(matches) > 1 { + errorCode := matches[1] + if slices.Contains(orgPolicyConstraintErrorCodes, errorCode) { + return true + } + } + + return false } diff --git a/pkg/gce-cloud-provider/compute/gce_test.go b/pkg/gce-cloud-provider/compute/gce_test.go index f7f2e0e6e..3cd25addb 100644 --- a/pkg/gce-cloud-provider/compute/gce_test.go +++ b/pkg/gce-cloud-provider/compute/gce_test.go @@ -108,10 +108,20 @@ func TestErrorIsGCPViolationRegex(t *testing.T) { expectedResult bool }{ { - name: "is gcp org violation error", - inputErr: errors.New("Your api request violates constraint constraints/gcp.resourceLocations"), + name: "is gcp org violation error, error code 400", + inputErr: errors.New("Failed to scale up: googleapi: Error 400: 'us-central1' violates constraint '`constraints/gcp.resourceLocations`' on the resource 'projects/test-project/locations/us-central1/clusters/test-cluster/nodePools/test-node-pool'"), expectedResult: true, }, + { + name: "is gcp org violation error, error code 412", + inputErr: errors.New("createSnapshot for content [snapcontent-xyz]: error occurred in createSnapshotWrapper: failed to take snapshot of the volume projects/test-project/regions/europe-west3/disks/pvc-test: \"rpc error: code = Internal desc = Failed to create snapshot: googleapi: Error 412: Location EU violates constraint constraints/gcp.resourceLocations on the resource projects/test-project/global/snapshots/snapshot-xyz., conditionNotMet\""), + expectedResult: true, + }, + { + name: "is not gcp org violation, error doesn't match", + inputErr: errors.New("createSnapshot for content [snapcontent-xyz]: error occurred in createSnapshotWrapper: failed to take snapshot of the volume projects/test-project/regions/europe-west3/disks/pvc-test: \"rpc error: code = Internal desc = Failed to create snapshot: googleapi: Error 500: Location EU violates constraint constraints/gcp.resourceLocations on the resource projects/test-project/global/snapshots/snapshot-xyz., conditionNotMet\""), + expectedResult: false, + }, { name: "is not gcp org violation error", inputErr: errors.New("Some incorrect error message"), @@ -135,12 +145,12 @@ func TestErrorIsSnapshotExistsError(t *testing.T) { expectedResult bool }{ { - name: "is ss error", - inputErr: errors.New("The resource projects/dcme-pre-opt-mdp-rmp-00/global/snapshots/snapshot-3c208602-d815-40ae-a61e-3259e3bd29ca already exists, alreadyExists"), + name: "is snapshot already exists error", + inputErr: errors.New("The resource projects/test-project/global/snapshots/snapshot-xyz already exists, alreadyExists"), expectedResult: true, }, { - name: "is not ss already exists error", + name: "is not snapshot already exists error", inputErr: errors.New("Some incorrect error message"), expectedResult: false, },