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: Add option for CPU manager policy #749

Merged

Conversation

@m10ev m10ev requested review from bharathkkb, Jberlinsky and a team as code owners November 29, 2020 13:45
@m10ev
Copy link
Contributor Author

m10ev commented Nov 29, 2020

Doesn't pass make docker_test_lint yet, still working on it.

@m10ev m10ev marked this pull request as draft November 29, 2020 13:54
@comment-bot-dev
Copy link

comment-bot-dev commented Nov 29, 2020

Thanks for the PR! 🚀
✅ Lint checks have passed.

@m10ev m10ev marked this pull request as ready for review November 29, 2020 14:47
Copy link
Member

@bharathkkb bharathkkb 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 PR @m10ev

@@ -520,6 +520,10 @@ resource "google_container_node_pool" "pools" {
}

boot_disk_kms_key = lookup(each.value, "boot_disk_kms_key", "")

kubelet_config {
Copy link
Member

Choose a reason for hiding this comment

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

Does this cause nodes to be cordoned and drained when introducing kubelet_config with a default of none?
If that is the case, it may make sense to make this a dynamic block that is only created if kubelet_config is explicitly set.

Copy link

Choose a reason for hiding this comment

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

Yes it will replace the nodes. Recommend to use dynamic block

Copy link

Choose a reason for hiding this comment

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

somethink like this should work

    dynamic "kubelet_config" {
      for_each = contains(keys(each.value), "cpu_manager_policy") ? [1] : []
      content {
        cpu_manager_policy = lookup(each.value, "cpu_manager_policy")
      }
    }

Copy link

@flecno flecno Dec 1, 2020

Choose a reason for hiding this comment

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

This is a good example to adapt here to be more consistent (it's still a google-beta feature too according to the docs right?):

Copy link
Member

Choose a reason for hiding this comment

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

@flecno that approach lgtm. Regarding beta-cluster gating, this is already within the conditional.

@@ -520,6 +520,14 @@ resource "google_container_node_pool" "pools" {
}

boot_disk_kms_key = lookup(each.value, "boot_disk_kms_key", "")

dynamic "kubelet_config" {
for_each = each.value.cpu_manager_policy[*]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this should be wrapped in a lookup because I don't know how maps work yet. Accessing a non-existent map key with . in the console seemed to be an error but the example I was iterating on seemed to pass the checks?

Copy link
Member

Choose a reason for hiding this comment

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

Something like this should work
#749 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for bearing with me while I learn tf :D

Copy link

Choose a reason for hiding this comment

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

you are welcome. 🙂
But I know, using this dynamic block like this feels a little bit ugly at the beginning but you will see later why and how you want to avoid breaking changes. And this is a change where terraform will not inform you that it leads to a recreation of the whole node-pool instances.

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

LGTM

@bharathkkb bharathkkb changed the title Add option for CPU manager policy feat: Add option for CPU manager policy Dec 3, 2020
@morgante morgante merged commit 721f846 into terraform-google-modules:master Dec 3, 2020
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.

None yet

5 participants