add ability to update Bigtable instance cluster node count

[mackenziestarr]

This PR allows the Bigtable instance cluster num_nodes to updated without forcing the instance to be re-created. I also tried to support upgrading from a DEVELOPMENT to a PRODUCTION instance but the bigtable package only exposes that information at instance creation time. Switching to the v2 admin API would allow this via Instance.GetType but for now I've left the instance_type schema as ForceNew. All tests under make testacc TEST=./google TESTARGS='-run=TestAccBigtableInstance*' pass for me.

fixes #2521

[rileykarson]

Hey @mackenziestarr! Thanks for your contribution. Unfortunately, I believe that changing the type of cluster from TypeSet to TypeList will count as a breaking change (as order will suddenly matter, resulting in diffs for some users), blocking us from applying this change until the 3.0.0 release of the provider.

Assuming the functionality discussed at the end in #2521 is exposed in the client libraries, we'll be able to add this in a future 2.X series release. If that's the case, we'll supersede this PR with one exposing it there.

[mackenziestarr]

Hi @rileykarson thanks for taking a look. I converted that to TypeList because of the issue with TypeSet elements with ForceNew causing the entire resource to be recreated reference here hashicorp/terraform#15420 (comment)

A change in any property in the set acts as if the entire set has changed (and if any of the properties in the set are ForceNew=true, then the entire resource gets re-created)

I see your point about TypeList not being ideal because it enforces ordering where there shouldn't be any. Is there a way to get around the bug above with TypeSet (possibly using a different structure for the implementation that is transparent to user's existing configs) — it doesn't seem to be a limitation of the Bigtable APIs as described in #2521 but an actual bug / unexpected property of using TypeSet

[rileykarson]

I'm not aware of a workaround for the issue that wouldn't constitute a breaking change for users (I looked into the same topic a few days ago). I see two possible paths forward:

• Convert this field to a TypeList in 3.0.0, assuming clusters are ordered when returned from the API. We have a limited ability to suppress diffs when they're returned unordered that we've used before, but it isn't transparent- I believe if any field has changed Terraform returns an unintuitive diff.

• The other gets a little gritty. Terraform can't tell which set elements have been modified / removed at update time. When an API only allows us to call addX/modifyX/removeX endpoints on subfields (effectively, this is how BT Cluster behaves) instead of supporting PUT or PATCH on the parent, we can't update the subfield. Terraform can tell the final state of the field, so a PUT or PATCH endpoint or a similar function in a client library would both let us add / remove clusters and perform size updates transparently even while we continue to use TypeSet.

[mackenziestarr]

@rileykarson just to clarify on the second option, how would that work with fields that can't be changed like cluster_id (and therefore should be marked in the schema with ForceNew)? Would the new API endpoint need to accept that in the PUT / PATCH payload and throw a validation error? Thanks for all the great info so far, this is my first contribution.

[rileykarson]

Ideally the client lib / API would throw a validation error if we attempted an impossible change, but we can work around it if it doesn't.

This functionality was recently added to the grpc client with https://code-review.googlesource.com/c/gocloud/+/42850. With that implementation, adding/removing clusters isn't possible (which could simplify our client code considerably 🙁 ). Regardless, it means we don't need to own much code in our Update fn and can just send our entire end state to the client lib.

Looking at this again, I think that we can safely make this change with approach #1, provided we can perform diff comparisons in a CustomizeDiff to avoid a breaking change. We've treated a TypeList as unordered in the past in GCE Subnetwork with https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_subnetwork.go#L164, if we add a similar custom diff function here we can do the same.

I believe all we need to do is make the same schema changes you've made, and add the CD to make sure that ordering of the list doesn't matter. CustomizeDiff functions can be pretty finicky- let me know if you'd like to take a shot at the implementation. If not, I can take over on implementing it.

[mackenziestarr]

hey @rileykarson thanks for all the info I think with the prior art there I can take a shot at implementing a CustomizeDiff function for the change, I'll try to be back with an update soon.

[rileykarson]

Sounds good! If you're stuck, feel free to reach out here.

[mackenziestarr]

Hey @rileykarson when I went to implement this change I noticed this from the ResourceDiff docs

All functions in ResourceDiff, save for ForceNew, can only be used on computed fields.

Given that, should I expect to have to modify the schema similar to this PR?

[rileykarson]

Yep!

Because of that change, in the customizediff we'll need to verify that the new # for cluster is at least 1 to simulate it being Required: true and MinItems: 1. You can see the equivalent value being pulled into newCount in this line: https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_compute_subnetwork.go#L169

[mackenziestarr]

Hey @rileykarson I managed to get a passing test for reordering clusters when making changes but I still need to handle the reordering cases where additional clusters are added / removed. Hopefully I'll have that soon!

[rileykarson]

If adding / removing clusters isn't necessary for your usecase, I'd prefer to omit it for now I think. If you return nil in the cases where count doesn't match at https://github.com/terraform-providers/terraform-provider-google/pull/4026/files#diff-97a6b5054641d12f7b5a4b1aafe531b0R322-R328 I think that's sufficient? Terraform will display a (probably ugly, but workable) diff that causes the instance to recreate.

I also left some surface-level comments on a couple pieces of the customizediff.

[rileykarson]

Good catch!

[mackenziestarr]

@rileykarson refactored the code a bit in 376017d to make it cleaner and fixed up the diff with regard to #4156, thanks for the review!

[mackenziestarr]

@rileykarson just bumping this, I think it’s ready to be merged

[rileykarson]

Sorry! The notification that you'd updated the PR got eaten, thanks for pinging me.

Just a couple more small things commented inline.

[mackenziestarr]

@rileykarson no worries, that happens to me as well with github notifications sometimes — just updated with some new test cases to cover the failure conditions you pointed out.

[rileykarson]

LGTM, thanks for contributing @mackenziestarr!

I'll upstream these changes to Magic Modules so they're applied to the beta provider as well, and then merge this PR.

[rileykarson]

Adding changelog metadata:

`num_nodes` can now be updated in `google_bigtable_instance`

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!