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

Conflicting comment and implementation for kubelet_enforce_node_allocatable #9552

Closed
yuha0 opened this issue Dec 8, 2022 · 2 comments
Closed
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@yuha0
Copy link
Contributor

yuha0 commented Dec 8, 2022

The current sample group_vars has this comment telling users to use comma to separate levels in kubelet_enforce_node_allocatable:

A comma separated list of levels of node allocatable enforcement to be enforced by kubelet.
Acceptable options are 'pods', 'system-reserved', 'kube-reserved' and ''. Default is "".

# A comma separated list of levels of node allocatable enforcement to be enforced by kubelet.
# Acceptable options are 'pods', 'system-reserved', 'kube-reserved' and ''. Default is "".
# kubelet_enforce_node_allocatable: pods

However, in kubelet config template, the value is split by .split()

{% if kubelet_enforce_node_allocatable is defined and kubelet_enforce_node_allocatable != "\"\"" %}
{% set kubelet_enforce_node_allocatable_list = kubelet_enforce_node_allocatable.split() %}
enforceNodeAllocatable:
{% for item in kubelet_enforce_node_allocatable_list %}
- {{ item }}
{% endfor %}
{% endif %}

split() uses whitespace as delimiter by default, not comma.

If I write this in my group_vars:

kubelet_enforce_node_allocatable: pods,system-reserved

I will get this in the rendered kubelet config file:

enforceNodeAllocatable: 
- pods,system-reserved

And kubelet will refuse to start:

Failed to validate kubelet configuration" err="invalid configuration: option \"pods,system-reserved\" specified for enforceNodeAllocatable

IMHO, we should either

  1. change the template to use comma as delimiter (split(',')), or
  2. Change the comment section to tell users to use whitespace as delimiter

It's worth noting that, even after fixing this particular issue,kubelet_enforce_node_allocatable still cannot take anything other than pods. This is because the current kubelet config template does not have systemReservedCgroup or kubeReservedCgroup fields and they are not configurable. These two fields are required for adding kubeReserved/systemReserved to enforceNodeAllocatable. See documentation for kubelet configuration:

https://kubernetes.io/docs/reference/config-api/kubelet-config.v1beta1/
enforceNodeAllocatable
[]string

This flag specifies the various Node Allocatable enforcements that Kubelet needs to perform. This flag accepts a list of options. Acceptable options are none, pods, system-reserved and kube-reserved. If none is specified, no other options may be specified. When system-reserved is in the list, systemReservedCgroup must be specified. When kube-reserved is in the list, kubeReservedCgroup must be specified.

It seems like the two fields can be added via #9209

@yuha0 yuha0 added the kind/bug Categorizes issue or PR as related to a bug. label Dec 8, 2022
@yuha0 yuha0 changed the title Conflicting comment and implementation Conflicting comment and implementation for kubelet_enforce_node_allocatable Dec 8, 2022
@yuha0
Copy link
Contributor Author

yuha0 commented Feb 5, 2023

Looks like this issue was brought up again in #9693 and fixed in #9694. Thanks @Tristan971 !

Closing...

@yuha0 yuha0 closed this as completed Feb 5, 2023
@Tristan971
Copy link
Contributor

Oh I’d completely missed this issue somehow when reporting it!

Well, all’s good that ends well :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

2 participants