Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compute: added support for advanced_machine_features.turbo_mode #12148

Conversation

wyardley
Copy link
Contributor

@wyardley wyardley commented Oct 26, 2024

  • Added support for advanced_machine_features.turbo_mode in google_compute_instance, google_compute_instance_template, and google_compute_region_instance_template
  • Adjusted tests to use c4 instance types for advanced_machine_features tests
  • Minor docs and formatting fixes

Fixes hashicorp/terraform-provider-google#20023

I'm guessing this may be missing something

I looked at adding it in google_container_cluster as well, but seems like it's not implemented yet in regular or beta container API.

Side note: EnableUefiNetworking and PerformanceMonitoringUnit also seem to be available here but not implemented in the provider yet.

Should I add anything in tgc as well (like #10848)?

Release Note Template for Downstream PRs (will be copied)

compute: added support for `advanced_machine_features.turbo_mode` to `google_compute_instance`, `google_compute_instance_template`, and `google_compute_region_instance_template`

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Oct 26, 2024
@wyardley
Copy link
Contributor Author

Note: these tests pass for me locally, but only if I explicitly set subnetwork = default in network_interface; given the number of other tests that are using the default network (and no explicitly set subnetwork) vs. a bootstrapped created network, I assume it'll be Ok.

@wyardley wyardley marked this pull request as ready for review October 26, 2024 03:40
Copy link

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

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

@github-actions github-actions bot requested a review from roaks3 October 26, 2024 03:40
@wyardley wyardley force-pushed the wyardley/issues_20023/advanced_mode_turbo branch from 84387bc to 0c774f5 Compare October 26, 2024 03:51
- Added support for `advanced_machine_features.turbo_mode` to
  `google_compute_instance`, `google_compute_instance_template`, and
  `google_compute_region_instance_template`
- Adjusted tests to use c4 instance types for `advanced_machine_features`
  tests
- Minor docs and formatting fixes

Fixes hashicorp/terraform-provider-google#20023
@wyardley wyardley force-pushed the wyardley/issues_20023/advanced_mode_turbo branch from 0c774f5 to 340fd34 Compare October 26, 2024 04:22
Copy link
Contributor

@roaks3 roaks3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just need to verify tests

@modular-magician modular-magician added service/compute-instances and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Oct 29, 2024
@modular-magician
Copy link
Collaborator

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 ( 10 files changed, 64 insertions(+), 27 deletions(-))
google-beta provider: Diff ( 10 files changed, 64 insertions(+), 27 deletions(-))
terraform-google-conversion: Diff ( 1 file changed, 2 insertions(+))

Missing test report

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

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" {
  advanced_machine_features {
    enable_nested_virtualization = # value needed
    threads_per_core             = # value needed
    turbo_mode                   = # value needed
    visible_core_count           = # 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" {
  advanced_machine_features {
    enable_nested_virtualization = # value needed
    threads_per_core             = # value needed
    turbo_mode                   = # value needed
    visible_core_count           = # value needed
  }
}

@wyardley
Copy link
Contributor Author

btw, if the TDX / confidential image tests for compute fail, that may be a known / pre-existing issue (hashicorp/terraform-provider-google#19885). I have #12149 up as a possible fix for those.

@github-actions github-actions bot requested a review from roaks3 October 29, 2024 16:05
@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 1044
Passed tests: 966
Skipped tests: 73
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
  • TestAccComputeInstanceConfidentialInstanceConfigMain
  • TestAccComputeInstanceFromMachineImage_confidentialInstanceConfigMain
  • TestAccComputeInstanceTemplate_AdvancedMachineFeatures
  • TestAccComputeInstance_advancedMachineFeatures
  • TestAccComputeRegionInstanceTemplate_AdvancedMachineFeatures

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

🟢 Tests passed during RECORDING mode:
TestAccComputeInstanceTemplate_AdvancedMachineFeatures [Debug log]
TestAccComputeInstance_advancedMachineFeatures [Debug log]
TestAccComputeRegionInstanceTemplate_AdvancedMachineFeatures [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.


🔴 Tests failed during RECORDING mode:
TestAccComputeInstanceConfidentialInstanceConfigMain [Error message] [Debug log]
TestAccComputeInstanceFromMachineImage_confidentialInstanceConfigMain [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

@wyardley
Copy link
Contributor Author

wyardley commented Oct 29, 2024

@roaks3 that test PR is merged if you want to re-run the tests now.

w/r/t the missing test warning, I think the existing test coverage for templates probably should cover most use cases, but let me know if you want to see something else tested.

@roaks3
Copy link
Contributor

roaks3 commented Oct 29, 2024

Yea, I think the tests you have are sufficient, and the test failures look unrelated.

@roaks3 roaks3 merged commit 81d5235 into GoogleCloudPlatform:main Oct 29, 2024
13 of 14 checks passed
@wyardley wyardley deleted the wyardley/issues_20023/advanced_mode_turbo branch October 30, 2024 03:47
niharika-98 pushed a commit to niharika-98/magic-modules that referenced this pull request Nov 1, 2024
BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Nov 5, 2024
akshat-jindal-nit pushed a commit to akshat-jindal-nit/magic-modules that referenced this pull request Nov 18, 2024
amanMahendroo pushed a commit to amanMahendroo/magic-modules that referenced this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

advancedMachineFeatures.turboMode missing
3 participants