Enable TPUs to use Shared VPC #3939

upodroid · 2020-09-01T21:09:32Z

Fixes: hashicorp/terraform-provider-google#7117

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)

tpu: added `use_service_networking` to `google_tpu_node` which enables Shared VPC Support.

Fork Update

modular-magician · 2020-09-01T21:10:42Z

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 · 2020-09-01T21:17:07Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 104 insertions(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 104 insertions(+), 30 deletions(-))
Ansible: Diff ( 2 files changed, 36 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 1 file changed, 21 insertions(+), 2 deletions(-))

modular-magician · 2020-09-01T21:20:09Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 105 insertions(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 105 insertions(+), 30 deletions(-))
Ansible: Diff ( 2 files changed, 36 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))
TF OiCS: Diff ( 1 file changed, 21 insertions(+), 2 deletions(-))

upodroid · 2020-09-01T21:25:05Z

Urgh, the google API takes the project name in the network field and holds it as a project number.

    TestAccTPUNode_tpuNodeFullExample: testing.go:674: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        DESTROY/CREATE: google_tpu_node.tpu
          accelerator_type:                "v3-8" => "v3-8"
          cidr_block:                      "10.96.0.0/29" => "<computed>"
          description:                     "Terraform Google Provider test TPU" => "Terraform Google Provider test TPU"
          id:                              "projects/REDACTED/locations/us-central1-b/nodes/tf-test-test-tpuv048js4jr2" => "<computed>"
          labels.foo:                      "bar" => "bar"
          name:                            "tf-test-test-tpuv048js4jr2" => "tf-test-test-tpuv048js4jr2"
          network:                         "projects/550924169191/global/networks/default" => "projects/REDACTED/global/networks/default" (forces new resource)
          network_endpoints:               "" => "<computed>" (forces new resource)
          network_endpoints.#:             "1" => "" (forces new resource)
          network_endpoints.0.ip_address:  "10.96.0.2" => "" (forces new resource)
          network_endpoints.0.port:        "8470" => "" (forces new resource)
          project:                         "REDACTED" => "<computed>"
          scheduling_config.#:             "1" => "1"
          scheduling_config.0.preemptible: "true" => "true"
          service_account:                 "[email protected]" => "<computed>"
          tensorflow_version:              "1.11" => "1.11"
          use_service_networking:          "true" => "true"
          zone:                            "us-central1-b" => "us-central1-b"
        
        
        
        STATE:
        
        data.google_compute_network.ne

upodroid · 2020-09-04T20:21:17Z

Tests are passing

modular-magician · 2020-09-04T20:27:31Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 112 insertions(+), 32 deletions(-))
Terraform Beta: Diff ( 3 files changed, 112 insertions(+), 32 deletions(-))
Ansible: Diff ( 2 files changed, 36 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 16 insertions(+))
TF OiCS: Diff ( 1 file changed, 21 insertions(+), 2 deletions(-))

megan07 · 2020-09-04T21:14:30Z

products/tpu/terraform.yaml

      schedulingConfig: !ruby/object:Overrides::Terraform::PropertyOverride
        diff_suppress_func: 'compareTpuNodeSchedulingConfig'
      schedulingConfig.preemptible: !ruby/object:Overrides::Terraform::PropertyOverride
        diff_suppress_func: 'compareTpuNodeSchedulingConfig'
+      network: !ruby/object:Overrides::Terraform::PropertyOverride


This seems to be overwriting the network block from line 36, and also, I don't think we want to always suppress this diff. What if the intent is to change the network, I don't think it'd recognize the change.

I thought it was suppressing the values the api returns from get call in the Read function.

Reworked it to check for a change in network.

I'm not sure this will make a huge difference, because if the user really wants to change the network, this will prevent them from doing so. I wonder if we could maybe do a diffsuppress that would parse the network and see if the only difference is the project and then check that the project number is the correct project number? Does that make sense? It's a little hacky, but would be the workaround until the upstream issue was solved.
Also, I think we want to use the diff_suppress_func from line 38 above rather than creating a new network block here. Then, in the new diff suppress func that you're implementing, make sure you include the same logic that is in compareSelfLinkOrResourceName so we don't lose that as well. Does that make sense?
Let me know if you have questions.

Have a look my last commit. How do I get the *Config in to the diffsupress function? I'm using other functions that expect the config block as arguments.

Hi @upodroid! What a finicky issue! I spent my morning looking into how we can do this. Although diff suppress would be ideal, it looks like we can't use it here. If we want access to config we'll have to use a CustomizeDiff.
Basically what I did in the CustomizeDiff was get the old (value using project number) and new (value using project name) values, used regex to find the submatch for project. Then called out to the projects API using the project name and if the returned project.Number was the same as the project number, then i did a diff.SetNew(old). Does that make sense? So basically if they're the same project, we're just going to set it to use the project number so there is no diff.
Let me know if you have questions! Thanks again!!

I amended it to use CustomizeDiff now but i'm still getting diffs.

nvm. It is ready to go.

REDACTED  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  66⬇  5✎  USAGE  $  make testacc TEST=./google-beta TESTARGS='-run=TestAccTPUNode_tpuNodeFullExample' ==> Checking source code against gofmt... ==> Checking that code complies with gofmt requirements... TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccTPUNode_tpuNodeFullExample -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc" === RUN TestAccTPUNode_tpuNodeFullExample === PAUSE TestAccTPUNode_tpuNodeFullExample === CONT TestAccTPUNode_tpuNodeFullExample --- PASS: TestAccTPUNode_tpuNodeFullExample (364.17s) PASS ok github.com/hashicorp/terraform-provider-google-beta/google-beta 366.543s

templates/terraform/constants/tpu_node.erb

upodroid · 2020-09-04T21:44:32Z

It is strange behaviour in the Google API.

POST API Call. Missing the network in the API call though.

020/09/04 21:12:57 [DEBUG] Retry Transport: request attempt 0
2020/09/04 21:12:57 [DEBUG] Google API Request Details:
---[ REQUEST ]---------------------------------------
POST /v1/projects/REDACTED/locations/us-central1-b/nodes?alt=json&nodeId=tf-test-test-tpuph6bmmfqw8 HTTP/1.1
Host: tpu.googleapis.com
User-Agent: HashiCorp Terraform/0.12.7-sdk (+https://www.terraform.io) Terraform Plugin SDK/1.11.0 terraform-provider-google-beta/acc
Content-Length: 232
Content-Type: application/json
Accept-Encoding: gzip

{
 "acceleratorType": "v3-8",
 "description": "Terraform Google Provider test TPU",
 "labels": {
  "foo": "bar"
 },
 "name": "tf-test-test-tpuph6bmmfqw8",
 "schedulingConfig": {
  "preemptible": true
 },
 "tensorflowVersion": "1.11",
 "useServiceNetworking": true
}

GET Call after the op is complete.

2020/09/04 21:15:13 [DEBUG] Google API Request Details:
---[ REQUEST ]---------------------------------------
GET /v1/projects/REDACTED/locations/us-central1-b/nodes/tf-test-test-tpuph6bmmfqw8?alt=json HTTP/1.1
Host: tpu.googleapis.com
User-Agent: HashiCorp Terraform/0.12.7-sdk (+https://www.terraform.io) Terraform Plugin SDK/1.11.0 terraform-provider-google-beta/acc
Content-Type: application/json
Accept-Encoding: gzip


-----------------------------------------------------
2020/09/04 21:15:14 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 200 OK
Connection: close
Transfer-Encoding: chunked
Alt-Svc: h3-29=":443"; ma=2592000,h3-27=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-T050=":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, 04 Sep 2020 20:15:14 GMT
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

2c9
{
  "name": "projects/REDACTED/locations/us-central1-b/nodes/tf-test-test-tpuph6bmmfqw8",
  "description": "Terraform Google Provider test TPU",
  "acceleratorType": "v3-8",
  "ipAddress": "10.71.0.2",
  "state": "READY",
  "tensorflowVersion": "1.11",
  "network": "projects/550924169191/global/networks/default",
  "cidrBlock": "10.71.0.0/29",
  "port": "8470",
  "serviceAccount": "[email protected]",
  "createTime": "2020-09-04T20:13:00.287428315Z",
  "schedulingConfig": {
    "preemptible": true
  },
  "networkEndpoints": [
    {
      "ipAddress": "10.71.0.2",
      "port": 8470
    }
  ],
  "labels": {
    "foo": "bar"
  },
  "useServiceNetworking": true
}

modular-magician · 2020-09-04T22:02:05Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 115 insertions(+), 32 deletions(-))
Terraform Beta: Diff ( 3 files changed, 115 insertions(+), 32 deletions(-))
Ansible: Diff ( 2 files changed, 36 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 19 insertions(+))
TF OiCS: Diff ( 1 file changed, 21 insertions(+), 2 deletions(-))

modular-magician · 2020-09-09T20:51:01Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 139 insertions(+), 31 deletions(-))
Terraform Beta: Diff ( 3 files changed, 139 insertions(+), 31 deletions(-))
Ansible: Diff ( 2 files changed, 37 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 43 insertions(+))
TF OiCS: Diff ( 1 file changed, 21 insertions(+), 2 deletions(-))

templates/terraform/examples/tpu_node_full.tf.erb

modular-magician · 2020-09-15T23:11:12Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 136 insertions(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 136 insertions(+), 30 deletions(-))
Ansible: Diff ( 2 files changed, 37 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 42 insertions(+))
TF OiCS: Diff ( 1 file changed, 20 insertions(+), 2 deletions(-))

modular-magician · 2020-09-16T09:08:49Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 135 insertions(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 135 insertions(+), 30 deletions(-))
Ansible: Diff ( 2 files changed, 37 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 41 insertions(+))
TF OiCS: Diff ( 1 file changed, 20 insertions(+), 2 deletions(-))

megan07 · 2020-09-16T16:11:49Z

/gcbrun

modular-magician · 2020-09-16T16:19:13Z

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

Diff report:

Terraform GA: Diff ( 3 files changed, 135 insertions(+), 30 deletions(-))
Terraform Beta: Diff ( 3 files changed, 135 insertions(+), 30 deletions(-))
Ansible: Diff ( 2 files changed, 37 insertions(+), 4 deletions(-))
TF Conversion: Diff ( 1 file changed, 41 insertions(+))
TF OiCS: Diff ( 1 file changed, 20 insertions(+), 2 deletions(-))

megan07

Looks good! Thank you!

upodroid added 4 commits August 3, 2020 12:29

Merge pull request #1 from GoogleCloudPlatform/master

71c3aeb

Fork Update

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

e1a9488

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

51eaff6

add support for shared vpc

6e215bc

google-cla bot added the cla: yes label Sep 1, 2020

modular-magician requested a review from megan07 September 1, 2020 21:10

make cidr_block computable

60d3923

add diffsuppression

a781e7b

megan07 suggested changes Sep 4, 2020

View reviewed changes

tweak diffsupression to ignore when network has a change

8a2eadf

rework diffsupress

72e2f5b

megan07 reviewed Sep 15, 2020

View reviewed changes

templates/terraform/examples/tpu_node_full.tf.erb Outdated Show resolved Hide resolved

add customizediff

7a99ede

switch the diff checks

5df6e20

megan07 approved these changes Sep 17, 2020

View reviewed changes

megan07 merged commit b5b2ceb into GoogleCloudPlatform:master Sep 17, 2020

modular-magician mentioned this pull request Sep 17, 2020

Enable TPUs to use Shared VPC terraform-google-modules/docs-examples#89

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable TPUs to use Shared VPC #3939

Enable TPUs to use Shared VPC #3939

upodroid commented Sep 1, 2020

modular-magician commented Sep 1, 2020

modular-magician commented Sep 1, 2020

modular-magician commented Sep 1, 2020

upodroid commented Sep 1, 2020 •

edited

Loading

upodroid commented Sep 4, 2020

modular-magician commented Sep 4, 2020

megan07 Sep 4, 2020

upodroid Sep 4, 2020

upodroid Sep 4, 2020

megan07 Sep 8, 2020

upodroid Sep 9, 2020 •

edited

Loading

megan07 Sep 15, 2020

upodroid Sep 15, 2020

upodroid Sep 16, 2020

upodroid commented Sep 4, 2020

modular-magician commented Sep 4, 2020

modular-magician commented Sep 9, 2020

modular-magician commented Sep 15, 2020

modular-magician commented Sep 16, 2020

megan07 commented Sep 16, 2020

modular-magician commented Sep 16, 2020

megan07 left a comment

Enable TPUs to use Shared VPC #3939

Enable TPUs to use Shared VPC #3939

Conversation

upodroid commented Sep 1, 2020

modular-magician commented Sep 1, 2020

modular-magician commented Sep 1, 2020

Diff report:

modular-magician commented Sep 1, 2020

Diff report:

upodroid commented Sep 1, 2020 • edited Loading

upodroid commented Sep 4, 2020

modular-magician commented Sep 4, 2020

Diff report:

megan07 Sep 4, 2020

Choose a reason for hiding this comment

upodroid Sep 4, 2020

Choose a reason for hiding this comment

upodroid Sep 4, 2020

Choose a reason for hiding this comment

megan07 Sep 8, 2020

Choose a reason for hiding this comment

upodroid Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

megan07 Sep 15, 2020

Choose a reason for hiding this comment

upodroid Sep 15, 2020

Choose a reason for hiding this comment

upodroid Sep 16, 2020

Choose a reason for hiding this comment

upodroid commented Sep 4, 2020

modular-magician commented Sep 4, 2020

Diff report:

modular-magician commented Sep 9, 2020

Diff report:

modular-magician commented Sep 15, 2020

Diff report:

modular-magician commented Sep 16, 2020

Diff report:

megan07 commented Sep 16, 2020

modular-magician commented Sep 16, 2020

Diff report:

megan07 left a comment

Choose a reason for hiding this comment

upodroid commented Sep 1, 2020 •

edited

Loading

upodroid Sep 9, 2020 •

edited

Loading