Skip to content

Commit 6c36bcf

Browse files
authored
feat(native-retries): add custom should-retry logic for mount initialization (#4805)
* feat(native-retries): add custom should-retry logic for mount initialization ### Description - Implemented `ShouldRetryOnMount`, `ShouldRetryOnMountWithRetryContext`, and `ShouldRetryOnMountWithMonitoringAndRetryContext` in `internal/storage/storageutil/custom_retry.go`. - Scoped mount initialization retries to standard transient errors (`ShouldRetryWithoutLogging`), missing buckets (`404` / `NotFound` with "bucket does not exist", case-insensitive), and `403` / `PermissionDenied`. - Configured retry logging in `ShouldRetryOnMountWithRetryContext` to use `logger.Errorf` to ensure visibility across standard error logs and GKE error files. ### Link to the issue in case of a bug fix. NA ### Testing details 1. Manual - NA 2. Unit tests - Added 5 table-driven test suites (19 cases) in `custom_retry_test.go` verifying nil guards, standard transient errors, thread-safe `logBuffer` concurrency, ERROR-level log output, case-insensitive missing bucket matching, and error categorization (`STALLED_READ_REQUEST` vs `OTHER_ERRORS`). Ran full package tests (`go test ./internal/storage/storageutil/...`). 3. Integration tests - NA ### Any backward incompatible change? If so, please explain. No * test(storageutil): add unit tests for wrapped gRPC and permanent errors on mount * refactor(native-retries): centralize mount retry error classification in determineRetryAction
1 parent 1ab1f07 commit 6c36bcf

2 files changed

Lines changed: 167 additions & 14 deletions

File tree

internal/storage/storageutil/custom_retry.go

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package storageutil
1717
import (
1818
"context"
1919
"errors"
20+
"strings"
2021

2122
"cloud.google.com/go/storage"
2223
"github.com/googlecloudplatform/gcsfuse/v3/internal/logger"
@@ -38,41 +39,69 @@ const (
3839
retry401
3940
// retryUnauthenticated indicates a gRPC Unauthenticated error which requires a retry due to credentials refresh.
4041
retryUnauthenticated
42+
// retry404BucketDoesNotExist indicates an HTTP 404 error where the bucket was not found during mount.
43+
retry404BucketDoesNotExist
44+
// retryNotFoundBucketDoesNotExist indicates a gRPC NotFound error where the bucket was not found during mount.
45+
retryNotFoundBucketDoesNotExist
46+
// retry403 indicates an HTTP 403 Permission Denied error during mount.
47+
retry403
48+
// retryPermissionDenied indicates a gRPC PermissionDenied error during mount.
49+
retryPermissionDenied
4150
)
4251

52+
const errStrBucketNotExist = "bucket does not exist"
53+
4354
func determineRetryAction(err error) retryAction {
4455
if storage.ShouldRetry(err) {
4556
return retryTransient
4657
}
4758

48-
// HTTP 401 errors - Invalid Credentials
49-
// This is a work-around to fix the corner case where GCSFuse checks the token
50-
// as valid but GCS says invalid. This might be due to client-server timer
51-
// issues. Actual fix will be refresh the token earlier than 1 hr.
52-
// Changes will be done post resolution of the below issue:
53-
// https://github.com/golang/oauth2/issues/623
54-
// TODO(b/518674297): Please incorporate the correct fix post resolution of the above issue.
55-
if typed, ok := err.(*googleapi.Error); ok {
56-
if typed.Code == 401 {
59+
var apiErr *googleapi.Error
60+
if errors.As(err, &apiErr) {
61+
// HTTP 401 errors - Invalid Credentials
62+
// This is a work-around to fix the corner case where GCSFuse checks the token
63+
// as valid but GCS says invalid. This might be due to client-server timer
64+
// issues. Actual fix will be refresh the token earlier than 1 hr.
65+
// Changes will be done post resolution of the below issue:
66+
// https://github.com/golang/oauth2/issues/623
67+
// TODO(b/518674297): Please incorporate the correct fix post resolution of the above issue.
68+
if apiErr.Code == 401 {
5769
return retry401
5870
}
71+
if apiErr.Code == 403 {
72+
return retry403
73+
}
74+
if apiErr.Code == 404 && strings.Contains(strings.ToLower(apiErr.Message), errStrBucketNotExist) {
75+
return retry404BucketDoesNotExist
76+
}
5977
}
6078

61-
// This is the same case as above, but for gRPC UNAUTHENTICATED errors. See
62-
// https://github.com/golang/oauth2/issues/623
63-
// TODO(b/518674297): Please incorporate the correct fix post resolution of the above issue.
6479
if status, ok := status.FromError(err); ok {
80+
// This is the same case as above, but for gRPC UNAUTHENTICATED errors. See
81+
// https://github.com/golang/oauth2/issues/623
82+
// TODO(b/518674297): Please incorporate the correct fix post resolution of the above issue.
6583
if status.Code() == codes.Unauthenticated {
6684
return retryUnauthenticated
6785
}
86+
if status.Code() == codes.PermissionDenied {
87+
return retryPermissionDenied
88+
}
89+
if status.Code() == codes.NotFound && strings.Contains(strings.ToLower(status.Message()), errStrBucketNotExist) {
90+
return retryNotFoundBucketDoesNotExist
91+
}
6892
}
6993
return noRetry
7094
}
7195

7296
// ShouldRetryWithoutLogging checks if the error is transient and should be retried.
7397
// This method is same as ShouldRetry except it doesn't add warning logs.
7498
func ShouldRetryWithoutLogging(err error) bool {
75-
return determineRetryAction(err) != noRetry
99+
switch determineRetryAction(err) {
100+
case retryTransient, retry401, retryUnauthenticated:
101+
return true
102+
default:
103+
return false
104+
}
76105
}
77106

78107
// ShouldRetryWithRetryContext checks if the given error is transient and should be retried,
@@ -114,3 +143,9 @@ func ShouldRetryWithMonitoringAndRetryContext(
114143
metricHandle.GcsRetryCount(1, val)
115144
return retry
116145
}
146+
147+
// ShouldRetryOnMount checks if the error is retryable during mount initialization.
148+
// In addition to standard transient errors, it retries HTTP 403/404 and gRPC PermissionDenied/NotFound errors.
149+
func ShouldRetryOnMount(err error) bool {
150+
return determineRetryAction(err) != noRetry
151+
}

internal/storage/storageutil/custom_retry_test.go

Lines changed: 119 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"bytes"
1919
"context"
2020
"errors"
21+
"fmt"
2122
"io"
2223
"net"
2324
"net/url"
@@ -180,6 +181,21 @@ func TestShouldRetryWithoutLogging(t *testing.T) {
180181
},
181182
expectedResult: false,
182183
},
184+
{
185+
name: "403 error - non-retryable for regular ops",
186+
err: &googleapi.Error{
187+
Code: 403,
188+
},
189+
expectedResult: false,
190+
},
191+
{
192+
name: "404 bucket missing error - non-retryable for regular ops",
193+
err: &googleapi.Error{
194+
Code: 404,
195+
Message: "The specified bucket does not exist.",
196+
},
197+
expectedResult: false,
198+
},
183199
}
184200

185201
for _, tc := range testCases {
@@ -233,7 +249,22 @@ func TestDetermineRetryAction(t *testing.T) {
233249
{
234250
name: "PermissionDeniedGrpcError",
235251
err: status.Error(codes.PermissionDenied, "permission denied"),
236-
expected: noRetry,
252+
expected: retryPermissionDenied,
253+
},
254+
{
255+
name: "GoogleApiError403",
256+
err: &googleapi.Error{Code: 403},
257+
expected: retry403,
258+
},
259+
{
260+
name: "GoogleApiError404BucketNotExist",
261+
err: &googleapi.Error{Code: 404, Message: "The specified bucket does not exist."},
262+
expected: retry404BucketDoesNotExist,
263+
},
264+
{
265+
name: "GrpcNotFoundBucketNotExist",
266+
err: status.Error(codes.NotFound, "The specified bucket does not exist."),
267+
expected: retryNotFoundBucketDoesNotExist,
237268
},
238269
{
239270
name: "UnexpectedEOF",
@@ -423,3 +454,90 @@ func TestShouldRetryWithMonitoringForRetryableErrors(t *testing.T) {
423454
})
424455
}
425456
}
457+
458+
func TestShouldRetryOnMount(t *testing.T) {
459+
testCases := []struct {
460+
name string
461+
err error
462+
expected bool
463+
}{
464+
{
465+
name: "nil error",
466+
err: nil,
467+
expected: false,
468+
},
469+
{
470+
name: "standard transient error 502",
471+
err: &googleapi.Error{Code: 502},
472+
expected: true,
473+
},
474+
{
475+
name: "standard transient error 401",
476+
err: &googleapi.Error{Code: 401},
477+
expected: true,
478+
},
479+
{
480+
name: "HTTP 403 Forbidden",
481+
err: &googleapi.Error{Code: 403, Message: "Permission denied on resource"},
482+
expected: true,
483+
},
484+
{
485+
name: "HTTP 404 missing bucket",
486+
err: &googleapi.Error{Code: 404, Message: "The specified bucket does not exist."},
487+
expected: true,
488+
},
489+
{
490+
name: "HTTP 404 missing bucket mixed case",
491+
err: &googleapi.Error{Code: 404, Message: "The Specified Bucket Does Not Exist."},
492+
expected: true,
493+
},
494+
{
495+
name: "HTTP 404 missing object",
496+
err: &googleapi.Error{Code: 404, Message: "No such object: my-bucket/test-object"},
497+
expected: false,
498+
},
499+
{
500+
name: "gRPC PermissionDenied",
501+
err: status.Error(codes.PermissionDenied, "caller does not have required permission"),
502+
expected: true,
503+
},
504+
{
505+
name: "gRPC NotFound missing bucket",
506+
err: status.Error(codes.NotFound, "The specified bucket does not exist."),
507+
expected: true,
508+
},
509+
{
510+
name: "gRPC NotFound missing bucket mixed case",
511+
err: status.Error(codes.NotFound, "The Specified Bucket Does Not Exist."),
512+
expected: true,
513+
},
514+
{
515+
name: "gRPC NotFound missing object",
516+
err: status.Error(codes.NotFound, "No such object: my-bucket/test-object"),
517+
expected: false,
518+
},
519+
{
520+
name: "permanent error HTTP 400",
521+
err: &googleapi.Error{Code: 400, Message: "Bad Request"},
522+
expected: false,
523+
},
524+
{
525+
name: "permanent error gRPC InvalidArgument",
526+
err: status.Error(codes.InvalidArgument, "invalid bucket name"),
527+
expected: false,
528+
},
529+
{
530+
name: "wrapped gRPC PermissionDenied",
531+
err: fmt.Errorf("mount failed: %w", status.Error(codes.PermissionDenied, "caller does not have required permission")),
532+
expected: true,
533+
},
534+
}
535+
536+
for _, tc := range testCases {
537+
t.Run(tc.name, func(t *testing.T) {
538+
result := ShouldRetryOnMount(tc.err)
539+
540+
assert.Equal(t, tc.expected, result)
541+
})
542+
}
543+
}

0 commit comments

Comments
 (0)