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(cgroup): Add option to set cgroup for autopilot modules #2273

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

scipe
Copy link

@scipe scipe commented Feb 9, 2025

Description

Added node_pools_cgroup_mode parameter for autopilot clusters. The variable is string with follow possible values:

  • CGROUP_MODE_UNSPECIFIED
  • CGROUP_MODE_V1
  • CGROUP_MODE_V2

By default value is empty and linux_node_config (and cgroup_mode) blocks will be not created.

Checks

  • docker_test_lint - ✅ (no issues related to my changes)
  • docker_generate_docs - ✅

Related issues

#2264

@scipe scipe requested review from apeabody, ericyz and a team as code owners February 9, 2025 18:41
@scipe scipe force-pushed the chore/asonn-cgroup-support-for-autopilot branch from c22bf3a to 03f2abe Compare February 9, 2025 18:41
@scipe scipe marked this pull request as draft February 10, 2025 09:17
@scipe
Copy link
Author

scipe commented Feb 10, 2025

Moved to draft. Found the issue.

@scipe scipe marked this pull request as ready for review February 10, 2025 09:40
@scipe
Copy link
Author

scipe commented Feb 10, 2025

Issue fixed and tested in my environment.
Screenshot 2025-02-10 at 11 40 21

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

For test coverage, please add one usage to an Autopilot example and testdata similar to: #2224

modules/beta-autopilot-private-cluster/variables.tf Outdated Show resolved Hide resolved
@Ameausoone
Copy link

I open the same PR, I just already fix the null remark.

I started to work on this issue before this PR was opened, but you open the PR before me. So I don't want to "cut the grass under your foot", so if you have time to fix this PR, and delete mine #2282, plz feel free to do it.

@scipe
Copy link
Author

scipe commented Feb 16, 2025

Sorry for the delay. I will take a look at the rest staff tomorrow.

@scipe scipe force-pushed the chore/asonn-cgroup-support-for-autopilot branch from eb97093 to 3176df6 Compare February 17, 2025 11:03
@scipe
Copy link
Author

scipe commented Feb 17, 2025

Hi @apeabody! Pushed new commits for changes you requested.

@scipe scipe requested a review from apeabody February 17, 2025 11:44
@apeabody
Copy link
Collaborator

/gcbrun

@scipe
Copy link
Author

scipe commented Feb 18, 2025

@apeabody I see one of workflow failed but I do not see the logs from Cloud Build. Can you provide me some details please and I will fix it.
Also do you wanna me rebase?

@apeabody
Copy link
Collaborator

@apeabody I see one of workflow failed but I do not see the logs from Cloud Build. Can you provide me some details please and I will fix it. Also do you wanna me rebase?

Looks like we have a perma-diff, but not sure if it's because of this change, or the provider/API. Will need to wait for the result from #2278

Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: Terraform used the selected providers to generate the following execution
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: plan. Resource actions are indicated with the following symbols:
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:   ~ update in-place
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: Terraform will perform the following actions:
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:   # module.gke.google_container_cluster.primary will be updated in-place
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:   ~ resource "google_container_cluster" "primary" {
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:         id                                       = "projects/ci-gke-147b091e-d6f7/locations/us-central1/clusters/simple-ap-private-non-default-sa-cluster"
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:         name                                     = "simple-ap-private-non-default-sa-cluster"
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:         # (35 unchanged attributes hidden)
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:       ~ node_pool_auto_config {
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:             # (1 unchanged attribute hidden)
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:           + network_tags {}
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:             # (1 unchanged block hidden)
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:         }
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:         # (37 unchanged blocks hidden)
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206:     }
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: 
Step #79 - "verify simple-autopilot-private-non-default-sa": TestSimpleAutopilotPrivateNonDefaultSA 2025-02-18T17:49:06Z command.go:206: Plan: 0 to add, 1 to change, 0 to destroy.

@apeabody
Copy link
Collaborator

/gcbrun

(retest)

@scipe
Copy link
Author

scipe commented Feb 19, 2025

Hey @apeabody. Is anything else I can help here with?

@apeabody
Copy link
Collaborator

Hey @apeabody. Is anything else I can help here with?

Hi @scipe - Still seeing the perma-diff for network-tags, likely because it's now being created when var.node_pools_cgroup_mode != null. One option would be to switch it to a dynamic block with a condition similar to for_each = length(var.network_tags) > 0 || var.add_cluster_firewall_rules || var.add_master_webhook_firewall_rules || var.add_shadow_firewall_rules || var.insecure_kubelet_readonly_port_enabled != null ? [1] : [].

https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/pull/2273/files#diff-a3a07be3819af6f1a65ed243a613e69b544b7274d248a4b4d7d4d127aec40aeeL284

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.

3 participants