From 20b79f1bd16035284388c902a7fd591f3db27db7 Mon Sep 17 00:00:00 2001 From: Modular Magician Date: Wed, 23 Dec 2020 20:41:00 +0000 Subject: [PATCH] Eliminate retries in the inner loop for errors that will not succeed. (#4334) Signed-off-by: Modular Magician --- .changelog/4334.txt | 6 ++++++ google/error_retry_predicates.go | 18 +++++++++++------- google/filestore_operation.go | 2 +- google/iam_storage_bucket.go | 4 ++-- google/resource_filestore_instance.go | 8 ++++---- ...source_filestore_instance_generated_test.go | 2 +- 6 files changed, 25 insertions(+), 15 deletions(-) create mode 100644 .changelog/4334.txt diff --git a/.changelog/4334.txt b/.changelog/4334.txt new file mode 100644 index 00000000000..46f57ed021c --- /dev/null +++ b/.changelog/4334.txt @@ -0,0 +1,6 @@ +```release-note:bug +filestore: fail fast on quota error which cannot succeed on retry. +``` +```release-note:bug +storage: refresh etag sooner on an IAM conflict error, which will make applications of multiple IAM resources much faster. +``` diff --git a/google/error_retry_predicates.go b/google/error_retry_predicates.go index b2dbf22a7e9..f7f1fe9c529 100644 --- a/google/error_retry_predicates.go +++ b/google/error_retry_predicates.go @@ -228,6 +228,17 @@ func isMonitoringConcurrentEditError(err error) (bool, string) { return false, "" } +// Retry if filestore operation returns a 429 with a specific message for +// concurrent operations. +func isNotFilestoreQuotaError(err error) (bool, string) { + if gerr, ok := err.(*googleapi.Error); ok { + if gerr.Code == 429 { + return false, "" + } + } + return isCommonRetryableErrorCode(err) +} + // Retry if App Engine operation returns a 409 with a specific message for // concurrent operations. func isAppEngineRetryableError(err error) (bool, string) { @@ -260,13 +271,6 @@ func isNotFoundRetryableError(opType string) RetryErrorPredicateFunc { } } -func isStoragePreconditionError(err error) (bool, string) { - if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 412 { - return true, fmt.Sprintf("Retry on storage precondition not met") - } - return false, "" -} - func isDataflowJobUpdateRetryableError(err error) (bool, string) { if gerr, ok := err.(*googleapi.Error); ok { if gerr.Code == 404 && strings.Contains(gerr.Body, "in RUNNING OR DRAINING state") { diff --git a/google/filestore_operation.go b/google/filestore_operation.go index 7e6c4be5e41..895c0cc65b8 100644 --- a/google/filestore_operation.go +++ b/google/filestore_operation.go @@ -33,7 +33,7 @@ func (w *FilestoreOperationWaiter) QueryOp() (interface{}, error) { // Returns the proper get. url := fmt.Sprintf("https://file.googleapis.com/v1/%s", w.CommonOperationWaiter.Op.Name) - return sendRequest(w.Config, "GET", w.Project, url, w.UserAgent, nil) + return sendRequest(w.Config, "GET", w.Project, url, w.UserAgent, nil, isNotFilestoreQuotaError) } func createFilestoreWaiter(config *Config, op map[string]interface{}, project, activity, userAgent string) (*FilestoreOperationWaiter, error) { diff --git a/google/iam_storage_bucket.go b/google/iam_storage_bucket.go index 2a38e226154..6904dc7c51f 100644 --- a/google/iam_storage_bucket.go +++ b/google/iam_storage_bucket.go @@ -111,7 +111,7 @@ func (u *StorageBucketIamUpdater) GetResourceIamPolicy() (*cloudresourcemanager. return nil, err } - policy, err := sendRequest(u.Config, "GET", "", url, userAgent, obj, isStoragePreconditionError) + policy, err := sendRequest(u.Config, "GET", "", url, userAgent, obj) if err != nil { return nil, errwrap.Wrapf(fmt.Sprintf("Error retrieving IAM policy for %s: {{err}}", u.DescribeResource()), err) } @@ -143,7 +143,7 @@ func (u *StorageBucketIamUpdater) SetResourceIamPolicy(policy *cloudresourcemana return err } - _, err = sendRequestWithTimeout(u.Config, "PUT", "", url, userAgent, obj, u.d.Timeout(schema.TimeoutCreate), isStoragePreconditionError) + _, err = sendRequestWithTimeout(u.Config, "PUT", "", url, userAgent, obj, u.d.Timeout(schema.TimeoutCreate)) if err != nil { return errwrap.Wrapf(fmt.Sprintf("Error setting IAM policy for %s: {{err}}", u.DescribeResource()), err) } diff --git a/google/resource_filestore_instance.go b/google/resource_filestore_instance.go index e9c403de4d5..ae1217574fc 100644 --- a/google/resource_filestore_instance.go +++ b/google/resource_filestore_instance.go @@ -220,7 +220,7 @@ func resourceFilestoreInstanceCreate(d *schema.ResourceData, meta interface{}) e billingProject = bp } - res, err := sendRequestWithTimeout(config, "POST", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutCreate)) + res, err := sendRequestWithTimeout(config, "POST", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutCreate), isNotFilestoreQuotaError) if err != nil { return fmt.Errorf("Error creating Instance: %s", err) } @@ -281,7 +281,7 @@ func resourceFilestoreInstanceRead(d *schema.ResourceData, meta interface{}) err billingProject = bp } - res, err := sendRequest(config, "GET", billingProject, url, userAgent, nil) + res, err := sendRequest(config, "GET", billingProject, url, userAgent, nil, isNotFilestoreQuotaError) if err != nil { return handleNotFoundError(err, d, fmt.Sprintf("FilestoreInstance %q", d.Id())) } @@ -381,7 +381,7 @@ func resourceFilestoreInstanceUpdate(d *schema.ResourceData, meta interface{}) e billingProject = bp } - res, err := sendRequestWithTimeout(config, "PATCH", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutUpdate)) + res, err := sendRequestWithTimeout(config, "PATCH", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutUpdate), isNotFilestoreQuotaError) if err != nil { return fmt.Errorf("Error updating Instance %q: %s", d.Id(), err) @@ -428,7 +428,7 @@ func resourceFilestoreInstanceDelete(d *schema.ResourceData, meta interface{}) e billingProject = bp } - res, err := sendRequestWithTimeout(config, "DELETE", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutDelete)) + res, err := sendRequestWithTimeout(config, "DELETE", billingProject, url, userAgent, obj, d.Timeout(schema.TimeoutDelete), isNotFilestoreQuotaError) if err != nil { return handleNotFoundError(err, d, "Instance") } diff --git a/google/resource_filestore_instance_generated_test.go b/google/resource_filestore_instance_generated_test.go index 2944f167a5b..7b543078d2e 100644 --- a/google/resource_filestore_instance_generated_test.go +++ b/google/resource_filestore_instance_generated_test.go @@ -94,7 +94,7 @@ func testAccCheckFilestoreInstanceDestroyProducer(t *testing.T) func(s *terrafor billingProject = config.BillingProject } - _, err = sendRequest(config, "GET", billingProject, url, config.userAgent, nil) + _, err = sendRequest(config, "GET", billingProject, url, config.userAgent, nil, isNotFilestoreQuotaError) if err == nil { return fmt.Errorf("FilestoreInstance still exists at %s", url) }