Add Bigtable autoscaling configs to Instance #5803

kongweihan · 2022-03-09T19:36:19Z

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

bigtable: added support for `autoscaling_config` to `google_bigtable_instance`

modular-magician · 2022-03-09T19:36:34Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2022-03-09T19:37:16Z

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.

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

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

modular-magician · 2022-03-10T17:05:26Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2022-03-10T17:16:49Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2022-03-15T20:46:09Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2022-03-16T21:16:50Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

modular-magician · 2022-03-16T22:40:32Z

Oops! It looks like you're using an unknown release-note type in your changelog entries:

REPLACEME

Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md.

megan07 · 2022-03-21T20:08:39Z

/gcbrun

megan07 · 2022-03-21T20:22:59Z

Hi @kongweihan would you mind running go mod tidy to update the go.sum file please? Thanks!

kongweihan · 2022-03-22T05:07:00Z

@megan07 I see the changelog-checker passes now, is that ok or do I still need to update go.sum? I'm not very familiar with go as it says go: go.mod file not found in current directory or any parent directory;. I need to init the mod locally?

megan07 · 2022-03-22T16:34:48Z

Hi @kongweihan, sorry, we just started managing this in magic-modules, so I was confused. You'll want to go to our downstream, make the update to go.mod there, then copy the go.sum file over here. Let me know if you have more questions! Thanks!

megan07 · 2022-03-22T17:09:44Z

/gcbrun

kongweihan · 2022-03-22T17:24:42Z

@megan07 updated. I see that downstream repo has both bigtable 1.10.1 and 1.13 in go.sum. So I copied the lines over. Is that correct?

modular-magician · 2022-03-22T17:25:00Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 213 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 4 files changed, 213 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

modular-magician · 2022-03-22T17:40:16Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 213 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 4 files changed, 213 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

megan07

Sorry for the delay, I needed to build and run locally. Tests passed, but there are some formatting issues. If you wouldn't mind fixing those up, I can get this merged today. Thanks so much!

megan07 · 2022-03-23T14:08:18Z

mmv1/third_party/terraform/resources/resource_bigtable_instance.go

+										Type:     schema.TypeInt,
+										Required: true,
+										Description: `The minimum number of nodes for autoscaling.`,


Would you mind formatting these, please? When I ran it locally I saw changes to these 3 attributes.

Suggested change

Type: schema.TypeInt,

Required: true,

Description: `The minimum number of nodes for autoscaling.`,

Type: schema.TypeInt,

Required: true,

Description: `The minimum number of nodes for autoscaling.`,

Thanks! Will fix. And, looking at https://github.com/GoogleCloudPlatform/magic-modules/pull/5800/files
Should I also update the doc? This change will be directly available to customer right, so we should publish the doc change?

Oh, yes please! Sorry, I missed that. Thank you!

I updated these, PTAL. Thanks!

megan07 · 2022-03-23T14:10:09Z

mmv1/third_party/terraform/tests/resource_bigtable_instance_test.go

+				// Create Autoscaling config with 2 nodes.
+				Config: testAccBigtableInstance_autoscalingCluster(instanceName, 2, 5, 70),
+				Check: resource.ComposeTestCheckFunc(resource.TestCheckResourceAttr("google_bigtable_instance.instance",
+						"cluster.0.num_nodes", "2"),),


There was also some formatting on these lines of the tests as well. You can run make fmt in the downstream to find them all, that would be great.

Suggested change

"cluster.0.num_nodes", "2"),),

"cluster.0.num_nodes", "2")),

kevinsi4508 · 2022-03-23T18:05:00Z

mmv1/third_party/terraform/resources/resource_bigtable_instance.go

+							Type:        schema.TypeList,
+							Optional:    true,
+							MaxItems:    1,
+							Description: "A list of Autoscaling configurations. Only one element is used and allowed.",


Is there a reason why a list is used here but exactly one element is required?

Yea it's the only way to nest a object into the schema in terraform. More details is in yaqs. See internal design doc.

kevinsi4508 · 2022-03-23T18:08:53Z

mmv1/third_party/terraform/resources/resource_bigtable_instance.go

+									"min_nodes": {
+										Type:     schema.TypeInt,
+										Required: true,
+										Description: `The minimum number of nodes for autoscaling.`,


nit: "The minimum number of nodes to scale down to"?

Similarly, "Maximum number of nodes to scale up to."

kevinsi4508 · 2022-03-23T18:12:56Z

mmv1/third_party/terraform/resources/resource_bigtable_instance.go

+										Required: true,
+										Description: `The maximum number of nodes for autoscaling.`,
+									},
+									"cpu_target": {


Are we doing some minimum validation on these field? e.g. 0 < cpu_target <= 100?

At very minimum we should document this.

It says that in the description?

https://screenshot.googleplex.com/BrDdQ87KW3FZd6o

kevinsi4508 · 2022-03-23T18:18:21Z

mmv1/third_party/terraform/resources/resource_bigtable_instance.go

-		})
+		}
+		autoscaling_configs := cluster["autoscaling_config"].([]interface{})
+		if len(autoscaling_configs) > 0 {


What happens if len(autoscaling_configs) > 1? Are we silently dropping the rest of the configs?

There's a check on the field validating it has at most 1 element.

kevinsi4508 · 2022-03-23T18:23:06Z

mmv1/third_party/terraform/tests/resource_bigtable_instance_test.go

+				// Create Autoscaling config with 2 nodes.
+				Config: testAccBigtableInstance_autoscalingCluster(instanceName, 2, 5, 70),
+				Check: resource.ComposeTestCheckFunc(resource.TestCheckResourceAttr("google_bigtable_instance.instance",
+						"cluster.0.num_nodes", "2"),),


What are we checking here? only the allocated nodes?

Yes, since that might not be set in the config, I want to explicitly check it.

kevinsi4508 · 2022-03-23T18:23:25Z

mmv1/third_party/terraform/tests/resource_bigtable_instance_test.go

+			},
+			{
+				// Update Autoscaling configs.
+				Config: testAccBigtableInstance_autoscalingCluster(instanceName, 1, 5, 80),


Do we need to verify the updates?

By default the test asserts on all the config fields.

kevinsi4508 · 2022-03-23T18:26:45Z

mmv1/third_party/terraform/tests/resource_bigtable_instance_test.go

+				ImportStateVerifyIgnore: []string{"deletion_protection", "instance_type"}, // we don't read instance type back
+			},
+			{
+				// Disable Autoscaling without specifying num_nodes, it should should use the current node count, which is 2.


"it should use the current node count"

kevinsi4508 · 2022-03-23T18:32:21Z

mmv1/third_party/terraform/tests/resource_bigtable_instance_test.go

@@ -230,6 +355,26 @@ resource "google_bigtable_instance" "instance" {
 `, instanceName, instanceName, numNodes)
 }

+
+func testAccBigtableInstance_noNumNodes(instanceName string) string {


Interesting! I thought this is invalid. How can this work? Will users be confused?

This is invalid at create time but ok for update. We plan to keep this behavior in case people have config relying on this.

kevinsi4508 · 2022-03-23T18:34:26Z

mmv1/third_party/terraform/tests/resource_bigtable_instance_test.go

+				Check: resource.ComposeTestCheckFunc(resource.TestCheckResourceAttr("google_bigtable_instance.instance",
+						"cluster.0.num_nodes", "2"),),
+			},
+			{


Sorry! just curious. What does this step do?

Checking that if we create a cluster with autoscaling.min_nodes = X, the cluster will start with X nodes.

modular-magician · 2022-03-23T18:41:35Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 213 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 4 files changed, 213 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

modular-magician · 2022-03-23T18:55:18Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 4 files changed, 211 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 4 files changed, 211 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

modular-magician · 2022-03-23T21:24:23Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 219 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 219 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

modular-magician · 2022-03-23T21:35:37Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 219 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 219 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

modular-magician · 2022-03-23T21:52:29Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 216 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 216 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

Design at go/cbt-autoscaler-terraform Fixes hashicorp/terraform-provider-google#10758 ```release-note:enhancement bigtable: Added cluster autoscaling support ```

modular-magician · 2022-03-24T14:37:53Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 5 files changed, 216 insertions(+), 4 deletions(-))
Terraform Beta: Diff ( 5 files changed, 216 insertions(+), 4 deletions(-))
TF Validator: Diff ( 2 files changed, 14 insertions(+), 15 deletions(-))

megan07 · 2022-03-24T15:12:23Z

Looks good! Thank you for contributing!

kongweihan · 2022-03-24T15:13:09Z

Thanks! How's the release process? When will this be available to customers? @megan07

megan07 · 2022-03-24T15:24:56Z

We have some maintenance being done next week on our release process, so this will likely be released the following week.

kongweihan · 2022-03-24T16:05:14Z

How do I know when it's released?

rileykarson · 2022-03-24T19:43:34Z

@kongweihan: I've linked internal material covering this in chat!

Design at go/cbt-autoscaler-terraform Fixes hashicorp/terraform-provider-google#10758 ```release-note:enhancement bigtable: Added cluster autoscaling support ```

modular-magician requested a review from megan07 March 9, 2022 19:37

kongweihan force-pushed the kongwh-autoscaling branch from de2b6c8 to 079cc35 Compare March 10, 2022 17:05

kongweihan force-pushed the kongwh-autoscaling branch from 079cc35 to 8585d5f Compare March 10, 2022 17:16

kongweihan marked this pull request as ready for review March 10, 2022 17:17

kongweihan force-pushed the kongwh-autoscaling branch from 8585d5f to 103d6a7 Compare March 15, 2022 20:45

kongweihan force-pushed the kongwh-autoscaling branch from 103d6a7 to ced5b2f Compare March 16, 2022 21:16

kongweihan force-pushed the kongwh-autoscaling branch from ced5b2f to 5a63a12 Compare March 16, 2022 22:40

kongweihan force-pushed the kongwh-autoscaling branch 3 times, most recently from e548dcd to 62d3aa9 Compare March 21, 2022 20:00

kongweihan force-pushed the kongwh-autoscaling branch from 62d3aa9 to 371d05c Compare March 22, 2022 16:23

kongweihan force-pushed the kongwh-autoscaling branch from 371d05c to cb023b3 Compare March 22, 2022 16:58

megan07 suggested changes Mar 23, 2022

View reviewed changes

kongweihan force-pushed the kongwh-autoscaling branch from cb023b3 to 3cc504c Compare March 23, 2022 18:27

kevinsi4508 reviewed Mar 23, 2022

View reviewed changes

kongweihan force-pushed the kongwh-autoscaling branch from 3cc504c to 83ad138 Compare March 23, 2022 18:42

kongweihan force-pushed the kongwh-autoscaling branch 2 times, most recently from 548eebf to 688727b Compare March 23, 2022 21:23

kongweihan force-pushed the kongwh-autoscaling branch 2 times, most recently from 4a16aa7 to 7fe688d Compare March 23, 2022 21:39

Add Bigtable autoscaling configs to Instance

b03dd6a

Design at go/cbt-autoscaler-terraform Fixes hashicorp/terraform-provider-google#10758 ```release-note:enhancement bigtable: Added cluster autoscaling support ```

kongweihan force-pushed the kongwh-autoscaling branch from 7fe688d to b03dd6a Compare March 24, 2022 14:22

megan07 approved these changes Mar 24, 2022

View reviewed changes

megan07 merged commit a2ab8e4 into GoogleCloudPlatform:main Mar 24, 2022

This was referenced Mar 24, 2022

Add Bigtable autoscaling configs to Instance hashicorp/terraform-provider-google#11344

Merged

Add Bigtable autoscaling configs to Instance hashicorp/terraform-provider-google-beta#4150

Merged

modular-magician mentioned this pull request Mar 24, 2022

Add Bigtable autoscaling configs to Instance GoogleCloudPlatform/terraform-validator#604

Merged

5 tasks

Add Bigtable autoscaling configs to Instance #5803

Add Bigtable autoscaling configs to Instance #5803

Conversation

kongweihan commented Mar 9, 2022 • edited by slevenick

modular-magician commented Mar 9, 2022

modular-magician commented Mar 9, 2022

modular-magician commented Mar 10, 2022

modular-magician commented Mar 10, 2022

modular-magician commented Mar 15, 2022

modular-magician commented Mar 16, 2022

modular-magician commented Mar 16, 2022

megan07 commented Mar 21, 2022

megan07 commented Mar 21, 2022

kongweihan commented Mar 22, 2022

megan07 commented Mar 22, 2022

megan07 commented Mar 22, 2022

kongweihan commented Mar 22, 2022

modular-magician commented Mar 22, 2022

Diff report:

modular-magician commented Mar 22, 2022

Diff report:

megan07 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Mar 23, 2022

Diff report:

modular-magician commented Mar 23, 2022

Diff report:

modular-magician commented Mar 23, 2022

Diff report:

modular-magician commented Mar 23, 2022

Diff report:

modular-magician commented Mar 23, 2022

Diff report:

modular-magician commented Mar 24, 2022

Diff report:

megan07 commented Mar 24, 2022

kongweihan commented Mar 24, 2022 • edited

megan07 commented Mar 24, 2022

kongweihan commented Mar 24, 2022

rileykarson commented Mar 24, 2022

kongweihan commented Mar 9, 2022 •

edited by slevenick

kongweihan commented Mar 24, 2022 •

edited