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

"azurerm_kubernetes_cluster" and "azurerm_kubernetes_cluster_node_pool" supports "kubelet_config", "linux_os_config" #11119

Merged
merged 7 commits into from
Jun 16, 2021

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented Mar 26, 2021

two new blocks in node pool: kubelet_config and linux_os_config

linux_os_config contains a sub block "sysctl_config", which could set the the kernel parameters, could refer to https://kubernetes.io/docs/tasks/administer-cluster/sysctl-cluster/

this two blocks are forcenew fields, if we try to update it, the rest api will report error code: CustomKubeletConfigOrCustomLinuxOSConfigCanNotBeChanged.

image

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @njuCZ

Thanks for this PR.

Taking a look through here whilst this is off to a good start, the descriptions for these fields don't make sense (in that they don't really specify what this does clearly) or are missing (e.g. "the sysctl value foo.bar") - would you be able to update these?

Thanks!


* `transparent_huge_page_defrag` - (Optional) Specifies the Transparent Huge Page defrag configuration. Possible values are `always`, `defer`, `defer+madvise`, `madvise` and `never`. Changing this forces a new resource to be created.

* `transparent_huge_page_enabled` - (Optional) Specifies the Transparent Huge Page enabled configuration. Possible values are `always`, `madvise` and `never`. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

this description doesn't make any sense, presumably this toggles if Transparent Huge Pages are enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


* `fs_aio_max_nr` - (Optional) The sysctl setting fs.aio-max-nr. Must be between `65536` and `6553500`. Changing this forces a new resource to be created.

* `fs_file_max` - (Optional) The sysctl setting fs.file-max. Must be between `8192` and `12000500`. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -454,6 +494,68 @@ A `ssh_key` block supports the following:

---

A `sysctl_config` block supports the following:

* `fs_aio_max_nr` - (Optional) The sysctl setting fs.aio-max-nr. Must be between `65536` and `6553500`. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


* `transparent_huge_page_defrag` - (Optional) Specifies the Transparent Huge Page defrag configuration. Possible values are `always`, `defer`, `defer+madvise`, `madvise` and `never`. Changing this forces a new resource to be created.

* `transparent_huge_page_enabled` - (Optional) Specifies the Transparent Huge Page enabled configuration. Possible values are `always`, `madvise` and `never`. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

(as below) this description doesn't make sense, should this be "specifies if Transparent Huge Pages are enabled?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/kubernetes_cluster.html.markdown Outdated Show resolved Hide resolved
@njuCZ
Copy link
Contributor Author

njuCZ commented Apr 7, 2021

Hi @tombuildsstuff thanks for your review and suggestions, I have added a partial test for those properties.

as for the doc for linuxOSConfig, those properties are used to Tuning linux kernel. Personally I think only professional users should set it, so tell the users which kernel parameter it correspond to is enough and might be more clear.

image

@liammoat
Copy link
Contributor

liammoat commented Apr 8, 2021

Related issue: #10038

@ghost ghost removed the waiting-response label Apr 8, 2021
@njuCZ njuCZ requested a review from tombuildsstuff April 14, 2021 02:44
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @njuCZ! I took another pass over this PR. It looks mostly good but we should update the documentation to point people to where they can find more information about these attributes.

@njuCZ
Copy link
Contributor Author

njuCZ commented May 21, 2021

@mbfrahry thanks for your suggestions! I have refined this PR.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

This should be good to go once the conflicts are fixed!

@njuCZ
Copy link
Contributor Author

njuCZ commented Jun 15, 2021

@mbfrahry I have rebased the latest code and resolved all conflicts
image

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@katbyte katbyte added this to the v2.64.0 milestone Jun 16, 2021
@katbyte katbyte merged commit 732e21d into hashicorp:master Jun 16, 2021
katbyte added a commit that referenced this pull request Jun 16, 2021
@github-actions
Copy link

This functionality has been released in v2.64.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants