Support Delayed Deletion for Vmwareengine Private Cloud #10764
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| func vmwareenginePrivateCloudStandardTypeDiffSuppressFunc(_, old, new string, d *schema.ResourceData) bool { | ||
| if (old == "STANDARD" && new == "") || (old == "" && new == "STANDARD") { | ||
| return true | ||
| } | ||
| if (isMultiNodePrivateCloud(d) && old == "TIME_LIMITED" && new == "STANDARD") { | ||
| log.Printf("[DEBUG] Multinode Private Cloud found, facilitating TYPE change to STANDARD") | ||
|
There was a problem hiding this comment. Can you give more context as to why a type diff from There was a problem hiding this comment. This change is separate from delayed deletion. The TIME_LIMITED -> STANDARD PC update behavior is explained in go/upgrade-pc-in-terraform A feature request was raised where the user wanted to change the PC type from their end for this use case. Since API does not support this change in its update call (it automatically changes it in the backend after a node count 1 -> 3&+ node count increase), I have added this suppress function to enable this type change at the terraform level. There was a problem hiding this comment. I also see that |
||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func isMultiNodePrivateCloud(d *schema.ResourceData) bool { | ||
| nodeConfigMap := d.Get("management_cluster.0.node_type_configs").(*schema.Set).List() | ||
| totalNodeCount := 0 | ||
| for _, nodeConfig := range nodeConfigMap { | ||
| configMap, ok := nodeConfig.(map[string]interface{}) | ||
| if !ok { | ||
| log.Printf("[DEBUG] Invalid node configuration format for private cloud.") | ||
| continue | ||
| } | ||
| nodeCount, ok := configMap["node_count"].(int) | ||
| if !ok { | ||
| log.Printf("[DEBUG] Invalid node_count format for private cloud.") | ||
| continue | ||
| } | ||
| totalNodeCount += nodeCount | ||
| } | ||
| log.Printf("[DEBUG] The node count of the private cloud is found to be %v nodes.", totalNodeCount) | ||
| if totalNodeCount > 2 { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // Delay deletion of the Private Cloud if delationDelayHours value is set | ||
| delationDelayHours := d.Get("deletion_delay_hours").(int) | ||
| if delationDelayHours > 0 || (delationDelayHours == 0 && d.Get("send_deletion_delay_hours_if_zero").(bool) == true) { | ||
| log.Printf("[DEBUG] Triggering delete of the Private Cloud with a delay of %v hours.\n", delationDelayHours) | ||
| url = url + "?delay_hours=" + fmt.Sprintf("%v", delationDelayHours) | ||
| } else { | ||
| log.Printf("[DEBUG] No deletion delay provided, triggering DELETE API without setting delay hours.\n") | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ func TestAccVmwareenginePrivateCloud_vmwareEnginePrivateCloudUpdate(t *testing.T | |
| t.Parallel() | ||
|
|
||
| context := map[string]interface{}{ | ||
| "region": "me-west1", | ||
|
There was a problem hiding this comment. can you provide more context as to why this is being updated? There was a problem hiding this comment. Some months back, we exhausted the number of nodes in the southamerca-west1 region which were being used for this test, leading to PC creation failures in multiple terraform tests. As a result, go/gcve-resource-optimisation-for-tf-tests was created and additional nodes were reserved in me-west1 region for terraform test projects. This PR is part of its implementation efforts along with PR #10992. |
||
| "random_suffix": acctest.RandString(t, 10), | ||
| "org_id": envvar.GetTestOrgFromEnv(t), | ||
| "billing_account": envvar.GetTestBillingAccountFromEnv(t), | ||
|
|
@@ -33,82 +33,66 @@ func TestAccVmwareenginePrivateCloud_vmwareEnginePrivateCloudUpdate(t *testing.T | |
| CheckDestroy: testAccCheckVmwareenginePrivateCloudDestroyProducer(t), | ||
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: testPrivateCloudCreateConfig(context), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| acctest.CheckDataSourceStateMatchesResourceStateWithIgnores( | ||
| "data.google_vmwareengine_private_cloud.ds", | ||
| "google_vmwareengine_private_cloud.vmw-engine-pc", | ||
| map[string]struct{}{ | ||
| "type": {}, | ||
| "deletion_delay_hours": {}, | ||
| "send_deletion_delay_hours_if_zero": {}, | ||
| }), | ||
| testAccCheckGoogleVmwareengineNsxCredentialsMeta("data.google_vmwareengine_nsx_credentials.nsx-ds"), | ||
| testAccCheckGoogleVmwareengineVcenterCredentialsMeta("data.google_vmwareengine_vcenter_credentials.vcenter-ds"), | ||
| ), | ||
| }, | ||
|
|
||
| { | ||
| ResourceName: "google_vmwareengine_private_cloud.vmw-engine-pc", | ||
| ImportState: true, | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIgnore: []string{"location", "name", "update_time", "type", "deletion_parameters"}, | ||
| }, | ||
| { | ||
| Config: testPrivateCloudUpdateConfig(context), | ||
| Check: resource.ComposeTestCheckFunc( | ||
| acctest.CheckDataSourceStateMatchesResourceStateWithIgnores( | ||
| "data.google_vmwareengine_private_cloud.ds", | ||
| "google_vmwareengine_private_cloud.vmw-engine-pc", | ||
| map[string]struct{}{ | ||
| "type": {}, | ||
| "deletion_delay_hours": {}, | ||
| "send_deletion_delay_hours_if_zero": {}, | ||
| }), | ||
| ), | ||
| }, | ||
|
|
||
| { | ||
| ResourceName: "google_vmwareengine_private_cloud.vmw-engine-pc", | ||
| ImportState: true, | ||
| ImportStateVerify: true, | ||
| ImportStateVerifyIgnore: []string{"location", "name", "update_time", "type", "deletion_parameters"}, | ||
| }, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| func testPrivateCloudCreateConfig(context map[string]interface{}) string { | ||
| return acctest.Nprintf(` | ||
| resource "google_vmwareengine_network" "default-nw" { | ||
| name = "tf-test-pc-nw-%{random_suffix}" | ||
| location = "global" | ||
| type = "STANDARD" | ||
| description = "PC network description." | ||
| } | ||
|
|
||
| resource "google_vmwareengine_private_cloud" "vmw-engine-pc" { | ||
| location = "%{region}-b" | ||
| name = "tf-test-sample-pc%{random_suffix}" | ||
| description = "test description" | ||
| type = "TIME_LIMITED" | ||
| deletion_delay_hours = 1 | ||
| network_config { | ||
| management_cidr = "192.168.30.0/24" | ||
| vmware_engine_network = google_vmwareengine_network.default-nw.id | ||
|
|
@@ -117,15 +101,14 @@ resource "google_vmwareengine_private_cloud" "vmw-engine-pc" { | |
| cluster_id = "tf-test-sample-mgmt-cluster-custom-core-count%{random_suffix}" | ||
| node_type_configs { | ||
| node_type_id = "standard-72" | ||
| node_count = 1 | ||
| custom_core_count = 32 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| data "google_vmwareengine_private_cloud" "ds" { | ||
| location = "%{region}-b" | ||
| name = "tf-test-sample-pc%{random_suffix}" | ||
| depends_on = [ | ||
| google_vmwareengine_private_cloud.vmw-engine-pc, | ||
|
|
@@ -144,6 +127,46 @@ data "google_vmwareengine_vcenter_credentials" "vcenter-ds" { | |
| `, context) | ||
| } | ||
|
|
||
| func testPrivateCloudUpdateConfig(context map[string]interface{}) string { | ||
| return acctest.Nprintf(` | ||
| resource "google_vmwareengine_network" "default-nw" { | ||
| name = "tf-test-pc-nw-%{random_suffix}" | ||
| location = "global" | ||
| type = "STANDARD" | ||
| description = "PC network description." | ||
| } | ||
|
|
||
| resource "google_vmwareengine_private_cloud" "vmw-engine-pc" { | ||
| location = "%{region}-b" | ||
| name = "tf-test-sample-pc%{random_suffix}" | ||
| description = "updated description" | ||
| type = "STANDARD" | ||
| deletion_delay_hours = 0 | ||
| send_deletion_delay_hours_if_zero = true | ||
| network_config { | ||
| management_cidr = "192.168.30.0/24" | ||
| vmware_engine_network = google_vmwareengine_network.default-nw.id | ||
| } | ||
| management_cluster { | ||
| cluster_id = "tf-test-sample-mgmt-cluster-custom-core-count%{random_suffix}" | ||
| node_type_configs { | ||
| node_type_id = "standard-72" | ||
| node_count = 3 | ||
| custom_core_count = 32 | ||
| } | ||
| } | ||
| } | ||
|
|
||
| data "google_vmwareengine_private_cloud" "ds" { | ||
| location = "%{region}-b" | ||
| name = "tf-test-sample-pc%{random_suffix}" | ||
| depends_on = [ | ||
| google_vmwareengine_private_cloud.vmw-engine-pc, | ||
| ] | ||
| } | ||
| `, context) | ||
| } | ||
|
|
||
| func testAccCheckGoogleVmwareengineNsxCredentialsMeta(n string) resource.TestCheckFunc { | ||
| return func(s *terraform.State) error { | ||
| rs, ok := s.RootModule().Resources[n] | ||
|
|
@@ -198,15 +221,21 @@ func testAccCheckVmwareenginePrivateCloudDestroyProducer(t *testing.T) func(s *t | |
| if config.BillingProject != "" { | ||
| billingProject = config.BillingProject | ||
| } | ||
| res, err := transport_tpg.SendRequest(transport_tpg.SendRequestOptions{ | ||
| Config: config, | ||
| Method: "GET", | ||
| Project: billingProject, | ||
| RawURL: url, | ||
| UserAgent: config.UserAgent, | ||
| }) | ||
| if err == nil { | ||
| pcState, ok := res["state"] | ||
| if !ok { | ||
| return fmt.Errorf("Unable to fetch state for existing VmwareenginePrivateCloud %s", url) | ||
| } | ||
| if pcState.(string) != "DELETED" { | ||
| return fmt.Errorf("VmwareenginePrivateCloud still exists at %s", url) | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import ( | |
| ) | ||
|
|
||
| func TestAccVmwareengineSubnet_vmwareEngineUserDefinedSubnetUpdate(t *testing.T) { | ||
| // Temporarily skipping so that this test does not run and consume resources during PR pushes. It is bound to fail and is being fixed by PR #10992 | ||
|
There was a problem hiding this comment. we will remove these before submitting right? There was a problem hiding this comment. We can keep these commented till PR #10992 gets submitted. That way, the consumption of nodes for tests which are bound to fail would be prevented. These tests would be modified and combined by PR #10992 which is built on the design go/gcve-resource-optimisation-for-tf-tests There was a problem hiding this comment. Understood, so the relevant test for these code changes are all in |
||
| acctest.SkipIfVcr(t) | ||
| t.Parallel() | ||
|
|
||
| context := map[string]interface{}{ | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the API default value for deletion delay? Is it 0? How often do you think users will need to additionally set
send_deletion_delay_hours_if_zero?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the default delay at the API level is set to 3 hours. However, it might be modified in the near future.
Users would need to set
send_deletion_delay_hours_if_zeroif they want to senddelay_hours=0through the API call and delete the PC immediately. I have added this boolean field after a discussion with Stephen in a previous thread since terraform does not differentiate between "no value set" and "0 value set".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thank you for the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that I do not like the need for
send_deletion_delay_hours_if_zerobut cannot think of another way to force send 0 only when the user has specified 0 due to TF limitations. I am sure this was discussed previouslyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the previous discussion thread for implementing this change.
Another approach which I could think of was using
delay_hoursinside a nested objectdeletion_parametersbut nested objects are not yet supported forvirtual_fields.