Skip to content

Filter out GCE operation error codes caused by user errors #1261

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 11, 2023
Merged

Filter out GCE operation error codes caused by user errors #1261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 27 additions & 1 deletion pkg/gce-cloud-provider/compute/gce-compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,14 +856,40 @@ func (cloud *CloudProvider) WaitForAttach(ctx context.Context, project string, v
}

func wrapOpErr(name string, opErr *computev1.OperationErrorErrors) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what about error handling for cases where the operation is not even initiated, and we encounter error in say insertDisk, attachDisk where the WaitForOp is not called. e.g code

Also we may need to handle uncastable error (i.e errors.As cannot case to gce.UnsupportedDiskError) like the ones we saw in filestore. May need some manual testing here (1. insert disk when quota is exhausted, 2. attachdisk when disk already attached) or we can address them lazily if we encounter them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have looked into quiet a few clusters, and I haven't seen any errors that are incorrectly returned as an Internal error at this point ( where the operation is not even initiated). I will look into some more clusters, but I have probably looked into at least 20 at this point, and I haven't seen errors like that.

Also we may need to handle uncastable error (i.e errors.As cannot cast to gce.UnsupportedDiskError) like the ones we saw in filestore.

I'm a little confused by this comment. Are you saying we could be missing some cases where we need to return an UnsupportedDiskError (which we do when the operation error has the UNSUPPORTED_OPERATION code). This won't happen like in filestore because wrapOpErr passes in an operation error of type computev1.OperationErrorErrors directly. This error type doesn't actually implement the error interface, so this will never happen. (errors.As cannot even be run on the operation error of type computev1.OperationErrorErrors since it doesn't implement error interface).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. I missed the OperationErrorErrors type.

And yes, for unsupported operations, I think we would return kIternal for the UnsupportedDiskError. Until we repro this scenario, it would be difficult to handle this error. So that would be a followup for future to understand the sceario better.

As discussed, lets capture the known issues in some task:

  1. AttachDisk errors not handled by errors.As() code
  2. InsertDisk, DeleteDisk, DetachDisk errors are not handled.

if opErr == nil {
return nil
}

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)
grpcErrCode := codeForGCEOpError(*opErr)
return status.Errorf(grpcErrCode, "operation %v failed (%v): %v", name, opErr.Code, opErr.Message)
}

// codeForGCEOpError return the grpc error code for the passed in
// gce operation error.
func codeForGCEOpError(err computev1.OperationErrorErrors) codes.Code {
userErrors := map[string]codes.Code{
"RESOURCE_NOT_FOUND": codes.NotFound,
"RESOURCE_ALREADY_EXISTS": codes.AlreadyExists,
"RESOURCE_IN_USE_BY_ANOTHER_RESOURCE": codes.InvalidArgument,
"OPERATION_CANCELED_BY_USER": codes.Aborted,
"QUOTA_EXCEEDED": codes.ResourceExhausted,
"ZONE_RESOURCE_POOL_EXHAUSTED": codes.Unavailable,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use codes.ResourceExhausted for ZONE_RESOURCE_POOL_EXHAUSTED, ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these codes, it looks like GCE maps it to the Unavailable grpc error code, so that is why I chose Unavailable. That being said, we don't filter out Unavailable fromm our SLO, so if we don't want these, then I should use resource exhausted. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We looked at this for the CreateVolume SLO as well and ultimately decided we would want to get notified if users were hitting an irregular amount of ZONE_RESOURCE_POOL_EXHAUSTED errors since they're not under their control. So +1 to Unavailable since it allows us to track it (or not) separately from other user-caused ResourceExhausted errors.

Also it looks like this PR accomplishes what I was doing in #1263 (classifying QUOTA_EXCEEDED as ResourceExhausted) so I'll close that - thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Please capture the reasoning in a comment in the code

"ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS": codes.Unavailable,
"REGION_QUOTA_EXCEEDED": codes.ResourceExhausted,
"RATE_LIMIT_EXCEEDED": codes.ResourceExhausted,
"INVALID_USAGE": codes.InvalidArgument,
}
if code, ok := userErrors[err.Code]; ok {
return code
}
return codes.Internal
}

func opIsDone(op *computev1.Operation) (bool, error) {
Expand Down
73 changes: 73 additions & 0 deletions pkg/gce-cloud-provider/compute/gce-compute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"testing"

computev1 "google.golang.org/api/compute/v1"
"google.golang.org/grpc/codes"
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
)

Expand Down Expand Up @@ -101,3 +102,75 @@ func TestValidateDiskParameters(t *testing.T) {
}
}
}

func TestCodeForGCEOpError(t *testing.T) {
testCases := []struct {
name string
inputErr computev1.OperationErrorErrors
expCode codes.Code
}{
{
name: "RESOURCE_NOT_FOUND error",
inputErr: computev1.OperationErrorErrors{Code: "RESOURCE_NOT_FOUND"},
expCode: codes.NotFound,
},
{
name: "RESOURCE_ALREADY_EXISTS error",
inputErr: computev1.OperationErrorErrors{Code: "RESOURCE_ALREADY_EXISTS"},
expCode: codes.AlreadyExists,
},
{
name: "OPERATION_CANCELED_BY_USER error",
inputErr: computev1.OperationErrorErrors{Code: "OPERATION_CANCELED_BY_USER"},
expCode: codes.Aborted,
},
{
name: "QUOTA_EXCEEDED error",
inputErr: computev1.OperationErrorErrors{Code: "QUOTA_EXCEEDED"},
expCode: codes.ResourceExhausted,
},
{
name: "ZONE_RESOURCE_POOL_EXHAUSTED error",
inputErr: computev1.OperationErrorErrors{Code: "ZONE_RESOURCE_POOL_EXHAUSTED"},
expCode: codes.Unavailable,
},
{
name: "ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS error",
inputErr: computev1.OperationErrorErrors{Code: "ZONE_RESOURCE_POOL_EXHAUSTED_WITH_DETAILS"},
expCode: codes.Unavailable,
},
{
name: "REGION_QUOTA_EXCEEDED error",
inputErr: computev1.OperationErrorErrors{Code: "REGION_QUOTA_EXCEEDED"},
expCode: codes.ResourceExhausted,
},
{
name: "RATE_LIMIT_EXCEEDED error",
inputErr: computev1.OperationErrorErrors{Code: "RATE_LIMIT_EXCEEDED"},
expCode: codes.ResourceExhausted,
},
{
name: "INVALID_USAGE error",
inputErr: computev1.OperationErrorErrors{Code: "INVALID_USAGE"},
expCode: codes.InvalidArgument,
},
{
name: "RESOURCE_IN_USE_BY_ANOTHER_RESOURCE error",
inputErr: computev1.OperationErrorErrors{Code: "RESOURCE_IN_USE_BY_ANOTHER_RESOURCE"},
expCode: codes.InvalidArgument,
},
{
name: "UNSUPPORTED_OPERATION error",
inputErr: computev1.OperationErrorErrors{Code: "UNSUPPORTED_OPERATION"},
expCode: codes.Internal,
},
}

for _, tc := range testCases {
t.Logf("Running test: %v", tc.name)
errCode := codeForGCEOpError(tc.inputErr)
if errCode != tc.expCode {
t.Errorf("test %v failed: got %v, expected %v", tc.name, errCode, tc.expCode)
}
}
}