From 5de39f2f9dbda8380d1a00922002b9b7d5495eb3 Mon Sep 17 00:00:00 2001 From: The Magician Date: Thu, 14 Apr 2022 12:07:19 -0500 Subject: [PATCH] Retry all GCE's 403 per-minute quota exceeded errors (#5913) (#4223) Co-authored-by: Radoslaw Majkut Signed-off-by: Modular Magician Co-authored-by: Radoslaw Majkut --- .changelog/5913.txt | 3 +++ google-beta/common_operation.go | 2 +- google-beta/error_retry_predicates.go | 27 ++++++++-------------- google-beta/error_retry_predicates_test.go | 26 +++++++++++++++++++-- 4 files changed, 38 insertions(+), 20 deletions(-) create mode 100644 .changelog/5913.txt diff --git a/.changelog/5913.txt b/.changelog/5913.txt new file mode 100644 index 0000000000..a80f315a77 --- /dev/null +++ b/.changelog/5913.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +provider: modified request retry logic to retry all per-minute quota limits returned with a 403 error code. Previously, only read requests were retried. This will generally affect Google Compute Engine resources. +``` diff --git a/google-beta/common_operation.go b/google-beta/common_operation.go index 624c37e83f..bbd724b8c2 100644 --- a/google-beta/common_operation.go +++ b/google-beta/common_operation.go @@ -111,7 +111,7 @@ func CommonRefreshFunc(w Waiter) resource.StateRefreshFunc { op, err := w.QueryOp() if err != nil { // Retry 404 when getting operation (not resource state) - if isRetryableError(err, isNotFoundRetryableError("GET operation"), isOperationReadQuotaError) { + if isRetryableError(err, isNotFoundRetryableError("GET operation")) { log.Printf("[DEBUG] Dismissed retryable error on GET operation %q: %s", w.OpName(), err) return nil, "done: false", nil } diff --git a/google-beta/error_retry_predicates.go b/google-beta/error_retry_predicates.go index 01b473dad8..6b3983e171 100644 --- a/google-beta/error_retry_predicates.go +++ b/google-beta/error_retry_predicates.go @@ -6,6 +6,7 @@ import ( "log" "net" "net/url" + "regexp" "strings" "google.golang.org/api/googleapi" @@ -45,7 +46,7 @@ var defaultErrorRetryPredicates = []RetryErrorPredicateFunc{ // reads, causing significant failure for our CI and for large customers. // GCE returns the wrong error code, as this should be a 429, which we retry // already. - is403ReadRequestsForMinuteError, + is403QuotaExceededPerMinuteError, } /** END GLOBAL ERROR RETRY PREDICATES HERE **/ @@ -127,15 +128,18 @@ func isSubnetworkUnreadyError(err error) (bool, string) { // GCE (and possibly other APIs) incorrectly return a 403 rather than a 429 on // rate limits. -func is403ReadRequestsForMinuteError(err error) (bool, string) { +func is403QuotaExceededPerMinuteError(err error) (bool, string) { gerr, ok := err.(*googleapi.Error) if !ok { return false, "" } - - if gerr.Code == 403 && strings.Contains(gerr.Body, "Quota exceeded for quota metric") && strings.Contains(gerr.Body, "Read requests per minute") { - log.Printf("[DEBUG] Dismissed an error as retryable based on error code 403 and error message 'Quota exceeded for quota metric' on metric `Read requests per minute`: %s", err) - return true, "Read requests per minute" + var QuotaRegex = regexp.MustCompile(`Quota exceeded for quota metric '(?P.*)' and limit '(?P.* per minute)' of service`) + if gerr.Code == 403 && QuotaRegex.MatchString(gerr.Body) { + matches := QuotaRegex.FindStringSubmatch(gerr.Body) + metric := matches[QuotaRegex.SubexpIndex("Metric")] + limit := matches[QuotaRegex.SubexpIndex("Limit")] + log.Printf("[DEBUG] Dismissed an error as retryable based on error code 403 and error message 'Quota exceeded for quota metric `%s`: %s", metric, err) + return true, fmt.Sprintf("Waiting for quota limit %s to refresh", limit) } return false, "" } @@ -260,17 +264,6 @@ func isBigqueryIAMQuotaError(err error) (bool, string) { return false, "" } -// Retry if operation returns a 403 with the message for -// exceeding the quota limit for 'OperationReadGroup' -func isOperationReadQuotaError(err error) (bool, string) { - if gerr, ok := err.(*googleapi.Error); ok { - if gerr.Code == 403 && strings.Contains(gerr.Body, "Quota exceeded for quota group") { - return true, "Waiting for quota to refresh" - } - } - return false, "" -} - // Retry if Monitoring operation returns a 409 with a specific message for // concurrent operations. func isMonitoringConcurrentEditError(err error) (bool, string) { diff --git a/google-beta/error_retry_predicates_test.go b/google-beta/error_retry_predicates_test.go index 0c1c8e906f..f34a044565 100644 --- a/google-beta/error_retry_predicates_test.go +++ b/google-beta/error_retry_predicates_test.go @@ -84,14 +84,36 @@ func TestIsCommonRetryableErrorCode_otherError(t *testing.T) { func TestIsOperationReadQuotaError_quotaExceeded(t *testing.T) { err := googleapi.Error{ Code: 403, - Body: "Quota exceeded for quota group 'OperationReadGroup' and limit 'Operation read requests per 100 seconds' of service 'compute.googleapis.com' for consumer 'project_number:11111111'.", + Body: "Quota exceeded for quota metric 'OperationReadGroup' and limit 'Operation read requests per minute' of service 'compute.googleapis.com' for consumer 'project_number:11111111'.", } - isRetryable, _ := isOperationReadQuotaError(&err) + isRetryable, _ := is403QuotaExceededPerMinuteError(&err) if !isRetryable { t.Errorf("Error not detected as retryable") } } +func TestIs403QuotaExceededPerMinuteError_perMinuteQuotaExceeded(t *testing.T) { + err := googleapi.Error{ + Code: 403, + Body: "Quota exceeded for quota metric 'Queries' and limit 'Queries per minute' of service 'compute.googleapis.com' for consumer 'project_number:11111111'.", + } + isRetryable, _ := is403QuotaExceededPerMinuteError(&err) + if !isRetryable { + t.Errorf("Error not detected as retryable") + } +} + +func TestIs403QuotaExceededPerMinuteError_perDayQuotaExceededNotRetryable(t *testing.T) { + err := googleapi.Error{ + Code: 403, + Body: "Quota exceeded for quota metric 'Queries' and limit 'Queries per day' of service 'compute.googleapis.com' for consumer 'project_number:11111111'.", + } + isRetryable, _ := is403QuotaExceededPerMinuteError(&err) + if isRetryable { + t.Errorf("Error incorrectly detected as retryable") + } +} + func TestGRPCRetryable(t *testing.T) { code := codes.FailedPrecondition err := status.Error(code, "is retryable")