Add update support for bigtable clusters (such as resizing) #2350

stankiewicz · 2019-09-20T11:54:36Z

This PR is to address Issue #2521 for terraform-google-provider

What will be doable is scaling up and down number of clusters within instance without recreate.
If anything else is changed within cluster (change of disk, zone, name) it will be forced to recreate, so the behaviour mimics what is possible in UI.

fyi:

this is my first PR in this project
this is my first golang chunk of code
I'm new to TF

I've tested it with all kind of scenarios (resize, force recreate, both)

Any feedback highly appreciated so I can complete this PR.

modular-magician · 2019-09-20T11:55:57Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. They will authorize it to run through our CI pipeline, which will generate downstream PRs.

Thanks for your contribution! A human will be with you soon.

@rileykarson, please review this PR or find an appropriate assignee.

stankiewicz · 2019-09-20T12:05:27Z

Will add test for this case.

stankiewicz · 2019-09-20T13:27:12Z

One challenge I have,
Error: Provider produced inconsistent final plan

   " When expanding the plan for google_bigtable_instance.instance to include new
    values learned so far during apply, provider "google" produced an invalid new
    value for .cluster[1].zone: was cty.StringVal("us-central1-b"), but now
    cty.StringVal("us-central1-c")."

stankiewicz · 2019-09-20T13:53:45Z

third_party/terraform/resources/resource_bigtable_instance.go

 			old_cluster_order = append(old_cluster_order, c)
 		}
 	}
+	for _, value := range clusters {
+		old_cluster_order = append(old_cluster_order, value)
+	}

 	err := diff.SetNew("cluster", old_cluster_order)


this is producing "Provider produced inconsistent final plan" after the change.

rileykarson · 2019-09-20T17:58:43Z

This feature is being tracked at a new issue, hashicorp/terraform-provider-google#4318. (Sorry for the web of BT cluster updating issues! I think I missed an xref between them somewhere.) I've been working with a member of the BT team to have this functionality added directly to the BT client library as https://code-review.googlesource.com/c/gocloud/+/44410, which should land soon.

I'd strongly prefer we use that implementation instead of a separate one in TF. It's a commitment of mine to add it to the provider once that change lands. Given that, I'll probably take over the implementation unless you'd prefer to make the contribution.

FWIW, the error you encountered is because Terraform 0.12 and onwards are very strict that values shown in terraform plan match the ordering of values before/after terraform apply. I suspect that ordering in the CustomizeDiff is different than the real ordering applied in d.Set, causing the issue you encountered.

stankiewicz · 2019-09-20T18:10:34Z

@rileykarson I've spoken to Gary about that. If his change can land soon then great!
I think that in that case you can take over integration or I can help under your guidance if you don't have capacity. I have some and some work is done here already as same logic will need to be implemented. I know where is the issue but I don't know yet how to fix it.

stankiewicz · 2019-09-23T14:51:09Z

@rileykarson , I've updated code and tests, so I don't have any more "inconsistent final plan".

rileykarson · 2019-10-02T21:01:52Z

Sorry for the wait- I think https://code-review.googlesource.com/c/gocloud/+/44410 is close to merging.

rileykarson · 2019-10-22T19:52:20Z

third_party/terraform/resources/resource_bigtable_instance.go

@@ -335,29 +364,43 @@ func resourceBigtableInstanceClusterReorderTypeList(diff *schema.ResourceDiff, m
 		return fmt.Errorf("config is invalid: Too many cluster blocks: No more than 4 \"cluster\" blocks are allowed")
 	}

-	if old_count.(int) != new_count.(int) {


The validation above here is worth preserving. I'd expect the new CustomizeDiff can supersede the rest, though.

rileykarson · 2019-10-22T19:58:47Z

third_party/terraform/resources/resource_bigtable_instance.go

@@ -236,16 +234,47 @@ func resourceBigtableInstanceUpdate(d *schema.ResourceData, meta interface{}) er
 		clusterMap[cluster.Name] = cluster
 	}

+	clusterTodoMap := make(map[string]interface{}, d.Get("cluster.#").(int))


I'd expect all this updating logic will go away, and get replaced by expanders very similar (if not identical) to Create; https://github.com/googleapis/google-cloud-go/blob/master/bigtable/admin.go#L1290 presents the same interface as it.

If they end up identical, feel free to refactor into a shared "expandBigtableInstance" method or just copy/paste. Either is fine.

rileykarson · 2019-10-22T20:08:20Z

third_party/terraform/resources/resource_bigtable_instance.go

+	new_clusters := make(map[string]interface{}, new_count.(int))
+	old_clusters := make(map[string]interface{}, old_count.(int))
+
+	for i := 0; i < new_count.(int) || i < old_count.(int); i++ {


Do you mind commenting the blocks like this with what they're doing? While it's possible to figure out, customizeDiffs are pretty finicky and it's a big help to be able to read quickly what they're doing.

rileykarson · 2019-10-22T20:10:57Z

third_party/terraform/tests/resource_bigtable_instance_test.go

+		Steps: []resource.TestStep{
+			{
+				Config: testAccBigtableInstance_one_in_the_cluster(instanceName, 3),
+				Check: resource.ComposeTestCheckFunc(


After rebasing, you'll probably need to change to use "import-style" checks. See https://github.com/GoogleCloudPlatform/magic-modules/pull/2409/files#diff-1296b3b7cf174ed6b01d0139e46bdd24L69-R76.

If you're seeing differences on cluster in "ImportStateVerify" that you don't expect, flag them here and don't worry about fixing them before pushing.

rileykarson · 2020-01-07T22:02:16Z

Superseded with #2923

Thanks for taking the time to submit this! Sorry about the unfortunate timing between your work + the client lib work I'd been waiting to integrate, you did a great job with your changes.

expand, reduce bigtable

817cc15

googlebot added the cla: yes label Sep 20, 2019

modular-magician added the community label Sep 20, 2019

stankiewicz commented Sep 20, 2019

View reviewed changes

test for resize

cba6df7

fix plan

5e0c339

rileykarson reviewed Oct 22, 2019

View reviewed changes

rileykarson closed this Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add update support for bigtable clusters (such as resizing) #2350

Add update support for bigtable clusters (such as resizing) #2350

stankiewicz commented Sep 20, 2019

modular-magician commented Sep 20, 2019

stankiewicz commented Sep 20, 2019

stankiewicz commented Sep 20, 2019

stankiewicz Sep 20, 2019

rileykarson commented Sep 20, 2019

stankiewicz commented Sep 20, 2019

stankiewicz commented Sep 23, 2019

rileykarson commented Oct 2, 2019

rileykarson Oct 22, 2019

rileykarson Oct 22, 2019

rileykarson Oct 22, 2019

rileykarson Oct 22, 2019

rileykarson commented Jan 7, 2020

Add update support for bigtable clusters (such as resizing) #2350

Add update support for bigtable clusters (such as resizing) #2350

Conversation

stankiewicz commented Sep 20, 2019

modular-magician commented Sep 20, 2019

stankiewicz commented Sep 20, 2019

stankiewicz commented Sep 20, 2019

stankiewicz Sep 20, 2019

Choose a reason for hiding this comment

rileykarson commented Sep 20, 2019

stankiewicz commented Sep 20, 2019

stankiewicz commented Sep 23, 2019

rileykarson commented Oct 2, 2019

rileykarson Oct 22, 2019

Choose a reason for hiding this comment

rileykarson Oct 22, 2019

Choose a reason for hiding this comment

rileykarson Oct 22, 2019

Choose a reason for hiding this comment

rileykarson Oct 22, 2019

Choose a reason for hiding this comment

rileykarson commented Jan 7, 2020