Add remaining fields to TPU VM resource #9308

roaks3 · 2023-10-19T13:28:59Z

b/292585830

This adds the remaining fields to google_tpu_v2_vm that are available from the API.

Notable exceptions (which may end up making it into this PR):

accelerator_config
- This conflicts with accelerator_type, and we'll need to work around the existing default behavior

One other interesting piece is service_account.scopes. If the user specifies this field but does not include https://www.googleapis.com/auth/pubsub, it will be automatically added by the server, so I've added a diff_suppress to ignore that value.

Release Note Template for Downstream PRs (will be copied)

tpuv2: added more fields to `google_tpu_v2_vm` resource, including `network_config`, `scheduling_config`, `shielded_instance_config`, `service_account` and `data_disks`

modular-magician · 2023-10-19T13:40:55Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 218 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1247 insertions(+), 34 deletions(-))
TF Conversion: Diff ( 1 file changed, 268 insertions(+))
TF OiCS: Diff ( 1 file changed, 52 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-19T14:59:40Z

Tests analytics

Total tests: 3162
Passed tests 2840
Skipped tests: 318
Affected tests: 4

Action taken

Found 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAttachedCluster_containerAttachedClusterFullExample|TestAccContainerAttachedCluster_update|TestAccTpuV2Vm_update|TestAccTpuV2Vm_tpuV2VmFullExample

Get to know how VCR tests work

modular-magician · 2023-10-19T15:12:29Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerAttachedCluster_containerAttachedClusterFullExample[Debug log]
TestAccContainerAttachedCluster_update[Debug log]
TestAccTpuV2Vm_tpuV2VmFullExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccTpuV2Vm_update[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

modular-magician · 2023-10-19T15:37:57Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 218 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1255 insertions(+), 34 deletions(-))
TF Conversion: Diff ( 1 file changed, 268 insertions(+))
TF OiCS: Diff ( 1 file changed, 52 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-19T16:50:49Z

Tests analytics

Total tests: 3162
Passed tests 2841
Skipped tests: 318
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerAttachedCluster_containerAttachedClusterFullExample|TestAccContainerAttachedCluster_update|TestAccTpuV2Vm_update

Get to know how VCR tests work

modular-magician · 2023-10-19T16:58:44Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerAttachedCluster_containerAttachedClusterFullExample[Debug log]
TestAccContainerAttachedCluster_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccTpuV2Vm_update[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

modular-magician · 2023-10-19T17:33:50Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 218 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1263 insertions(+), 32 deletions(-))
TF Conversion: Diff ( 1 file changed, 268 insertions(+))
TF OiCS: Diff ( 1 file changed, 52 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-19T18:52:24Z

Tests analytics

Total tests: 3164
Passed tests 2840
Skipped tests: 318
Affected tests: 6

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerNodePool_fastSocket|TestAccContainerNodePool_012_ConfigModeAttr|TestAccDataSourceGoogleServiceAccountJwt|TestAccDataSourceGoogleServiceAccountAccessToken_basic|TestAccTpuV2Vm_update|TestAccWorkstationsWorkstationConfig_workstationConfigAcceleratorsExample

Get to know how VCR tests work

roaks3 · 2023-10-19T19:16:08Z

I've added you both for initial review since this should be done (except for the additional fields I will be trying to add), and tests have passed locally, but it is just taking a very long time to get the VCR tests in a happy place.

I already have a change queued up to attempt adding the networking fields as well, just waiting on VCR to complete to do that.

@rileykarson I added you since I know you have context but feel free to re-assign.

modular-magician · 2023-10-19T19:40:22Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerNodePool_012_ConfigModeAttr[Debug log]
TestAccDataSourceGoogleServiceAccountAccessToken_basic[Debug log]
TestAccTpuV2Vm_update[Debug log]
TestAccWorkstationsWorkstationConfig_workstationConfigAcceleratorsExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_fastSocket[Error message] [Debug log]
TestAccDataSourceGoogleServiceAccountJwt[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

modular-magician · 2023-10-19T19:52:29Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 257 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1410 insertions(+), 32 deletions(-))
TF Conversion: Diff ( 1 file changed, 300 insertions(+))
TF OiCS: Diff ( 1 file changed, 72 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-19T21:03:24Z

Tests analytics

Total tests: 3163
Passed tests 2841
Skipped tests: 317
Affected tests: 5

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerNodePool_012_ConfigModeAttr|TestAccProjectIamPolicy_invalidMembers|TestAccTpuV2Vm_tpuV2VmFullExample|TestAccTpuV2Vm_update|TestAccWorkstationsWorkstationConfig_workstationConfigAcceleratorsExample

Get to know how VCR tests work

rileykarson

Partial pass, I'm still working through updatable fields.

mmv1/templates/terraform/constants/tpu_vm.go.erb

mmv1/products/tpuv2/Vm.yaml

rileykarson

OK, pass done- was shorter to finish than I expected.

mmv1/products/tpuv2/Vm.yaml

mmv1/third_party/terraform/services/tpuv2/resource_tpu_v2_vm_test.go.erb

modular-magician · 2023-10-19T21:53:12Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerNodePool_012_ConfigModeAttr[Debug log]
TestAccProjectIamPolicy_invalidMembers[Debug log]
TestAccTpuV2Vm_tpuV2VmFullExample[Debug log]
TestAccTpuV2Vm_update[Debug log]
TestAccWorkstationsWorkstationConfig_workstationConfigAcceleratorsExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

roaks3 · 2023-10-19T21:54:18Z

Oh good, it looks like the firewall issue I was seeing was specific to my local environment 🎉

rileykarson

LGTM, nothing blocking left on my end- it'd be nice to test updating a disk mode too, but as you mentioned that wasn't succeeding for API business logic reasons.

mmv1/products/tpuv2/Vm.yaml

modular-magician · 2023-10-20T16:13:33Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 258 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1399 insertions(+), 32 deletions(-))
TF Conversion: Diff ( 1 file changed, 302 insertions(+))
TF OiCS: Diff ( 1 file changed, 72 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-20T17:24:36Z

Tests analytics

Total tests: 3163
Passed tests 2844
Skipped tests: 317
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryDataTable_bigtable|TestAccTpuV2Vm_update

Get to know how VCR tests work

modular-magician · 2023-10-20T17:34:04Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Debug log]
TestAccTpuV2Vm_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

zeleena · 2023-10-19T23:01:53Z

mmv1/products/tpuv2/Vm.yaml

+    item_type: Api::Type::String
+  - !ruby/object:Api::Type::String
+    name: 'state'
+    output: true


I see that output is true here, but does that mean it is output only? Want to check that it cannot be set in the .tf file. Also, are the output-only fields typically defined here in the resource yaml?

Yep, output: true is our equivalent to output-only; it cannot be set by the user. And yea, this is where we specify that for mmv1 resources.

I see. It didn't seem required to state the outputs when I tested it and I didn't see this in some of the other resource files I checked. This seems fine, just curious if this is the preferred method since I'm not sure if I saw any difference when output fields were omitted.

Yea, you won't typically see a difference in output fields when testing, but we include them to better support users. They can feed outputs into other config or automation they may be using.

mmv1/templates/terraform/examples/tpu_v2_vm_full.tf.erb

zeleena · 2023-10-20T17:54:23Z

mmv1/third_party/terraform/services/tpuv2/resource_tpu_v2_vm_test.go.erb

+  tags = ["foo"]
+
+  lifecycle {
+    prevent_destroy = true


Why do we want prevent_destroy to be true?

This is sort of a trick I've seen used a few times that might be adopted more. The point of this type of test is to create the resource, update it in-place, and then destroy it. However, by default the "update in-place" requirement is not enforced, so changing an immutable field will still pass, and since that will recreate the resource, you can no longer tell if any of the fields are appropriately configured in terms of mutability.

So the workaround you see here ensures that Terraform will error if the resource needs to be recreated, so you know that only in-place updates are taking place if the test passes. Then, in order to clean up after the test is done, we need to set prevent_destroy = false, and then finally delete the resource.

Longer term we'll be able to use hashicorp/terraform-plugin-testing#61 to assert properties of the test plan

mmv1/third_party/terraform/services/tpuv2/resource_tpu_v2_vm_test.go.erb

modular-magician · 2023-10-20T19:46:30Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 262 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1407 insertions(+), 32 deletions(-))
TF Conversion: Diff ( 1 file changed, 302 insertions(+))
TF OiCS: Diff ( 1 file changed, 72 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-20T20:57:14Z

Tests analytics

Total tests: 3164
Passed tests 2845
Skipped tests: 317
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccSpannerInstanceIamPolicy|TestAccTpuV2Vm_tpuV2VmFullExample

Get to know how VCR tests work

modular-magician · 2023-10-20T21:06:53Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccSpannerInstanceIamPolicy[Debug log]
TestAccTpuV2Vm_tpuV2VmFullExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

modular-magician · 2023-10-20T23:06:00Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 262 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1407 insertions(+), 32 deletions(-))
TF Conversion: Diff ( 1 file changed, 302 insertions(+))
TF OiCS: Diff ( 1 file changed, 72 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-21T00:22:41Z

Tests analytics

Total tests: 3170
Passed tests 2849
Skipped tests: 320
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccTpuV2Vm_update

Get to know how VCR tests work

modular-magician · 2023-10-21T00:30:54Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccTpuV2Vm_update[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

This reverts commit 5dc9f2e.

modular-magician · 2023-10-21T02:53:16Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 272 insertions(+))
Terraform Beta: Diff ( 4 files changed, 1575 insertions(+), 74 deletions(-))
TF Conversion: Diff ( 1 file changed, 302 insertions(+))
TF OiCS: Diff ( 1 file changed, 72 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_tpu_v2_vm (5 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_tpu_v2_vm" "primary" {
  scheduling_config {
    reserved = # value needed
  }
}

modular-magician · 2023-10-21T04:04:45Z

Tests analytics

Total tests: 3170
Passed tests 2850
Skipped tests: 320
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

roaks3 requested review from zeleena and rileykarson October 19, 2023 19:13

rileykarson reviewed Oct 19, 2023

View reviewed changes

mmv1/products/tpuv2/Vm.yaml Show resolved Hide resolved

mmv1/third_party/terraform/services/tpuv2/resource_tpu_v2_vm_test.go.erb Show resolved Hide resolved

rileykarson approved these changes Oct 20, 2023

View reviewed changes

mmv1/products/tpuv2/Vm.yaml Show resolved Hide resolved

zeleena reviewed Oct 20, 2023

View reviewed changes

mmv1/templates/terraform/examples/tpu_v2_vm_full.tf.erb Outdated Show resolved Hide resolved

zeleena reviewed Oct 20, 2023

View reviewed changes

roaks3 added 8 commits October 20, 2023 22:42

Revert "Skip generated tests for google_tpu_v2_vm (#9315)"

36ea69f

This reverts commit 5dc9f2e.

Add remaining fields to TPU VM resource

9fe9e7a

Change update test to use preemptible vm

0c58bc1

Retain disk in test so that it isn't removed prematurely

0c26d55

Add networking fields

156f2f4

Respond to PR comments

22f4aa3

More tweaks from PR comments

bde527d

Tweak test names based on feedback

2188f6b

roaks3 force-pushed the tpu-vm-full branch from 612026a to 2188f6b Compare October 21, 2023 02:42

roaks3 merged commit b48fc2d into main Oct 23, 2023
6 checks passed

roaks3 deleted the tpu-vm-full branch October 23, 2023 13:12

roaks3 mentioned this pull request Oct 23, 2023

Add accelerator_config to google_tpu_v2_vm #9332

Merged

Add remaining fields to TPU VM resource #9308

Add remaining fields to TPU VM resource #9308

Conversation

roaks3 commented Oct 19, 2023 • edited Loading

modular-magician commented Oct 19, 2023

Diff report

Missing test report

modular-magician commented Oct 19, 2023

Tests analytics

Action taken

modular-magician commented Oct 19, 2023

Rerun these tests in REPLAYING mode to catch issues

modular-magician commented Oct 19, 2023

Diff report

Missing test report

modular-magician commented Oct 19, 2023

Tests analytics

Action taken

modular-magician commented Oct 19, 2023

Rerun these tests in REPLAYING mode to catch issues

modular-magician commented Oct 19, 2023

Diff report

Missing test report

modular-magician commented Oct 19, 2023

Tests analytics

Action taken

roaks3 commented Oct 19, 2023

modular-magician commented Oct 19, 2023

Rerun these tests in REPLAYING mode to catch issues

modular-magician commented Oct 19, 2023

Diff report

Missing test report

modular-magician commented Oct 19, 2023

Tests analytics

Action taken

rileykarson left a comment

Choose a reason for hiding this comment

rileykarson left a comment

Choose a reason for hiding this comment

modular-magician commented Oct 19, 2023

Rerun these tests in REPLAYING mode to catch issues

roaks3 commented Oct 19, 2023

rileykarson left a comment

Choose a reason for hiding this comment

modular-magician commented Oct 20, 2023

Diff report

Missing test report

modular-magician commented Oct 20, 2023

Tests analytics

Action taken

modular-magician commented Oct 20, 2023

Rerun these tests in REPLAYING mode to catch issues

zeleena Oct 19, 2023

Choose a reason for hiding this comment

roaks3 Oct 20, 2023

Choose a reason for hiding this comment

zeleena Oct 20, 2023

Choose a reason for hiding this comment

roaks3 Oct 20, 2023

Choose a reason for hiding this comment

zeleena Oct 20, 2023

Choose a reason for hiding this comment

roaks3 Oct 20, 2023

Choose a reason for hiding this comment

rileykarson Oct 20, 2023

Choose a reason for hiding this comment

modular-magician commented Oct 20, 2023

Diff report

Missing test report

modular-magician commented Oct 20, 2023

Tests analytics

Action taken

modular-magician commented Oct 20, 2023

Rerun these tests in REPLAYING mode to catch issues

modular-magician commented Oct 20, 2023

Diff report

Missing test report

modular-magician commented Oct 21, 2023

Tests analytics

Action taken

roaks3 commented Oct 19, 2023 •

edited

Loading