Skip to content
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.

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

Investigate and fix flaky sql user test. #1212

Merged
merged 3 commits into from
Mar 23, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 2 additions & 2 deletions google/resource_sql_database_instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -685,10 +685,10 @@ func resourceSqlDatabaseInstanceCreate(d *schema.ResourceData, meta interface{})
// replica, then any users are inherited from the master instance and should be left alone.
if !sqlResourceIsReplica(d) {
var users *sqladmin.UsersListResponse
err = retry(func() error {
err = retryTime(func() error {
users, err = config.clientSqlAdmin.Users.List(project, instance.Name).Do()
return err
})
}, 3)
if err != nil {
return fmt.Errorf("Error, attempting to list users associated with instance %s: %s", instance.Name, err)
}
Expand Down
30 changes: 24 additions & 6 deletions google/resource_sql_user.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package google

import (
"errors"
"fmt"
"log"
"strings"
"time"

"github.com/hashicorp/terraform/helper/schema"
"google.golang.org/api/googleapi"
"google.golang.org/api/sqladmin/v1beta4"
)

Expand Down Expand Up @@ -87,7 +90,9 @@ func resourceSqlUserCreate(d *schema.ResourceData, meta interface{}) error {
"user %s into instance %s: %s", name, instance, err)
}

d.SetId(fmt.Sprintf("%s/%s", instance, name))
// This will include a double-slash (//) for 2nd generation instances,
// for which user.Host is an empty string. That's okay.
d.SetId(fmt.Sprintf("%s/%s/%s", user.Name, user.Host, user.Instance))

err = sqladminOperationWait(config, op, project, "Insert User")

Expand All @@ -111,14 +116,27 @@ func resourceSqlUserRead(d *schema.ResourceData, meta interface{}) error {
name := d.Get("name").(string)
host := d.Get("host").(string)

users, err := config.clientSqlAdmin.Users.List(project, instance).Do()

if err != nil {
return handleNotFoundError(err, d, fmt.Sprintf("SQL User %q in instance %q", name, instance))
var users *sqladmin.UsersListResponse
backoff := 1 * time.Second
err = nil
for users == nil || err != nil {
users, err = config.clientSqlAdmin.Users.List(project, instance).Do()

if e, ok := err.(*googleapi.Error); ok && (e.Code == 429 || e.Code == 503) {
backoff = backoff * 2
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be simplified with retry, right? That already retries (with backoff) on 429s and 503s. The only obstacle I'd see is the users == nil case- was that one that came up in testing or were you including it just-in-case?

err = retry(func() {
  users, err = config.clientSqlAdmin.Users.List(project, instance).Do()
  return err
})
if err != nil {
  return handleNotFoundError(...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using that as an entry condition. Yours is smarter, changing it.

if backoff > 30*time.Second {
return errors.New("Too many quota / service unavailable errors waiting for operation.")
}
time.Sleep(backoff)
} else if err != nil {
return handleNotFoundError(err, d, fmt.Sprintf("SQL User %q in instance %q", name, instance))
}
}

var user *sqladmin.User
for _, currentUser := range users.Items {
// The second part of this conditional is irrelevant for 2nd generation instances because
// host and currentUser.Host will always both be empty.
if currentUser.Name == name && currentUser.Host == host {
user = currentUser
break
Expand All @@ -136,6 +154,7 @@ func resourceSqlUserRead(d *schema.ResourceData, meta interface{}) error {
d.Set("instance", user.Instance)
d.Set("name", user.Name)
d.Set("project", project)
d.SetId(fmt.Sprintf("%s/%s/%s", user.Name, user.Host, user.Instance))
return nil
}

Expand Down Expand Up @@ -225,7 +244,6 @@ func resourceSqlUserImporter(d *schema.ResourceData, meta interface{}) ([]*schem
d.Set("instance", parts[0])
d.Set("host", parts[1])
d.Set("name", parts[2])
d.SetId(fmt.Sprintf("%s/%s", parts[0], parts[2]))
} else {
return nil, fmt.Errorf("Invalid specifier. Expecting {instance}/{name} for 2nd generation instance and {instance}/{host}/{name} for 1st generation instance")
}
Expand Down
1 change: 1 addition & 0 deletions google/resource_sql_user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ func TestAccSqlUser_secondGen(t *testing.T) {
},
resource.TestStep{
ResourceName: "google_sql_user.user",
ImportStateId: instance + "/admin",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"password"},
Expand Down
10 changes: 5 additions & 5 deletions google/sqladmin_operation.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"time"

"github.com/hashicorp/terraform/helper/resource"
"google.golang.org/api/googleapi"
"google.golang.org/api/sqladmin/v1beta4"
)

Expand All @@ -18,13 +19,12 @@ type SqlAdminOperationWaiter struct {

func (w *SqlAdminOperationWaiter) RefreshFunc() resource.StateRefreshFunc {
return func() (interface{}, string, error) {
var op *sqladmin.Operation
var err error

log.Printf("[DEBUG] self_link: %s", w.Op.SelfLink)
op, err = w.Service.Operations.Get(w.Project, w.Op.Name).Do()
op, err := w.Service.Operations.Get(w.Project, w.Op.Name).Do()

if err != nil {
if e, ok := err.(*googleapi.Error); ok && (e.Code == 429 || e.Code == 503) {
return w.Op, "PENDING", nil
} else if err != nil {
return nil, "", err
}

Expand Down