Stop reading data_disk_type value from API response into state of google_notebooks_instance resource #6512

SarahFrench · 2022-09-06T10:58:41Z

Description

Fixes hashicorp/terraform-provider-google#8485

That issue linked above describes a permadiff/permadelete situation where:

the Terraform config defines a type of disk to use in data_disk_type
the notebook resource is created
after creation, TF reads the resource from the API and overwrites state
The API response doesn't include the data_disk_type value, so it is set to "" in state
On the next plan TF identifies that the config has a non-zero value and the state has a zero value, so proposes a change
The change is on a ForceNew field

This PR stops the data_disk_type value being read from the API to overwrite state.

This matches how the boot disk arguments are handled:

magic-modules/mmv1/products/notebooks/terraform.yaml

Lines 70 to 73 in ea34ac2

    
           bootDiskSizeGb: !ruby/object:Overrides::Terraform::PropertyOverride 
        
             ignore_read: true 
        
           bootDiskType: !ruby/object:Overrides::Terraform::PropertyOverride 
        
             ignore_read: true

List

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).~~ N/A
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
- Tests starting TestAccNotebooksInstance_ pass
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

notebooks: fixed perma-diff in `google_notebooks_instance`

modular-magician · 2022-09-06T10:59:45Z

Hello! I am a robot who works on Magic Modules PRs.

I've detected that you're a community contributor. @ScottSuarez, a repository maintainer, has been assigned to assist you and help review your changes.

❓ First time contributing? Click here for more details

Your assigned reviewer will help review your code by:

Ensuring it's backwards compatible, covers common error cases, etc.
Summarizing the change into a user-facing changelog note.
Passes tests, either our "VCR" suite, a set of presubmit tests, or with manual test runs.

You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails.

If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox.

modular-magician · 2022-09-06T11:10:46Z

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

Diff report:

Terraform GA: Diff ( 2 files changed, 9 insertions(+), 17 deletions(-))
Terraform Beta: Diff ( 2 files changed, 9 insertions(+), 17 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-09-06T12:14:09Z

Tests analytics

Total tests: 2156
Passed tests 1916
Skipped tests: 234
Failed tests: 6

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests

TestAccComputeInstance_soleTenantNodeAffinities|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccCloudRunService_cloudRunServiceStaticOutboundExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccSqlDatabaseInstance_SqlServerAuditConfig|TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange

modular-magician · 2022-09-06T12:28:44Z

Tests passed during RECORDING mode:
TestAccComputeGlobalForwardingRule_internalLoadBalancing[Debug log]
TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample[Debug log]

Tests failed during RECORDING mode:
TestAccComputeInstance_soleTenantNodeAffinities[Error message] [Debug log]
TestAccCloudRunService_cloudRunServiceStaticOutboundExample[Error message] [Debug log]
TestAccSqlDatabaseInstance_SqlServerAuditConfig[Error message] [Debug log]
TestAccSqlDatabaseInstance_withPrivateNetwork_withAllocatedIpRange[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

ScottSuarez · 2022-09-06T20:12:18Z

Do we have an example of what the api is returning? Strange that it's getting set to "" as that should only happen if the api is returning that value. I could also be off base here on my own knowledge

SarahFrench · 2022-09-07T13:11:08Z

Yeah I was surprised that there wasn't a more explicit problem when trying to access a field that isn't present in the response. Here's some info about the API responses:

The documentation for GET https://notebooks.googleapis.com/v1/{name} says that the API response should look like this and include fields such as dataDiskType and bootDiskType at the top level:

{
  "name": string,
  ...
  "bootDiskType": enum (DiskType),
  "bootDiskSizeGb": string,
  "dataDiskType": enum (DiskType),
  "dataDiskSizeGb": string,
  ...
  "disks": [
      {
        object (Disk)
      }
    ],
  ...
}

The response doesn't have those top-level fields and only has the disks array:

{
  "name": "projects/[PROJECT_ID]/locations/us-central1-a/instances/notebook-disk-type-test-jupyter-instance",

  ...

  "disks": [
      {
        "autoDelete": true,
        "boot": true,
        "deviceName": "boot",
        "diskSizeGb": "110",
        "guestOsFeatures": [
          {
            "type": "VIRTIO_SCSI_MULTIQUEUE"
          },
          {
            "type": "UEFI_COMPATIBLE"
          },
          {
            "type": "GVNIC"
          }
        ],
        "interface": "SCSI",
        "kind": "compute#attachedDisk",
        "licenses": [
           ...
        ],
        "mode": "READ_WRITE",
        "source": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/zones/us-central1-a/disks/notebook-disk-type-test-jupyter-instance-boot",
        "type": "PERSISTENT"
      },
      {
        "deviceName": "data",
        "diskSizeGb": "110",
        "index": "1",
        "interface": "SCSI",
        "kind": "compute#attachedDisk",
        "mode": "READ_WRITE",
        "source": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/zones/us-central1-a/disks/notebook-disk-type-test-jupyter-instance-data",
        "type": "PERSISTENT"
      }
  ],
}

Expand to see the full response

---[ REQUEST ]---------------------------------------
GET /v1/projects/[PROJECT_ID]/locations/us-central1-a/instances/notebook-disk-type-test-jupyter-instance?alt=json HTTP/1.1
Host: notebooks.googleapis.com
User-Agent: Terraform/1.2.5 (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google/dev
Content-Type: application/json
Accept-Encoding: gzip


-----------------------------------------------------: timestamp=2022-09-07T13:04:27.240+0100
2022-09-07T13:04:28.155+0100 [INFO]  provider.terraform-provider-google_v4.33.0_x5: 2022/09/07 13:04:28 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Alt-Svc: h3=":443"; ma=2592000,h3-29=":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: Wed, 07 Sep 2022 12:04:28 GMT
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

{
  "name": "projects/[PROJECT_ID]/locations/us-central1-a/instances/notebook-disk-type-test-jupyter-instance",
  "containerImage": {
    "repository": "gcr.io/deeplearning-platform-release/base-cpu",
    "tag": "latest"
  },
  "serviceAccount": "[email protected]",
  "machineType": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/zones/us-central1-a/machineTypes/e2-medium",
  "state": "PROVISIONING",
  "network": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/global/networks/default",
  "subnet": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/regions/us-central1/subnetworks/default",
  "labels": {
    "goog-caip-notebook": ""
  },
  "metadata": {
    "notebooks-api": "PROD",
    "enable-guest-attributes": "TRUE",
    "container": "gcr.io/deeplearning-platform-release/base-cpu:latest",
    "terraform": "true",
    "proxy-mode": "service_account",
    "shutdown-script": "/opt/deeplearning/bin/shutdown_script.sh"
  },
  "createTime": "2022-09-07T12:04:15.775679808Z",
  "updateTime": "2022-09-07T12:04:25.182117192Z",
  "disks": [
    {
      "autoDelete": true,
      "boot": true,
      "deviceName": "boot",
      "diskSizeGb": "110",
      "guestOsFeatures": [
        {
          "type": "VIRTIO_SCSI_MULTIQUEUE"
        },
        {
          "type": "UEFI_COMPATIBLE"
        },
        {
          "type": "GVNIC"
        }
      ],
      "interface": "SCSI",
      "kind": "compute#attachedDisk",
      "licenses": [
        "https://www.googleapis.com/compute/v1/projects/click-to-deploy-images/global/licenses/c2d-tensorflow",
        "https://www.googleapis.com/compute/v1/projects/click-to-deploy-images/global/licenses/c2d-dl-platform-gvnic",
        "https://www.googleapis.com/compute/v1/projects/click-to-deploy-images/global/licenses/c2d-dl-platform-common-container",
        "https://www.googleapis.com/compute/v1/projects/click-to-deploy-images/global/licenses/c2d-dl-platform-debian-10",
"https://www.googleapis.com/compute/v1/projects/click-to-deploy-images/global/licenses/c2d-dl-platform-dlvm"
      ],
      "mode": "READ_WRITE",
      "source": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/zones/us-central1-a/disks/notebook-disk-type-test-jupyter-instance-boot",
      "type": "PERSISTENT"
    },
    {
      "deviceName": "data",
      "diskSizeGb": "110",
      "index": "1",
      "interface": "SCSI",
      "kind": "compute#attachedDisk",
      "mode": "READ_WRITE",
      "source": "https://www.googleapis.com/compute/v1/projects/[PROJECT_ID]/zones/us-central1-a/disks/notebook-disk-type-test-jupyter-instance-data",
      "type": "PERSISTENT"
    }
  ],
  "shieldedInstanceConfig": {
    "enableVtpm": true,
    "enableIntegrityMonitoring": true
  },
  "creator": "sarah-french-google@[PROJECT_ID].iam.gserviceaccount.com"
}

This is the TF config I've used, and below that output from Terraform saying how values have been changed in state unexpectedly:

resource "google_notebooks_instance" "notebook" {
  name = "notebook-disk-type-test-jupyter-instance"
  location = var.zone
  machine_type = "e2-medium"

  container_image {
    repository = "gcr.io/deeplearning-platform-release/base-cpu"
    tag = "latest"
  }

  boot_disk_type = "PD_STANDARD"
  boot_disk_size_gb = 110

  data_disk_type = "PD_STANDARD"
  data_disk_size_gb = 110
  no_remove_data_disk = true

  metadata = {
    terraform = "true"
  }
}

2022-09-07T13:04:28.156+0100 [WARN]  Provider "provider[\"registry.terraform.io/hashicorp/google\"]" produced an unexpected new value for google_notebooks_instance.notebook, but we are tolerating it because it is using the legacy plugin SDK.
    The following problems may be the cause of any confusing errors from downstream operations:
      - .data_disk_type: was cty.StringVal("PD_STANDARD"), but now cty.StringVal("")
      - .custom_gpu_driver_path: was null, but now cty.StringVal("")
      - .kms_key: was null, but now cty.StringVal("")
      - .no_proxy_access: was null, but now cty.False
      - .disk_encryption: was null, but now cty.StringVal("")
      - .nic_type: was null, but now cty.StringVal("")
      - .install_gpu_driver: was null, but now cty.False
      - .no_public_ip: was null, but now cty.False
      - .post_startup_script: was null, but now cty.StringVal("")

ScottSuarez

Thanks for your work and diligence Sarah !! ^^

Stop reading data_disk_type from API response into state

2f3a465

modular-magician requested a review from ScottSuarez September 6, 2022 10:59

ScottSuarez approved these changes Sep 7, 2022

View reviewed changes

ScottSuarez merged commit 49a9c07 into GoogleCloudPlatform:main Sep 7, 2022

SarahFrench deleted the vertex-ai-notebook-disk-types branch March 26, 2024 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop reading data_disk_type value from API response into state of google_notebooks_instance resource #6512

Stop reading data_disk_type value from API response into state of google_notebooks_instance resource #6512

SarahFrench commented Sep 6, 2022 •

edited

Loading

modular-magician commented Sep 6, 2022

modular-magician commented Sep 6, 2022

modular-magician commented Sep 6, 2022

modular-magician commented Sep 6, 2022

ScottSuarez commented Sep 6, 2022

SarahFrench commented Sep 7, 2022

ScottSuarez left a comment

	bootDiskSizeGb: !ruby/object:Overrides::Terraform::PropertyOverride
	ignore_read: true
	bootDiskType: !ruby/object:Overrides::Terraform::PropertyOverride
	ignore_read: true

Stop reading data_disk_type value from API response into state of google_notebooks_instance resource #6512

Stop reading data_disk_type value from API response into state of google_notebooks_instance resource #6512

Conversation

SarahFrench commented Sep 6, 2022 • edited Loading

Description

List

modular-magician commented Sep 6, 2022

modular-magician commented Sep 6, 2022

Diff report:

modular-magician commented Sep 6, 2022

Tests analytics

Action taken

modular-magician commented Sep 6, 2022

ScottSuarez commented Sep 6, 2022

SarahFrench commented Sep 7, 2022

ScottSuarez left a comment

Choose a reason for hiding this comment

SarahFrench commented Sep 6, 2022 •

edited

Loading