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

feat: support enable_nested_virtualization #2012

Conversation

DrFaust92
Copy link
Contributor

No description provided.

@DrFaust92 DrFaust92 requested review from ericyz, gtsorbo and a team as code owners July 26, 2024 03:31
@apeabody
Copy link
Collaborator

/gcbrun

1 similar comment
@apeabody
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @DrFaust92!

This change looks OK to me, could we add to a appropriate example/test?

@DrFaust92 DrFaust92 force-pushed the enable_nested_virtualization branch 2 times, most recently from 8aacbc6 to 1691c6e Compare July 31, 2024 03:26
@DrFaust92
Copy link
Contributor Author

apeabody tried to add a test, hope this is well

@DrFaust92 DrFaust92 requested a review from apeabody July 31, 2024 03:39
@apeabody
Copy link
Collaborator

/gcbrun

@apeabody apeabody self-assigned this Jul 31, 2024
@DrFaust92 DrFaust92 requested a review from apeabody July 31, 2024 23:28
@apeabody
Copy link
Collaborator

/gcbrun

1 similar comment
@apeabody
Copy link
Collaborator

apeabody commented Aug 1, 2024

/gcbrun

@DrFaust92
Copy link
Contributor Author

Hi apeabody, can you share the error?

@apeabody
Copy link
Collaborator

apeabody commented Aug 2, 2024

Hi apeabody, can you share the error?

Hmm, unfortunately it didn't accept the null for threads_per_core. Perhaps the original "0" or "" are considered "unset"? I'll need to investigate further and perhaps get the documentation updated.

"The number of threads per physical core. To disable simultaneous multithreading (SMT) set this to 1. If unset, the maximum number of threads supported per core by the underlying processor is assumed."

Step #67 - "converge node-pool-local": STDERR: Error: Missing required argument
Step #67 - "converge node-pool-local": 
Step #67 - "converge node-pool-local":   with module.example.module.gke.google_container_node_pool.pools["pool-05"],
Step #67 - "converge node-pool-local":   on ../../../modules/beta-public-cluster/cluster.tf line 637, in resource "google_container_node_pool" "pools":
Step #67 - "converge node-pool-local":  637:   node_config {
Step #67 - "converge node-pool-local": 
Step #67 - "converge node-pool-local": The argument "node_config.0.advanced_machine_features.0.threads_per_core" is
Step #67 - "converge node-pool-local": required, but no definition was found.

@DrFaust92
Copy link
Contributor Author

apeabody probably zero us unset as its an int and in golang many times its as good as unset

@apeabody
Copy link
Collaborator

apeabody commented Aug 2, 2024

apeabody probably zero us unset as its an int and in golang many times its as good as unset

Agreed, "0" is my educated guess as well. We can go forward with that value for now if you like.

@DrFaust92 DrFaust92 force-pushed the enable_nested_virtualization branch from 4206aa9 to a19e995 Compare August 2, 2024 23:48
@apeabody
Copy link
Collaborator

apeabody commented Aug 5, 2024

/gcbrun

@apeabody
Copy link
Collaborator

apeabody commented Aug 5, 2024

/gcbrun

@apeabody apeabody merged commit e298e74 into terraform-google-modules:master Aug 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants