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 reservation affinity #2010

Merged

Conversation

DrFaust92
Copy link
Contributor

@DrFaust92 DrFaust92 commented Jul 26, 2024

Closes #1990

@DrFaust92 DrFaust92 requested review from ericyz, gtsorbo and a team as code owners July 26, 2024 02:52
@DrFaust92 DrFaust92 changed the title support reservation affinity feat: support reservation affinity Jul 26, 2024
@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!

STDERR: Error: Incorrect attribute value type

  on ../../../cluster.tf line 603, in resource "google_container_node_pool" "pools":
 603:         values                   = reservation_affinity.value.values

Inappropriate value for attribute "values": set of string required.

@DrFaust92 DrFaust92 requested a review from apeabody July 27, 2024 14:48
@DrFaust92
Copy link
Contributor Author

apeabody I think i fixed it given the limitation

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody
Copy link
Collaborator

STDERR: Error: Invalid function argument

  on ../../../cluster.tf line 603, in resource "google_container_node_pool" "pools":
 603:         values                   = split(",", reservation_affinity.value.values)
    ├────────────────
    │ reservation_affinity.value.values is null

Invalid value for "str" parameter: argument must not be null.

@apeabody apeabody self-assigned this Jul 31, 2024
@DrFaust92 DrFaust92 force-pushed the reservation-affinity branch from a69aced to 7c5f1d8 Compare August 3, 2024 00:32
@apeabody
Copy link
Collaborator

apeabody commented Aug 5, 2024

/gcbrun

@DrFaust92 DrFaust92 force-pushed the reservation-affinity branch from 7c5f1d8 to 8d5e4d7 Compare August 7, 2024 19:31
@apeabody
Copy link
Collaborator

apeabody commented Aug 8, 2024

/gcbrun

@DrFaust92
Copy link
Contributor Author

apeabody can you share error?

@apeabody
Copy link
Collaborator

apeabody commented Aug 9, 2024

apeabody can you share error?

Of course:

        	Error Trace:	/builder/home/go/pkg/mod/github.com/gruntwork-io/[email protected]/modules/terraform/apply.go:34
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:571
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:630
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:669
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/utils/stages.go:31
        	            				/builder/home/go/pkg/mod/github.com/!google!cloud!platform/cloud-foundation-toolkit/infra/[email protected]/pkg/tft/terraform.go:669
        	Error:      	Received unexpected error:
        	            	FatalError{Underlying: error while running command: exit status 1; 
        	            	Error: Missing required argument
        	            	
        	            	  with module.example.module.gke.google_container_node_pool.pools["asm-node-pool"],
        	            	  on ../../../cluster.tf line 573, in resource "google_container_node_pool" "pools":
        	            	 573:   node_config {
        	            	
        	            	The argument "node_config.0.reservation_affinity.0.consume_reservation_type"
        	            	is required, but no definition was found.}
        	Test:       	TestSimpleZonalWithASM

@DrFaust92 DrFaust92 force-pushed the reservation-affinity branch from 15b76fa to 5802685 Compare August 15, 2024 23:51
@DrFaust92 DrFaust92 marked this pull request as draft August 15, 2024 23:51
@DrFaust92 DrFaust92 marked this pull request as ready for review August 16, 2024 00:52
@DrFaust92 DrFaust92 force-pushed the reservation-affinity branch from 9c40885 to 146ef14 Compare August 16, 2024 00:53
@DrFaust92
Copy link
Contributor Author

apeabody I think this should work properly now, did some local testing on this

@apeabody
Copy link
Collaborator

/gcbrun

@DrFaust92 DrFaust92 force-pushed the reservation-affinity branch from 146ef14 to a1d17dc Compare August 25, 2024 18:44
@DrFaust92 DrFaust92 requested a review from apeabody August 25, 2024 19:03
@apeabody
Copy link
Collaborator

/gcbrun

1 similar comment
@apeabody
Copy link
Collaborator

/gcbrun

@DrFaust92
Copy link
Contributor Author

apeabody looks good?

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 @DrFaust92!

One comment on the documentation, otherwise I think this looks good to me.

autogen/main/README.md Outdated Show resolved Hide resolved
@DrFaust92 DrFaust92 force-pushed the reservation-affinity branch from 113fb17 to cc2ccf5 Compare August 28, 2024 02:50
@DrFaust92 DrFaust92 requested a review from apeabody August 28, 2024 03:12
@apeabody
Copy link
Collaborator

/gcbrun

@apeabody apeabody merged commit 7cc0626 into terraform-google-modules:master Aug 29, 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.

Add reservation_affinity to node_pools config
2 participants