Add working retries for SQL operations where the operation has the error (#3298)

<!-- This change is generated by MagicModules. -->
/cc @rileykarson

google/common_operation.go

@@ -22,7 +22,9 @@ type Waiter interface {
 	SetOp(interface{}) error
 
 	// QueryOp sends a request to the server to get the current status of the
-	// operation.
+	// operation. It's expected that QueryOp will return exactly one of an
+	// operation or an error as non-nil, and that requests will be retried by
+	// specific implementations of the method.
 	QueryOp() (interface{}, error)
 
 	// OpName is the name of the operation and is used to log its status.
@@ -90,35 +92,22 @@ func OperationDone(w Waiter) bool {
 
 func CommonRefreshFunc(w Waiter) resource.StateRefreshFunc {
 	return func() (interface{}, string, error) {
-		// First, read the operation from the server.
 		op, err := w.QueryOp()
-
-		// If we got a non-retryable error, return it.
 		if err != nil {
-			if !isRetryableError(err) {
-				return nil, "", fmt.Errorf("Not retriable error: %s", err)
-			}
-
-			log.Printf("[DEBUG] Saw error polling for op, but dismissed as retriable: %s", err)
-			if op == nil {
-				return nil, "", fmt.Errorf("Cannot continue, Operation is nil. %s", err)
-			}
+			// Importantly, this error is in the GET to the operation, and isn't an error
+			// with the resource CRUD request itself.
+			return nil, "", fmt.Errorf("error while retrieving operation: %s", err)
 		}
 
-		log.Printf("[DEBUG] working with op %#v", op)
-
-		// Try to set the operation (so we can check it's Error/State),
-		// and fail if we can't.
 		if err = w.SetOp(op); err != nil {
-			return nil, "", fmt.Errorf("Cannot continue %s", err)
+			return nil, "", fmt.Errorf("Cannot continue, unable to use operation: %s", err)
 		}
 
-		// Fail if the operation object contains an error.
 		if err = w.Error(); err != nil {
 			return nil, "", err
 		}
-		log.Printf("[DEBUG] Got %v while polling for operation %s's status", w.State(), w.OpName())
 
+		log.Printf("[DEBUG] Got %v while polling for operation %s's status", w.State(), w.OpName())
 		return op, w.State(), nil
 	}
 }

google/sqladmin_operation.go

@@ -61,7 +61,18 @@ func (w *SqlAdminOperationWaiter) QueryOp() (interface{}, error) {
 		return nil, fmt.Errorf("Cannot query operation, service is nil.")
 	}
 
-	return w.Service.Operations.Get(w.Project, w.Op.Name).Do()
+	var op interface{}
+	var err error
+	err = retryTimeDuration(
+		func() error {
+			op, err = w.Service.Operations.Get(w.Project, w.Op.Name).Do()
+			return err
+		},
+
+		DefaultRequestTimeout,
+	)
+
+	return op, err
 }
 
 func (w *SqlAdminOperationWaiter) OpName() string {

google/transport.go

@@ -14,6 +14,8 @@ import (
 	"google.golang.org/api/googleapi"
 )
 
+var DefaultRequestTimeout = 5 * time.Minute
+
 func isEmptyValue(v reflect.Value) bool {
 	switch v.Kind() {
 	case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
@@ -33,7 +35,7 @@ func isEmptyValue(v reflect.Value) bool {
 }
 
 func sendRequest(config *Config, method, rawurl string, body map[string]interface{}) (map[string]interface{}, error) {
-	return sendRequestWithTimeout(config, method, rawurl, body, 5*time.Minute)
+	return sendRequestWithTimeout(config, method, rawurl, body, DefaultRequestTimeout)
 }
 
 func sendRequestWithTimeout(config *Config, method, rawurl string, body map[string]interface{}, timeout time.Duration) (map[string]interface{}, error) {

google/utils.go

@@ -351,11 +351,13 @@ func retryTimeDuration(retryFunc func() error, duration time.Duration) error {
 
 func isRetryableError(err error) bool {
 	if gerr, ok := err.(*googleapi.Error); ok && (gerr.Code == 429 || gerr.Code == 500 || gerr.Code == 502 || gerr.Code == 503) {
+		log.Printf("[DEBUG] Dismissed an error as retryable based on error code: %s", err)
 		return true
 	}
 	// These operations are always hitting googleapis.com - they should rarely
 	// time out, and if they do, that timeout is retryable.
 	if urlerr, ok := err.(*url.Error); ok && urlerr.Timeout() {
+		log.Printf("[DEBUG] Dismissed an error as retryable based on googleapis.com target: %s", err)
 		return true
 	}
 	if gerr, ok := err.(*googleapi.Error); ok && gerr.Code == 409 && strings.Contains(gerr.Body, "operationInProgress") {
@@ -363,6 +365,7 @@ func isRetryableError(err error) bool {
 		// The only way right now to determine it is a SQL 409 due to concurrent calls is to
 		// look at the contents of the error message.
 		// See https://github.com/terraform-providers/terraform-provider-google/issues/3279
+		log.Printf("[DEBUG] Dismissed an error as retryable based on error code 409 and error reason 'operationInProgress': %s", err)
 		return true
 	}