Added volume module for Google Cloud NetApp Volume #9513

G-NamanGupta · 2023-11-22T17:22:16Z

Fixes hashicorp/terraform-provider-google#16019

This PR is to support provisioning volume for Google Cloud NetApp Volumes through GCP terraform provider.

`google_netapp_volume`

This is extension to #9229

modular-magician · 2023-11-22T17:23:54Z

Hello! I am a robot. It looks like you are a: Community Contributor ~~Googler~~ ~~Core Contributor~~. Tests will require approval to run.

@rileykarson, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

rileykarson · 2023-11-22T17:53:37Z

mmv1/products/netapp/volume.yaml

+# limitations under the License.
+
+--- !ruby/object:Api::Resource
+name: 'volume'


Suggested change

name: 'volume'

name: 'Volume'

rileykarson · 2023-11-22T17:54:17Z

mmv1/products/netapp/volume.yaml

+  - !ruby/object:Api::Type::String
+    name: 'name'
+    description:
+      The  name of the volume. Needs to be unique per location.


Suggested change

The name of the volume. Needs to be unique per location.

The name of the volume. Needs to be unique per location.

rileykarson · 2023-11-22T17:54:52Z

mmv1/products/netapp/volume.yaml

+    url_param_only: true
+examples:
+  - !ruby/object:Provider::Terraform::Examples
+    name: 'Volume_create'


Suggested change

name: 'Volume_create'

name: 'volume_basic'

change the filename, we use _basic for the simple examples by convention!

rileykarson · 2023-11-22T17:55:32Z

mmv1/products/netapp/volume.yaml

+base_url: projects/{{project}}/locations/{{location}}/volumes
+self_link: projects/{{project}}/locations/{{location}}/volumes/{{name}}
+create_url: projects/{{project}}/locations/{{location}}/volumes?volumeId={{name}}
+update_url: projects/{{project}}/locations/{{location}}/volumes/{{name}}


We'll need to add an update test! https://googlecloudplatform.github.io/magic-modules/develop/test/#add-an-update-test

Each updatable field should have its value changed in at least one test!

rileykarson · 2023-11-22T17:55:41Z

mmv1/products/netapp/volume.yaml

+      network_name: 'test-network'
+      global_name: 'test-address'
+properties:
+  # Fields go here


Suggested change

# Fields go here

mmv1/products/netapp/volume.yaml

rileykarson · 2023-11-22T18:00:46Z

mmv1/products/netapp/volume.yaml

+        - !ruby/object:Api::Type::String
+          name: 'exportFull'
+          description: |
+            Full export ptah of the volume.


Suggested change

Full export ptah of the volume.

Full export path of the volume.

mmv1/products/netapp/volume.yaml

rileykarson · 2023-11-22T18:04:16Z

mmv1/products/netapp/volume.yaml

+    immutable: true
+    properties:
+      - !ruby/object:Api::Type::String
+        name: 'sourceSnapshot'


required: true? We can remove it if we add sourceBackup in the future.

modular-magician · 2023-11-22T18:16:44Z

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 ( 5 files changed, 2724 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 5 files changed, 2724 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 624 insertions(+))
TF OiCS: Diff ( 4 files changed, 148 insertions(+))

Missing test report

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

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

resource "google_netapp_volume" "primary" {
  export_policy {
    rules {
      access_type           = # value needed
      allowed_clients       = # value needed
      has_root_access       = # value needed
      kerberos5_read_only   = # value needed
      kerberos5_read_write  = # value needed
      kerberos5i_read_only  = # value needed
      kerberos5i_read_write = # value needed
      kerberos5p_read_only  = # value needed
      kerberos5p_read_write = # value needed
      nfsv3                 = # value needed
      nfsv4                 = # value needed
    }
  }
  restore_parameters {
    source_snapshot = # value needed
  }
  restricted_actions = # value needed
  snapshot_policy {
    daily_schedule {
      hour              = # value needed
      minute            = # value needed
      snapshots_to_keep = # value needed
    }
    enabled = # value needed
    hourly_schedule {
      minute            = # value needed
      snapshots_to_keep = # value needed
    }
    monthly_schedule {
      days_of_month     = # value needed
      hour              = # value needed
      minute            = # value needed
      snapshots_to_keep = # value needed
    }
    weekly_schedule {
      day               = # value needed
      hour              = # value needed
      minute            = # value needed
      snapshots_to_keep = # value needed
    }
  }
}

modular-magician · 2023-11-22T19:28:18Z

Tests analytics

Total tests: 3238
Passed tests 2906
Skipped tests: 330
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

TestAccDataprocClusterIamPolicy|TestAccNetappvolume_volumeCreateExample

Get to know how VCR tests work

modular-magician · 2023-11-22T19:41:06Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[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:}}$
TestAccNetappvolume_volumeCreateExample[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

G-NamanGupta · 2023-11-23T06:05:43Z

Hi @rileykarson, Thanks for all the suggestions. I'll update the code and will push it in some time.
Meantime, the error we are seeing here seems to be related to hashicorp/terraform-provider-google#16275. just to confirm can you please post the error logs?

okrause · 2023-11-28T13:06:02Z

Hi @rileykarson, Thanks for all the suggestions. I'll update the code and will push it in some time. Meantime, the error we are seeing here seems to be related to hashicorp/terraform-provider-google#16275. just to confirm can you please post the error logs?

Are we blocked by that? Can we use the workaround which was meanwhile mentioned in hashicorp/terraform-provider-google#16275? We need to move on!

imrannayer · 2023-12-05T13:04:22Z

@G-NamanGupta any update on this?

G-NamanGupta · 2023-12-05T15:49:42Z

@G-NamanGupta any update on this?

@imrannayer , we are working with google dev team to reach to a viable solution for the issue we are facing with VPC network.
Will update here once the approach is finalized.

rileykarson · 2023-12-06T18:48:58Z

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

+  network       = google_compute_network.peering_network.id
+}
+
+# Create a private connection


We're gonna want to use a bootstrapping function here- @shuyama1 was looking into some pre-work required to use one for this case. Leaving this open until that's resolved.

#9598 is merged, which refactored the bootstrapping function BootstrapSharedServiceNetworkingConnection to allow using different services. Hopefully it unblocks the test here.

BootstrapSharedServiceNetworkingConnection(t, "YOUR_NETWORK_NAME", acctest.ServiceNetworkWithParentService("netapp.servicenetworking.goog")) is likely what you'll need in this case.

Added the changes related to network, added update test and also did cosmetic changes asked by Riley.

rileykarson · 2023-12-11T21:42:32Z

mmv1/products/netapp/volume.yaml

+      Name of the Private Service Access allocated range. Inherited from storage pool.
+    output: true
+  - !ruby/object:Api::Type::String
+    name: 'storagePool'


this seems like it would be immutable? Can we test changing it if not?

Volumes can be assigned to a different storagePool in certain situations.
The condition set is complex and cannot be handled by the provider. Let the API return good or bad.
https://cloud.google.com/netapp/volumes/docs/configure-and-use/volumes/edit-volume
Summary: storagePool should be mutable. Test case: create two pools in same region with same parameters (VPC, AD, CMEK, LDAP), but differing service levels (premium and extreme). Reassign a volume from one to the other.

rileykarson · 2023-12-11T21:45:02Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+  security_style = "UNIX"
+  kerberos_enabled = false
+  export_policy {
+    rules {


Can we add a second rule here?

rileykarson · 2023-12-11T21:45:57Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+		access_type           = "READ_ONLY"
+		allowed_clients       = "0.0.0.0/0"
+		has_root_access       = "true"
+		kerberos5_read_only   = false


Can we change the value of these booleans between the update steps?

has_root_access can be changed easily. All the kerberos parameters can only be changed it the volumes would be exported with Kerberos and that requires complex infrastructure to be in place in the test environment. Not realistic.

rileykarson · 2023-12-11T21:46:20Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+  capacity_gib = "200"
+  share_name = "testshare"
+  storage_pool = google_netapp_storage_pool.default.name
+  protocols = ["NFSV3"]


Can we add a protocol in this update?

Protocols setting is immutable. We should mark it immutable.

rileykarson · 2023-12-11T21:46:52Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+  share_name = "testshare"
+  storage_pool = google_netapp_storage_pool.default.name
+  protocols = ["NFSV3"]
+  smb_settings = []


Can we set some SMB settings in the initial step?

I recommend:
SHOW_SNAPSHOT, SHOW_PREVIOUS_VERSIONS
I intentionally left away a few settings which I expect the API to set automatically during volume creation, like BROWSABLE, CHANGE_NOTIFY, OPLOCKS (and maybe more or less).
TF should expect that create returns some settings which the user didn't put in.

I thought about it again: smb_settings only apply to SMB volumes and SMb volumes cannot be created without an AD server in the test environment and setting up an AD server is pretty heavy weight and out of scope for the tests.
But please make sure a TF is prepared to receive back some default values from the create operation.

I tested it. If smb_settings isn't specified at create, API doesn't return smb_settings on subsequent reads. That means it doesn't return any unexpected default values.

rileykarson · 2023-12-11T21:47:04Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+  storage_pool = google_netapp_storage_pool.default.name
+  protocols = ["NFSV3"]
+  smb_settings = []
+  unix_permissions = "0770"


Suggested change

unix_permissions = "0770"

unix_permissions = "0740"

rileykarson · 2023-12-11T21:47:56Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+    value= "pool"
+  }
+  description = "This is a test description"
+  snapshot_directory = false


Can we change this value during the update?

rileykarson · 2023-12-11T21:49:09Z

mmv1/third_party/terraform/services/netapp/resource_netapp_volume_test.go

+      minute            = 1
+      snapshots_to_keep = 1
+    }
+    enabled = true


Could we add a third update step where we disable the schedule?

rileykarson · 2023-12-11T21:49:35Z

mmv1/products/netapp/volume.yaml

+            description: |-
+              Set the day or days of the month to make a snapshot (1-31). Accepts a comma separated number of days. Defaults to '1'.
+  - !ruby/object:Api::Type::NestedObject
+    name: restoreParameters


Can this appear in a test?

No. This is a parameter for Volume Backup. The resources for backup (backupPolicies, backupVaults and backupVaults.backups) are new their creation is not scheduled yet.

Should we consider excluding this from implementation for now? We can implement it after the resources for backup are added to the provider since we can't test the creation of volume from backup and can not test if the feature is working or not for now.

modular-magician · 2023-12-14T17:48: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 ( 6 files changed, 3028 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 3028 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 624 insertions(+))
TF OiCS: Diff ( 4 files changed, 130 insertions(+))

Missing test report

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

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

resource "google_netapp_volume" "primary" {
  restore_parameters {
    source_snapshot = # value needed
  }
}

modular-magician · 2023-12-14T18:48:46Z

Tests analytics

Total tests: 3295
Passed tests 2956
Skipped tests: 337
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

TestAccNetappvolume_volumeCreateExample_update|TestAccNetappVolume_volumeBasicExample

Get to know how VCR tests work

modular-magician · 2024-01-11T17:13:47Z

Tests analytics

Total tests: 10
Passed tests 8
Skipped tests: 0
Affected tests: 2

Click here to see the affected service packages

netapp

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

TestAccNetappVolume_volumeBasicExample|TestAccNetappVolume_volumeBasicExample_update

Get to know how VCR tests work

modular-magician · 2024-01-11T17:14:04Z

$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccNetappVolume_volumeBasicExample[Error message] [Debug log]
TestAccNetappVolume_volumeBasicExample_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

shuyama1 · 2024-01-11T18:10:29Z

@G-NamanGupta Tests failed with error Error creating storagePool: googleapi: Error 400: StoragePool creation is not allowed in this region. Discussed with the service team offline, it seems only us-east4 or us-west2 could be used for StoragePool creation besides us-central1. Let's try one of those and see if it works. Thanks

okrause · 2024-01-12T10:27:00Z

/gcbrun

okrause · 2024-01-12T10:53:45Z

Okay, I didn't expect my /gcbrun to work, but I had to test it anyway :D

modular-magician · 2024-01-12T17:32:46Z

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 ( 6 files changed, 2943 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 6 files changed, 2943 insertions(+), 2 deletions(-))
TF Conversion: Diff ( 1 file changed, 595 insertions(+))
TF OiCS: Diff ( 4 files changed, 121 insertions(+))

modular-magician · 2024-01-12T17:36:11Z

Tests analytics

Total tests: 10
Passed tests 8
Skipped tests: 0
Affected tests: 2

Click here to see the affected service packages

netapp

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

TestAccNetappVolume_volumeBasicExample|TestAccNetappVolume_volumeBasicExample_update

Get to know how VCR tests work

modular-magician · 2024-01-12T17:46:30Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccNetappVolume_volumeBasicExample[Debug log]
TestAccNetappVolume_volumeBasicExample_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

okrause · 2024-01-12T18:24:34Z

@shuyama1 Can you please have a look if the resource is fine now? Sorry for being pushy.

shuyama1

Thank you!

okrause · 2024-01-12T19:33:55Z

@rileykarson @shuyama1 Many, many thanks for your help on getting this reviewed and merged. Very appreciated!

imrannayer · 2024-01-17T18:53:57Z

@okrause is there any plan to add BackupConfig block to google_netapp_volume?
Thanks

…rm#9513) * added volume module * Added updated function and network fix * updated update test fields. * updated unixPermissions field. * updated test file for snapshot_policy * updated description. updated test file with basic example and function name. * removed the *_UNSPECIFIED values for inputs Default values for minutes, hours, days and day_of_week in snapshot block Updated tests to incorporate suggestions by shuya Fixes some indentation in tests Updates some test to get better test coverage commented restoreParamters as suggested by Shuya. * updated tests to use us-west1 region * updated region to us-west2

okrause · 2024-01-18T11:33:01Z

@okrause is there any plan to add BackupConfig block to google_netapp_volume? Thanks

We plan to add more features over time. backupConfig was blocked so far, since we didn't had resources for Backup Vault and Backup Policy. They are now merged. Adding backupConfig should be easy to do, but we may experience some problems to solve in the required test code due to associating/disassociating a volume from a vault. Need to take a closer look.

…rm#9513) * added volume module * Added updated function and network fix * updated update test fields. * updated unixPermissions field. * updated test file for snapshot_policy * updated description. updated test file with basic example and function name. * removed the *_UNSPECIFIED values for inputs Default values for minutes, hours, days and day_of_week in snapshot block Updated tests to incorporate suggestions by shuya Fixes some indentation in tests Updates some test to get better test coverage commented restoreParamters as suggested by Shuya. * updated tests to use us-west1 region * updated region to us-west2

added volume module

0e9fd78

modular-magician requested a review from rileykarson November 22, 2023 17:23

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Nov 22, 2023

rileykarson reviewed Nov 22, 2023

View reviewed changes

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Nov 22, 2023

imrannayer mentioned this pull request Dec 4, 2023

Add google_netapp_storage_volume resource to the provider hashicorp/terraform-provider-google#16019

Closed

rileykarson reviewed Dec 6, 2023

View reviewed changes

shuyama1 mentioned this pull request Dec 7, 2023

add optional input ParentService to BootstrapSharedServiceNetworkingC… #9598

Merged

Added updated function and network fix

8adc649

modular-magician requested review from rileykarson and shuyama1 December 11, 2023 12:01

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 11, 2023

rileykarson reviewed Dec 11, 2023

View reviewed changes

updated update test fields.

c807182

modular-magician requested a review from rileykarson December 12, 2023 18:21

updated unixPermissions field.

fa1656d

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Dec 14, 2023

updated region to us-west2

c4bae53

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jan 12, 2024

G-NamanGupta mentioned this pull request Jan 12, 2024

Adding Support for Snapshot module for Google Cloud Netapp Volume #9812

Merged

modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Jan 12, 2024

shuyama1 approved these changes Jan 12, 2024

View reviewed changes

shuyama1 merged commit bd6b1b3 into GoogleCloudPlatform:main Jan 12, 2024
14 checks passed

modular-magician mentioned this pull request Jan 12, 2024

Added volume module for Google Cloud NetApp Volume GoogleCloudPlatform/terraform-google-conversion#1862

Merged

	The name of the volume. Needs to be unique per location.
	The name of the volume. Needs to be unique per location.

	Full export ptah of the volume.
	Full export path of the volume.

Added volume module for Google Cloud NetApp Volume #9513

Added volume module for Google Cloud NetApp Volume #9513

Conversation

G-NamanGupta commented Nov 22, 2023 • edited by shuyama1

modular-magician commented Nov 22, 2023

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 Nov 22, 2023

Diff report

Missing test report

modular-magician commented Nov 22, 2023

Tests analytics

Action taken

modular-magician commented Nov 22, 2023

Rerun these tests in REPLAYING mode to catch issues

G-NamanGupta commented Nov 23, 2023

okrause commented Nov 28, 2023

imrannayer commented Dec 5, 2023

G-NamanGupta commented Dec 5, 2023

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 Dec 14, 2023

Diff report

Missing test report

modular-magician commented Dec 14, 2023

Tests analytics

Action taken

modular-magician commented Jan 11, 2024

Tests analytics

Action taken

modular-magician commented Jan 11, 2024

shuyama1 commented Jan 11, 2024

okrause commented Jan 12, 2024

okrause commented Jan 12, 2024

modular-magician commented Jan 12, 2024

Diff report

modular-magician commented Jan 12, 2024

Tests analytics

Action taken

modular-magician commented Jan 12, 2024

Rerun these tests in REPLAYING mode to catch issues

okrause commented Jan 12, 2024

shuyama1 left a comment

Choose a reason for hiding this comment

okrause commented Jan 12, 2024

imrannayer commented Jan 17, 2024

okrause commented Jan 18, 2024

G-NamanGupta commented Nov 22, 2023 •

edited by shuyama1