compute: Add scheduling.termination_time field to compute_instance resources

[Asiderr]

This patch adds the scheduling.termination_time field to the following resources:

• google_compute_instance
• google_compute_instance_from_machine_image (beta)
• google_compute_instance_from_template
• google_compute_instance_template
• google_compute_region_instance_template

It also adds a helper function hasTerminationTimeChanged, which allows to stop the instance while updating the termination_time.

compute: added `scheduling.termination_time` field to `google_compute_instance` resource

compute: added `scheduling.termination_time` field to `google_compute_instance_from_machine_image` resource

compute: added `scheduling.termination_time` field to `google_compute_instance_from_template` resource

compute: added `scheduling.termination_time` field to `google_compute_instance_template` resource

compute: added `scheduling.termination_time` field to `google_compute_region_instance_template` resource

[github-actions]

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@melinath, 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.

[melinath]

In addition to the comment below, could you add unit tests for schedulingHasChangeRequiringReboot?

[modular-magician]

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.

google provider: Diff ( 4 files changed, 92 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 4 files changed, 92 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 1 file changed, 26 insertions(+), 1 deletion(-))

Missing test report

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

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

resource "google_compute_instance" "primary" {
  scheduling {
    maintenance_interval = # value needed
  }
}

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

resource "google_compute_instance_from_machine_image" "primary" {
  scheduling {
    availability_domain  = # value needed
    maintenance_interval = # value needed
    min_node_cpus        = # value needed
    preemptible          = # value needed
    termination_time     = # value needed
  }
}

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

resource "google_compute_instance_from_template" "primary" {
  scheduling {
    availability_domain  = # value needed
    maintenance_interval = # value needed
    min_node_cpus        = # value needed
    on_host_maintenance  = # value needed
    preemptible          = # value needed
    termination_time     = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 23
Passed tests: 5
Skipped tests: 18
Affected tests: 0

Click here to see the affected service packages

• compute

#### Non-exercised tests

🔴 Tests were added that are skipped in VCR:

• TestAccComputeInstance_schedulingTerminationTime
  🔴 Errors occurred during REPLAYING mode. Please fix them to complete your PR.

View the build log

[melinath]

looks like this also needs to be added to google_compute_instance_from_machine_image and google_compute_instance_from_template

[Asiderr]

@melinath

In addition to the comment below, could you add unit tests for schedulingHasChangeRequiringReboot?

The schedulingHasChangeRequiringReboot requires d *schema.ResourceData as an input parameter so it would be easier to create unit tests for hasNodeAffinitiesChanged, hasMaxRunDurationChanged and hasTerminationTimeChanged with corresponding test cases. There is no test file for this helper, so should I create a new test file or do we want to move schedulingHasChangeRequiringReboot and related functions to the resource_compute_instance?

looks like this also needs to be added to google_compute_instance_from_machine_image and google_compute_instance_from_template

It will be added in the future in a separate PR.

[melinath]

regarding unit tests for schedulingHasChangeRequiringReboot: the way we generally get around needing Terraform core structs is by making the function a thin wrapper that gets the necessary data (in this case, the scheduling data) and passes it to the function that's actually tested. A new compute_instance_helpers test file seems reasonable.

Regarding adding the field to the other two resources: their schemas are based on the compute instance schema so the field is already half added, which is why it's showing up in the missing test report for those resources. However, this change is causing the tests to fail in a panic:

panic: Invalid address to set: []string{"scheduling", "0", "termination_time"}

goroutine 18768 [running]:
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc002316800, {0x50ca439, 0xa}, {0x464afe0, 0xc0021d8d98})
	/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource_data.go:233 +0x2ae
github.com/hashicorp/terraform-provider-google-beta/google-beta/services/compute.resourceComputeInstanceTemplateRead(0xc002316800, {0x508b540?, 0xc000397008})
	/go/src/github.com/modular-magician/terraform-provider-google-beta/google-beta/services/compute/resource_compute_instance_template.go:1897 +0x1516

The panic is coming from this line: https://github.com/modular-magician/terraform-provider-google-beta/blob/5c41943bbe46c01273190359dab953c9ed63caed/google-beta/services/compute/resource_compute_instance_template.go#L1897

Taking another look at it, I think the problem is that the API doesn't actually support the field yet, so it's not present in the response.

Anyway - the main point is that if you don't want to add the fields to the from_template and from_machine_image resources, you'll need to modify the code to explicitly not add them, and you'll need to make sure that the shared helper doesn't run into issues if they're not present on a resource.

[github-actions]

@Asiderr, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days.

Please address any comments or change requests, or re-request review from a core reviewer if no action is required.

This notification can be disabled with the disable-automatic-closure label.

[Asiderr]

@melinath I added all missing fields which were connected to the termination_time. So new fields and tests are added to following resources:

• google_compute_instance
• google_compute_instance_from_machine_image (beta)
• google_compute_instance_from_template
• google_compute_instance_template
• google_compute_region_instance_template

[Asiderr]

@melinath Also I wonder if there is any easy possibility to unit test private functions in helpers? schedulingHasChangeRequiringReboot as well as hasTerminationTimeChanged, hasNodeAffinitiesChanged and hasMaxRunDurationChanged are privates due to naming, so I see no easy way to test them.

[github-actions]

@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[melinath]

@melinath Also I wonder if there is any easy possibility to unit test private functions in helpers? schedulingHasChangeRequiringReboot as well as hasTerminationTimeChanged, hasNodeAffinitiesChanged and hasMaxRunDurationChanged are privates due to naming, so I see no easy way to test them.

yes, you can add helper tests in the services/compute/ folder, which will have access to the private helpers in that folder.

[melinath]

regarding unit tests for schedulingHasChangeRequiringReboot: the way we generally get around needing Terraform core structs is by making the function a thin wrapper that gets the necessary data (in this case, the scheduling data) and passes it to the function that's actually tested. A new compute_instance_helpers test file seems reasonable.

It looks like I forgot to circle back to this and say: the wrapper we use is ResourceDataMock: https://github.com/hashicorp/terraform-provider-google-beta/blob/50e64ad11c68fbfe95842bb6e52535e235320a68/google-beta/tpgresource/resource_test_utils.go#L18

Examples of usage: https://github.com/search?q=repo%3Ahashicorp%2Fterraform-provider-google-beta+ResourceDataMock+path%3A%2F%5Egoogle-beta%5C%2Fservices%5C%2F%2F&type=code

[melinath]

apologies for the delayed review - taking a quick glance, this seems reasonable and I can see that you have added the requested tests for *_from_template and *_from_machine_image, as well as adding the field to instance_template and region_instance_template. If you could add some basic unit tests as previously discussed, that would be great; there's also a merge conflict that needs to be resolved. Other than that I think this should be good to go as long as the tests pass! Thanks!

[Asiderr]

apologies for the delayed review - taking a quick glance, this seems reasonable and I can see that you have added the requested tests for *_from_template and *_from_machine_image, as well as adding the field to instance_template and region_instance_template. If you could add some basic unit tests as previously discussed, that would be great; there's also a merge conflict that needs to be resolved. Other than that I think this should be good to go as long as the tests pass! Thanks!

@melinath I resolved conflicts. I added new test file for helper. The ResourceDataMock does not implement GetChange method that is used in schedulingHasChangeRequiringReboot so instead of testing schedulingHasChangeRequiringReboot I tested function hasTerminationTimeChanged that I'm adding to this helper file.

[melinath]

Setting terminationTime like this means that the tests become non-deterministic (because running it twice will give two different termination times.) Here & everywhere else - this should be changed to a relatively static time in the future (such as the beginning of the next year).

Suggested change

	terminationTime := now.Add(24 * time.Hour).Format(time.RFC3339)
	terminationTime := time.Date(time.Now().Year(), 12, 31, 0, 0, 0, 0, time.Now().Location()).AddDate(0, 0, 1).Format(time.RFC3339)

[Asiderr]

It should be implemented as: time.Date(now.Year(), now.Month(), now.Day(), 23, 59, 59, 9999, now.Location()).
The termination_time can be set as a time ranging from 30 seconds to 17 weeks. So let's set it as the end of the day on which the test takes place. I added proper change.

[melinath]

This is causing a bunch of unrelated tests to fail with the error:

        Error: Error creating instance: googleapi: Error 400: Invalid value for field 'resource.scheduling.terminationTime': ''. Termination time for given provisioning model is not supported without an instance termination action., invalid

This is happening because, on Create, the provider is sending "terminationTime": "" in the API request. I suspect this may be caused by something like: d.Get at create time returning "" for string fields (which means it's already "present" in original). But I could be wrong.

However, checking whether v == "" and only running this code if not would effectively mean that the ForceSendFields is never triggered in the cases where it would actually have an effect.

Do you know whether ForceSendFields is necessary here? If you're not sure, the way to check is to remove it and to add update tests that remove this field from a configuration (without removing the scheduling block overall). If ForceSendFields is necessary, those new tests will fail, and we can try to figure out the best workaround.

[Asiderr]

Yes, this is necessary. I created such test and it failed (termination_time is not removed after updating the configuration to remove this field).
Also, I was able to reproduce this problem after adding ForceSendFields. Then the following problem occurred after updating the configuration to remove this field. So we need to figure out some workaround here.
I updated TestAccComputeInstance_schedulingTerminationTime to cover this case.

[modular-magician]

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.

google provider: Diff ( 12 files changed, 455 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 13 files changed, 543 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 2 files changed, 48 insertions(+), 2 deletions(-))

Missing test report

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

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

resource "google_compute_instance" "primary" {
  scheduling {
    maintenance_interval = # value needed
  }
}

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

resource "google_compute_instance_from_machine_image" "primary" {
  scheduling {
    availability_domain  = # value needed
    maintenance_interval = # value needed
    min_node_cpus        = # value needed
    preemptible          = # value needed
  }
}

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

resource "google_compute_instance_from_template" "primary" {
  scheduling {
    availability_domain  = # value needed
    maintenance_interval = # value needed
    min_node_cpus        = # value needed
    on_host_maintenance  = # value needed
    preemptible          = # value needed
    termination_time     = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 1129
Passed tests: 1045
Skipped tests: 79
Affected tests: 5

Click here to see the affected service packages

• compute

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

• TestAccComputeInstanceFromMachineImage_terminationTime
• TestAccComputeInstanceFromTemplate_TerminationTime
• TestAccComputeInstanceTemplate_instanceTerminationAction_terminationTime
• TestAccComputeInstance_schedulingTerminationTime
• TestAccComputeRegionInstanceTemplate_instanceTerminationAction_terminationTime

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeInstanceFromMachineImage_terminationTime [Debug log]
TestAccComputeInstanceFromTemplate_TerminationTime [Debug log]
TestAccComputeInstanceTemplate_instanceTerminationAction_terminationTime [Debug log]
TestAccComputeRegionInstanceTemplate_instanceTerminationAction_terminationTime [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🔴 Tests failed during RECORDING mode:
TestAccComputeInstance_schedulingTerminationTime [Error message] [Debug log]

🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR.

View the build log or the debug log for each test

[melinath]

Continuing the discussion from #12791 (comment) - it sounds like ForceSendFields doesn't fix the problem of this not getting properly unset? It would definitely match the behavior of other string fields in this expand function, so they might also not behave as expected if this doesn't. (But that's outside the scope of this PR.)

I believe the fix would be to make sure that TerminationTime gets set to "" (and added to ForceSendFields) if it's not present in original.

[Asiderr]

I added workaround for this issue. Now everything is working as excepted.

[melinath]

What was the workaround?

[Asiderr]

I added force new to this field. Currently it's not possible to delete this field without destroying the old VM due to the API bug. Once there is no force new and termination_time is added to the forcesend, during deletion API is always trying to parse an empty string as a timestamp. One more improvement that could be added is creation of the customdiff that would set force new only when this field is deleted.
BTW other scheduling fields are also affected by this forcesend and could not be deleted once other scheduling fields still remain (I tested this for instance_termination_action).

[Asiderr]

This resource (google_compute_instance_from_template.inst) still needs to override scheduling.termination_time - this is why the automation is detecting google_compute_instance_from_template.scheduling.termination_time as untested: #12791 (comment)

I added overwrite.

[modular-magician]

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.

google provider: Diff ( 12 files changed, 464 insertions(+), 1 deletion(-))
google-beta provider: Diff ( 13 files changed, 552 insertions(+), 1 deletion(-))
terraform-google-conversion: Diff ( 2 files changed, 48 insertions(+), 2 deletions(-))

Missing test report

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

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

resource "google_compute_instance" "primary" {
  scheduling {
    maintenance_interval = # value needed
  }
}

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

resource "google_compute_instance_from_machine_image" "primary" {
  scheduling {
    availability_domain  = # value needed
    maintenance_interval = # value needed
    min_node_cpus        = # value needed
    preemptible          = # value needed
  }
}

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

resource "google_compute_instance_from_template" "primary" {
  scheduling {
    availability_domain  = # value needed
    maintenance_interval = # value needed
    min_node_cpus        = # value needed
    on_host_maintenance  = # value needed
    preemptible          = # value needed
  }
}

[modular-magician]

Tests analytics

Total tests: 1129
Passed tests: 1045
Skipped tests: 79
Affected tests: 5

Click here to see the affected service packages

• compute

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

• TestAccComputeInstanceFromMachineImage_terminationTime
• TestAccComputeInstanceFromTemplate_TerminationTime
• TestAccComputeInstanceTemplate_instanceTerminationAction_terminationTime
• TestAccComputeInstance_schedulingTerminationTime
• TestAccComputeRegionInstanceTemplate_instanceTerminationAction_terminationTime

Get to know how VCR tests work

[modular-magician]

🟢 Tests passed during RECORDING mode:
TestAccComputeInstanceFromMachineImage_terminationTime [Debug log]
TestAccComputeInstanceFromTemplate_TerminationTime [Debug log]
TestAccComputeInstanceTemplate_instanceTerminationAction_terminationTime [Debug log]
TestAccComputeInstance_schedulingTerminationTime [Debug log]
TestAccComputeRegionInstanceTemplate_instanceTerminationAction_terminationTime [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

---

🟢 All tests passed!

View the build log or the debug log for each test

[melinath]

Confirmed via experimentation - API returns errors for empty values for this field on update. ForceNew is an acceptable workaround.

Thanks for sticking with this!