Skip to content

Added filtering for incorrect slo boosting logs #2118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions pkg/gce-cloud-provider/compute/gce.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"net/http"
"net/url"
"os"
"regexp"
"runtime"
"sync"
"time"
Expand Down Expand Up @@ -77,6 +78,12 @@ const (
gcpTagsRequestTokenBucketSize = 8
)

var (
ssAlreadyExistsRegex = regexp.MustCompile("The resource [^']+ already exists")
gcpErrorCodeRegex = regexp.MustCompile(`Error (\d+)`)
orgPolicyConstraintErrorCodes = []string{"400", "412"}
)

// ResourceType indicates the type of a compute resource.
type ResourceType string

Expand Down Expand Up @@ -441,3 +448,21 @@ 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 {
matches := gcpErrorCodeRegex.FindStringSubmatch(err.Error())
if len(matches) > 1 {
errorCode := matches[1]
if slices.Contains(orgPolicyConstraintErrorCodes, errorCode) {
return true
}
}

return false
}
64 changes: 64 additions & 0 deletions pkg/gce-cloud-provider/compute/gce_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,70 @@ func TestIsGCEError(t *testing.T) {
}
}

func TestErrorIsGCPViolationRegex(t *testing.T) {
testCases := []struct {
name string
inputErr error
expectedResult bool
}{
{
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"),
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 snapshot already exists error",
inputErr: errors.New("The resource projects/test-project/global/snapshots/snapshot-xyz already exists, alreadyExists"),
expectedResult: true,
},
{
name: "is not snapshot 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
Expand Down
10 changes: 10 additions & 0 deletions pkg/gce-pd-csi-driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down