Add support for `processing_units` to `google_spanner_instance` #4993

upodroid · 2021-07-23T21:27:38Z

Fixes: hashicorp/terraform-provider-google#9599

This will be a breaking change as the new field is mutually exclusive with num_nodes.

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)

spanner: `num_nodes` will become a required field in the next major release and a value has to be set on `google_spanner_instance`. It will also conflict with the new field `processing_units`.

spanner: added `processing_units` to `google_spanner_instance`.

Co-authored-by: upodroid <cy@borg.dev>

modular-magician · 2021-07-23T21:27:53Z

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

breaking_change

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

modular-magician · 2021-07-23T21:29:01Z

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.

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

modular-magician · 2021-07-23T21:38:38Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 110 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 3 files changed, 110 insertions(+), 5 deletions(-))
Ansible: Diff ( 2 files changed, 29 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 4 files changed, 109 insertions(+))
Inspec: Diff ( 4 files changed, 8 insertions(+), 1 deletion(-))

c2thorn · 2021-07-23T21:44:19Z

mmv1/products/spanner/terraform.yaml

+      - !ruby/object:Provider::Terraform::Examples
+        name: "spanner_instance_processing_units"
+        primary_resource_id: "example"
+        # Randomness


What's the randomness here?

I don't know. Copied the other block

c2thorn · 2021-07-23T21:47:47Z

mmv1/products/spanner/api.yaml

-        default_value: 1
+        description: |
+          The number of nodes allocated to this instance. At most one of either node_count or processing_units can be present in terraforn. 
+        exactly_one_of:


Could conflicts be applied here instead of exactly_one_of? That way we keep them mutually exclusive but neither are required

c2thorn · 2021-07-23T21:48:45Z

mmv1/products/spanner/api.yaml

+        description: |
+          The number of nodes allocated to this instance. At most one of either node_count or processing_units can be present in terraforn. 
+        exactly_one_of:
+          - num_names


Suggested change

- num_names

- num_nodes

I tried that and it doesn't work. Trying conflicts_with

Pushed a commit with correct field name

brb

mmv1 is buggy.

I, [2021-07-23T22:17:44.793294 #95375] INFO -- : products/pubsub: Not specified, skipping generation bundler: failed to load command: compiler (compiler) RuntimeError: num_nodes does not exist /Users/maha4472/Desktop/Git/magic-modules/mmv1/api/type.rb:218:in `block in check_conflicts' /Users/maha4472/Desktop/Git/magic-modules/mmv1/api/type.rb:217:in `each' /Users/maha4472/Desktop/Git/magic-modules/mmv1/api/type.rb:217:in `check_conflicts' /Users/maha4472/Desktop/Git/magic-modules/mmv1/api/type.rb:136:in `validate' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:121:in `check_property_value' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:75:in `block in check' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:74:in `each' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:74:in `each_with_index' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:74:in `check' /Users/maha4472/Desktop/Git/magic-modules/mmv1/api/resource.rb:173:in `validate' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:121:in `check_property_value' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:75:in `block in check' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:74:in `each' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:74:in `each_with_index' /Users/maha4472/Desktop/Git/magic-modules/mmv1/google/yaml_validator.rb:74:in `check' /Users/maha4472/Desktop/Git/magic-modules/mmv1/api/product.rb:59:in `validate' compiler:178:in `block in <top (required)>' compiler:144:in `each' compiler:144:in `<top (required)>'

Regardless a schema like this doesn't work

"num_nodes": { Type: schema.TypeInt, Optional: true, Description: `The number of nodes allocated to this instance. At most one of either node_count or processing_units can be present in terraforn.`, Default: 1, ConflictsWith: []string{"processing_units"}, }, "processing_units": { Type: schema.TypeInt, Optional: true, Description: `The number of processing units allocated to this instance. At most one of processing_units or node_count can be present in terraform.`, ConflictsWith: []string{"num_nodes"}, },

Causes this error:

021/07/23 23:20:40 [DEBUG] Retry Transport: starting RoundTrip retry loop 2021/07/23 23:20:40 [DEBUG] Retry Transport: request attempt 0 2021/07/23 23:20:40 [DEBUG] Google API Request Details: ---[ REQUEST ]--------------------------------------- POST /v1/projects/REDACTED/instances?alt=json HTTP/1.1 Host: spanner.googleapis.com User-Agent: Terraform/0.14.11 (+https://www.terraform.io) Terraform-Plugin-SDK/2.5.0 terraform-provider-google/acc Content-Length: 231 Content-Type: application/json Accept-Encoding: gzip { "instance": { "config": "projects/REDACTED/instanceConfigs/regional-us-central1", "displayName": "Test Spanner Instance", "labels": { "foo": "bar" }, "nodeCount": 1, "processingUnits": 200 }, "instanceId": "tfgen-spanid-20210723222040904" } ----------------------------------------------------- 2021/07/23 23:20:41 [DEBUG] Google API Response Details: ---[ RESPONSE ]-------------------------------------- HTTP/2.0 400 Bad Request Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43" Cache-Control: private Content-Type: application/json; charset=UTF-8 Date: Fri, 23 Jul 2021 22:20:41 GMT Server: ESF Server-Timing: gfet4t7; dur=446 Vary: Origin Vary: X-Origin Vary: Referer X-Frame-Options: SAMEORIGIN X-Xss-Protection: 0 { "error": { "code": 400, "message": "Invalid CreateInstance request. At most one of node_count or processing_units may be specified.", "status": "INVALID_ARGUMENT" } } ----------------------------------------------------- 2021/07/23 23:20:41 [DEBUG] Retry Transport: Stopping retries, last request failed with non-retryable error: googleapi: got HTTP response code 400 with body: HTTP/2.0 400 Bad Request Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43" Cache-Control: private Content-Type: application/json; charset=UTF-8 Date: Fri, 23 Jul 2021 22:20:41 GMT Server: ESF Server-Timing: gfet4t7; dur=446 Vary: Origin Vary: X-Origin Vary: Referer X-Frame-Options: SAMEORIGIN X-Xss-Protection: 0 { "error": { "code": 400, "message": "Invalid CreateInstance request. At most one of node_count or processing_units may be specified.", "status": "INVALID_ARGUMENT" } } 2021/07/23 23:20:41 [DEBUG] Retry Transport: Returning after 1 attempts 2021/07/23 23:20:41 [WARN] Got error running Terraform: exit status 1 Error: Error creating Instance: googleapi: Error 400: Invalid CreateInstance request. At most one of node_count or processing_units may be specified. on terraform_plugin_test.tf line 2, in resource "google_spanner_instance" "example": 2: resource "google_spanner_instance" "example" { provider_test.go:268: Step 1/2 error: Error running apply: exit status 1 Error: Error creating Instance: googleapi: Error 400: Invalid CreateInstance request. At most one of node_count or processing_units may be specified. on terraform_plugin_test.tf line 2, in resource "google_spanner_instance" "example": 2: resource "google_spanner_instance" "example" { --- FAIL: TestAccSpannerInstance_spannerInstanceProcessingUnitsExample (18.62s) FAIL FAIL github.com/hashicorp/terraform-provider-google/google 21.535s FAIL

Removing the default value works but it is a breaking change

Also, you can't avoid ExactlyOneOf

2021/07/23 23:25:43 [DEBUG] Google API Request Details: ---[ REQUEST ]--------------------------------------- POST /v1/projects/REDSACTED/instances?alt=json HTTP/1.1 Host: spanner.googleapis.com User-Agent: Terraform/0.14.11 (+https://www.terraform.io) Terraform-Plugin-SDK/2.5.0 terraform-provider-google/acc Content-Length: 195 Content-Type: application/json Accept-Encoding: gzip { "instance": { "config": "projects/REDACTED/instanceConfigs/regional-us-central1", "displayName": "Test Spanner Instance", "labels": { "foo": "bar" } }, "instanceId": "tfgen-spanid-20210723222543515" } ----------------------------------------------------- 2021/07/23 23:25:44 [DEBUG] Google API Response Details: ---[ RESPONSE ]-------------------------------------- HTTP/2.0 400 Bad Request Alt-Svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43" Cache-Control: private Content-Type: application/json; charset=UTF-8 Date: Fri, 23 Jul 2021 22:25:44 GMT Server: ESF Server-Timing: gfet4t7; dur=406 Vary: Origin Vary: X-Origin Vary: Referer X-Frame-Options: SAMEORIGIN X-Xss-Protection: 0 { "error": { "code": 400, "message": "Invalid CreateInstance request.", "status": "INVALID_ARGUMENT", "details": [ { "@type": "type.googleapis.com/google.rpc.BadRequest", "fieldViolations": [ { "field": "instance.node_count", "description": "Node count must be a positive number." } ] } ] } } -

mmv1/products/spanner/api.yaml

modular-magician · 2021-07-23T22:22:57Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 110 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 3 files changed, 110 insertions(+), 5 deletions(-))
Ansible: Diff ( 2 files changed, 29 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 4 files changed, 109 insertions(+))
Inspec: Diff ( 4 files changed, 8 insertions(+), 1 deletion(-))

rileykarson · 2021-07-30T18:53:44Z

mmv1/products/spanner/api.yaml

@@ -102,8 +102,19 @@ objects:
        required: true
      - !ruby/object:Api::Type::Integer
        name: 'nodeCount'
-        description: 'The number of nodes allocated to this instance.'
-        default_value: 1


Drive-by note: While removing a default is a safe change, that's only if the field is moved to Optional + Computed. We may need to use a CustomizeDiff to simulate a default of 1 when processing_units is unset, as breaking a config like the following working would also be backwards-incompatible. (Changing API requirements make backwards compatibility difficult!)

resource "google_spanner_instance" "example" { config = "regional-us-central1" display_name = "Test Spanner Instance" labels = { "foo" = "bar" } }

How about if:

I write an encoder that sets num_nodes to 1 if processing_units is 0, mark num_nodes as O+C and remove the default.

Open an issue to make both of the fields ExactlyOneOf in v4 and remove the encoder?

File an issue to fix that bug in the ruby generator.

Yeah, that should also work. We should make sure to test a config w/o an explicit node_count if we don't already.

upodroid · 2021-07-30T21:54:21Z

This is resolved and ready to go now.

modular-magician · 2021-07-30T22:04:38Z

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

Diff report:

Terraform GA: Diff ( 4 files changed, 146 insertions(+), 3 deletions(-))
Terraform Beta: Diff ( 6 files changed, 149 insertions(+), 5 deletions(-))
Ansible: Diff ( 2 files changed, 29 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 14 insertions(+))
TF OiCS: Diff ( 4 files changed, 109 insertions(+))
Inspec: Diff ( 4 files changed, 8 insertions(+), 1 deletion(-))

mmv1/products/spanner/api.yaml

modular-magician · 2021-08-03T17:48:30Z

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

Diff report:

Terraform GA: Diff ( 4 files changed, 150 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 4 files changed, 150 insertions(+), 5 deletions(-))
Ansible: Diff ( 2 files changed, 29 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 14 insertions(+))
TF OiCS: Diff ( 4 files changed, 109 insertions(+))
Inspec: Diff ( 4 files changed, 8 insertions(+), 1 deletion(-))

c2thorn · 2021-08-03T18:12:44Z

/gcbrun

modular-magician · 2021-08-03T18:23:39Z

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

Diff report:

Terraform GA: Diff ( 4 files changed, 150 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 4 files changed, 150 insertions(+), 5 deletions(-))
Ansible: Diff ( 2 files changed, 29 insertions(+), 6 deletions(-))
TF Conversion: Diff ( 1 file changed, 14 insertions(+))
TF OiCS: Diff ( 4 files changed, 109 insertions(+))
Inspec: Diff ( 4 files changed, 8 insertions(+), 1 deletion(-))

modular-magician · 2021-08-03T19:50:13Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCloudbuildWorkerPool_basic|TestAccContainerNodePool_basic|TestAccContainerNodePool_basicWithClusterId|TestAccContainerNodePool_withNetworkConfig|TestAccContainerNodePool_nodeLocations|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_maxPodsPerNode|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_version|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_shieldedInstanceConfig|TestAccContainerNodePool_ephemeralStorageConfig|TestAccDataLossPreventionDeidentifyTemplate_dlpDeidentifyTemplateBasicExample|TestAccSpannerInstance_noNodeCountSpecified You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=199467

c2thorn · 2021-08-03T22:03:42Z

Confirmed that the change doesn't create a new diff for an existing config. Tests pass as well

…leCloudPlatform#4993) Co-authored-by: upodroid <cy@borg.dev> Co-authored-by: Cameron Thornton <camthornton@google.com>

add spanner processing units

6f1d016

Co-authored-by: upodroid <cy@borg.dev>

google-cla bot added the cla: yes label Jul 23, 2021

modular-magician requested a review from c2thorn July 23, 2021 21:29

c2thorn requested changes Jul 23, 2021

View reviewed changes

fix typo in ExactlyOneOf

dbde0a5

rileykarson reviewed Jul 30, 2021

View reviewed changes

upodroid mentioned this pull request Jul 30, 2021

Make num_nodes and processing_units mutually exclusive in google_spanner_instance hashicorp/terraform-provider-google#9685

Closed

add encoder and rework field logic

648a3e2

upodroid mentioned this pull request Jul 30, 2021

ExactlyOneOf doesn't work with mmv1 fields that have their names overriden in terraform.yaml hashicorp/terraform-provider-google#9689

Closed

c2thorn reviewed Aug 3, 2021

View reviewed changes

mmv1/products/spanner/api.yaml Outdated Show resolved Hide resolved

Update mmv1/products/spanner/api.yaml

3b6e132

c2thorn approved these changes Aug 3, 2021

View reviewed changes

c2thorn merged commit d9c69c1 into GoogleCloudPlatform:master Aug 3, 2021

c2thorn deleted the spanner-processing-units branch August 3, 2021 22:04

modular-magician mentioned this pull request Aug 3, 2021

Add support for processing_units to google_spanner_instance hashicorp/terraform-provider-google#9716

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `processing_units` to `google_spanner_instance` #4993

Add support for `processing_units` to `google_spanner_instance` #4993

upodroid commented Jul 23, 2021 •

edited

Loading

modular-magician commented Jul 23, 2021

modular-magician commented Jul 23, 2021

modular-magician commented Jul 23, 2021

c2thorn Jul 23, 2021

upodroid Jul 23, 2021

c2thorn Jul 23, 2021

c2thorn Jul 23, 2021

upodroid Jul 23, 2021

upodroid Jul 23, 2021

upodroid Jul 23, 2021

modular-magician commented Jul 23, 2021

rileykarson Jul 30, 2021

upodroid Jul 30, 2021

rileykarson Jul 30, 2021

upodroid commented Jul 30, 2021

modular-magician commented Jul 30, 2021

modular-magician commented Aug 3, 2021

c2thorn commented Aug 3, 2021

modular-magician commented Aug 3, 2021

modular-magician commented Aug 3, 2021

c2thorn commented Aug 3, 2021

Add support for processing_units to google_spanner_instance #4993

Add support for processing_units to google_spanner_instance #4993

Conversation

upodroid commented Jul 23, 2021 • edited Loading

modular-magician commented Jul 23, 2021

modular-magician commented Jul 23, 2021

modular-magician commented Jul 23, 2021

Diff report:

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 Jul 23, 2021

Diff report:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upodroid commented Jul 30, 2021

modular-magician commented Jul 30, 2021

Diff report:

modular-magician commented Aug 3, 2021

Diff report:

c2thorn commented Aug 3, 2021

modular-magician commented Aug 3, 2021

Diff report:

modular-magician commented Aug 3, 2021

c2thorn commented Aug 3, 2021

Add support for `processing_units` to `google_spanner_instance` #4993

Add support for `processing_units` to `google_spanner_instance` #4993

upodroid commented Jul 23, 2021 •

edited

Loading