provider/google: use a mutex to prevent concurrent sql instance operations #14424

danawillow · 2017-05-12T02:46:13Z

A few implementation questions:

Is resource_sql_database_instance_test the best test file for these changes? It's not about instance creation, it's about other stuff creation, but it's also not user or database specific so instance felt more general.
Is locking on the instance name sufficient or should we have some sort of prefix to make sure we don't accidentally conflict with other resources? I would do the instance URL but we don't have it at that point and it seems silly to do a read just to get the URL so we can lock on it (and I also don't want to reconstruct the URL). We can get the instance URL from the operation, but at that point it's too late, I think.

TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccGoogleSqlDatabaseInstance_multipleOperations -timeout 120m
=== RUN   TestAccGoogleSqlDatabaseInstance_multipleOperations
--- PASS: TestAccGoogleSqlDatabaseInstance_multipleOperations (51.86s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/google	51.985s

…tions

ctavan · 2017-05-15T14:10:10Z

Thanks a lot for attacking this issue!

While I don't feel 100% competent to comment on this I believe that in my case a mutex on the instance name would not be the ideal solution. While the default for google_sql_database_instance is to auto-generate a random instance name where using the instance name for the mutex should be fine, I have chosen instance names like main/default/master in practice and I have used the same name for many other resources as well since many of my google resources are grouped within terraform modules where I don't see a need for more specific naming (not sure if other google resources use the plain name for mutexes though).

I think prefixing would already improve the situation a lot even though a slightly uneasy feeling remains for me since there is still the hypothetical case where names could collide… I admit that it's probably not a problem in practice though.

danawillow · 2017-05-16T21:56:01Z

That makes sense! I added a prefix, and included the project name in the key as well just in case the same database name is being used in multiple projects with the same tf config.

ctavan · 2017-05-17T05:54:13Z

Great, that should be reasonably unique! Now another resource in the same project would have to choose that exact prefix to produce a clash. I still wonder if we should leave a small comment on the instanceMutexKey that we are aware that the way we determine the mutex key is not 100% safe but that we reason that in practice it should be very close to 100% safe… Should someone ever run into the issue that might help. But maybe it's not worth the effort.

Anyways, my concrete issue should be fixed by your patch, so thanks a lot for this!

paddycarver

This looks great. Mostly just looking for a comment update, and had a couple suggestions about the mutex strategy. If I'm misunderstanding, please feel free to correct me!

paddycarver · 2017-05-18T05:57:31Z

builtin/providers/google/provider.go

@@ -266,3 +267,6 @@ func handleNotFoundError(err error, d *schema.ResourceData, resource string) err

 	return fmt.Errorf("Error reading %s: %s", resource, err)
 }
+
+// This is a global MutexKV for use within this plugin.


Would be cool to explain what it's locking on, and what limitation makes the lock necessary.

Also, rather than having this be plugin-wide, what about making it just for this particular concept--e.g., locking insertion/removal of SQL instances? Then we don't need to worry about the prefix, we can just use something that uniquely identifies an SQL instance amongst all SQL instances.

The AWS plugin offers a similar Provider wide mutex, so the approach here seems consistent

Fair, and good to know!

Fair enough, and good to know!

paddycarver · 2017-05-18T05:58:48Z

builtin/providers/google/resource_sql_database.go

@@ -67,6 +68,7 @@ func resourceSqlDatabaseCreate(d *schema.ResourceData, meta interface{}) error {
 	}

 	err = sqladminOperationWait(config, op, "Insert Database")
+	googleMutexKV.Unlock(instanceMutexKey(project, instance_name))


I'd rather see this after line 60 in a defer, to be honest, just to cover our bases.

Does defer here actually result in the Unlock not being called until resourceSqlDatabaseRead is called? If so, then I think the unlock here is probably correct, assuming the resourceSqlDatabaseRead is idempotent

Indeed, defer here will result in the lock persisting until resourceSqlDatabaseRead completes, so probably not what we want to do.

https://play.golang.org/p/61yvP1eLbs

That said, we probably do want to add an additional unlock in the error check block, yeah?

Yeah, I knew defer would block until resourceSqlDatabaseRead was finished; I checked to make sure a lock wasn't needed inside it. IMO, having the lock stuff work without the danger of missing a lock (like in the error check block that you noticed) outweighs the expanded lock time, but I'm willing to be outvoted on that by you and @danawillow.

Regardless, I think we do want to make sure the lock is released after that error check block, even though (IIUC) Terraform will exit anyways, meaning the lock would never be used again regardless.

(I stand corrected on the unlock in the error check block--this Unlock will fire before the error checking, so another Unlock in the error check block below is unnecessary.)

(EDIT: Ooops, we mean the error check block above. I stand incorrected.)

paddycarver · 2017-05-18T05:59:12Z

builtin/providers/google/resource_sql_database.go

@@ -121,6 +124,7 @@ func resourceSqlDatabaseDelete(d *schema.ResourceData, meta interface{}) error {
 	}

 	err = sqladminOperationWait(config, op, "Delete Database")
+	googleMutexKV.Unlock(instanceMutexKey(project, instance_name))


Same, would really like to see the defer approach used here, for completeness.

paddycarver · 2017-05-18T06:02:09Z

builtin/providers/google/resource_sql_user.go

@@ -78,6 +79,7 @@ func resourceSqlUserCreate(d *schema.ResourceData, meta interface{}) error {
 	}

 	err = sqladminOperationWait(config, op, "Insert User")
+	googleMutexKV.Unlock(instanceMutexKey(project, instance))


defer this?

paddycarver · 2017-05-18T06:02:21Z

builtin/providers/google/resource_sql_user.go

@@ -154,6 +157,7 @@ func resourceSqlUserUpdate(d *schema.ResourceData, meta interface{}) error {
 		}

 		err = sqladminOperationWait(config, op, "Insert User")
+		googleMutexKV.Lock(instanceMutexKey(project, instance))


defer this?

paddycarver · 2017-05-18T06:02:29Z

builtin/providers/google/resource_sql_user.go

@@ -187,6 +192,7 @@ func resourceSqlUserDelete(d *schema.ResourceData, meta interface{}) error {
 	}

 	err = sqladminOperationWait(config, op, "Delete User")
+	googleMutexKV.Unlock(instanceMutexKey(project, instance))


defer this?

catsby

👍 from me, recommend waiting for @paddycarver to review my comment on defer

catsby · 2017-05-18T21:01:57Z

builtin/providers/google/provider.go

@@ -266,3 +267,6 @@ func handleNotFoundError(err error, d *schema.ResourceData, resource string) err

 	return fmt.Errorf("Error reading %s: %s", resource, err)
 }
+
+// This is a global MutexKV for use within this plugin.


The AWS plugin offers a similar Provider wide mutex, so the approach here seems consistent

paddycarver · 2017-05-24T19:04:42Z

I'm fine with it, but if @danawillow is willing to update to release the lock in the error block, that'd be even better. :) Leaving to @danawillow.

danawillow · 2017-05-25T00:24:07Z

I'm fine with the defer- releasing the lock after the read is done seems reasonable. We can always change it later. Updated to add it.

paddycarver

Last thing--is it worth using the mutexKV we already define in provider.go instead of adding googleMutexKV?

danawillow · 2017-05-25T20:35:14Z

Yes of course, just needed to merge with master. Done.

ghost · 2020-04-11T02:36:31Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

provider/google: use a mutex to prevent concurrent sql instance opera…

3674eb0

…tions

grubernaut added bug provider/google-cloud labels May 12, 2017

grubernaut requested a review from paddycarver May 15, 2017 15:39

use a prefix for locking

a8e0392

paddycarver suggested changes May 18, 2017

View reviewed changes

catsby reviewed May 18, 2017

View reviewed changes

paddycarver approved these changes May 24, 2017

View reviewed changes

use defers

3ed897c

paddycarver reviewed May 25, 2017

View reviewed changes

danawillow added 2 commits May 25, 2017 13:29

Merge branch 'master' into is-9018

bdc22f7

s/googleMutexKV/mutexKV/

0ab0b8a

danawillow merged commit 0d1718f into hashicorp:master May 31, 2017

danawillow deleted the is-9018 branch May 31, 2017 19:20

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/google: use a mutex to prevent concurrent sql instance operations #14424

provider/google: use a mutex to prevent concurrent sql instance operations #14424

danawillow commented May 12, 2017

ctavan commented May 15, 2017

danawillow commented May 16, 2017

ctavan commented May 17, 2017

paddycarver left a comment

paddycarver May 18, 2017

paddycarver May 18, 2017

catsby May 18, 2017

paddycarver May 24, 2017

paddycarver May 24, 2017

paddycarver May 18, 2017

catsby May 18, 2017

catsby May 18, 2017

paddycarver May 24, 2017

paddycarver May 24, 2017 •

edited

Loading

paddycarver May 18, 2017

paddycarver May 18, 2017

paddycarver May 18, 2017

paddycarver May 18, 2017

catsby left a comment

catsby May 18, 2017

paddycarver commented May 24, 2017

danawillow commented May 25, 2017

paddycarver left a comment

danawillow commented May 25, 2017

ghost commented Apr 11, 2020

provider/google: use a mutex to prevent concurrent sql instance operations #14424

provider/google: use a mutex to prevent concurrent sql instance operations #14424

Conversation

danawillow commented May 12, 2017

ctavan commented May 15, 2017

danawillow commented May 16, 2017

ctavan commented May 17, 2017

paddycarver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver commented May 24, 2017

danawillow commented May 25, 2017

paddycarver left a comment

Choose a reason for hiding this comment

danawillow commented May 25, 2017

ghost commented Apr 11, 2020

paddycarver May 24, 2017 •

edited

Loading