KEP 3983: target to beta for 1.29 #4242
Conversation
sohankunkerkar
commented
Sep 25, 2023
sohankunkerkar
commented
- One-line PR description: Updates target for KEP to beta in 1.29
- Issue link: Add support for a drop-in kubelet configuration directory #3983
- Other comments:
k8s-ci-robot
added
cncf-cla: yes
|
Hi @sohankunkerkar. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
kind/kep
k8s-ci-robot
added
sig/node
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
sohankunkerkar
force-pushed
the
update-3983
branch
from
88cb5d1 to
6972d98
Compare
sohankunkerkar
force-pushed
the
update-3983
branch
from
6972d98 to
6b7ca9d
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@SergeyKanzhelev could you take a look at it once? |
sohankunkerkar
force-pushed
the
update-3983
branch
from
6b7ca9d to
a0a60fa
Compare
k8s-ci-robot
removed
the
lgtm
|
still |
k8s-ci-robot
added
the
lgtm
|
@SergeyKanzhelev PTAL |
sohankunkerkar
force-pushed
the
update-3983
branch
from
a0a60fa to
b5c5116
Compare
sohankunkerkar
force-pushed
the
update-3983
branch
from
c831f9a to
ff0addc
Compare
|
still /lgtm |
k8s-ci-robot
added
the
lgtm
| @@ -65,7 +66,7 @@ Items marked with (R) are required *prior to targeting to a milestone / release* | |||
|
|
|||
| ## Summary | |||
|
|
|||
| Add support for a drop-in configuration directory for the Kubelet. This directory can be specified via a "--config-dir" flag, and configuration files will be processed in alphanumeric order. The flag will be empty by default and if not specified, drop-in support will not be enabled. Establishment of conventions for configuration processing will be done, and further work can be done to add this support for other components. | |||
| Add support for a drop-in configuration directory for the Kubelet. This directory can be specified via a `--config-dir` flag, and configuration files will be processed in alphanumeric order. The flag will be empty by default and if not specified, drop-in support will not be enabled. During the alpha phase, we introduced an environment variable called `KUBELET_CONFIG_DROPIN_DIR_ALPHA` to control the drop-in configuration directory for testing purposes. In the beta phase, we plan to enhance the feature with E2E testing and streamline the configuration process. As part of this optimization, we will remove the `KUBELET_CONFIG_DROPIN_DIR_ALPHA` environment variable, simplifying configuration management. The feature will remain off by default during the beta phase, and we will consider enabling it by default in future releases. | |||
There was a problem hiding this comment.
Is the last line necessary? Since we have a CLI flag if that flag it isn't used even when it goes GA it will be off by default.
There was a problem hiding this comment.
we can set a default value for GA so users don't need ti specify
| * Extend kubelet configuration parsing code to handle files in the drop-in directory. | ||
| * Define Kubernetes best-practices for configuration definitions, similarly to [API conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md). This is intended for other maintainers who would wish to setup a configuration object that works well with drop-in directories. | ||
| * As a goal for beta, Add ability to easily view the effective configuration that is being used by kubelet. | ||
| * In the beta phase, we are going to remove the KUBELET_CONFIG_DROPIN_DIR_ALPHA environment variable, which was introduced during the alpha phase. Users can enable the feature explicitly using the --config-dir flag. |
There was a problem hiding this comment.
this statement does not belong here.
| * Extend kubelet configuration parsing code to handle files in the drop-in directory. | ||
| * Define Kubernetes best-practices for configuration definitions, similarly to [API conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md). This is intended for other maintainers who would wish to setup a configuration object that works well with drop-in directories. | ||
| * As a goal for beta, Add ability to easily view the effective configuration that is being used by kubelet. |
There was a problem hiding this comment.
Why this goal was removed? Can we make it a goal without the attachment to a specific milestone
| @@ -182,15 +183,15 @@ As a cluster admin, I would like to have cgroup management and log size manageme | |||
|
|
|||
| ##### Unit tests | |||
|
|
|||
| * Unit tests will be added, details to be added here | |||
| * cmd/kubelet/app: 07-17-2023 | |||
| @@ -182,15 +183,15 @@ As a cluster admin, I would like to have cgroup management and log size manageme | |||
|
|
|||
| ##### Unit tests | |||
|
|
|||
| * Unit tests will be added, details to be added here | |||
| * cmd/kubelet/app: 07-17-2023 | |||
|
|
|||
| ##### Integration tests | |||
|
|
|||
| * : | |||
There was a problem hiding this comment.
say N/A if not applicable (likely are not applicable)
| - Implement the ability to directly query the kubelet via API for its full effective configuration, covering both standard mode and standalone kubelet mode. | ||
| - Thoroughly testing this querying capability is a crucial graduation criterion. | ||
| - Establish E2E testing requirements to enable accurate reporting of kubelet.conf.d directory and its contents in the configz endpoint. This enables users to inspect the effective configuration used by the kubelet. |
There was a problem hiding this comment.
these three items are basically saying the same?
There was a problem hiding this comment.
I will club that into a single comment.
|
|
||
| #### GA | ||
|
|
||
| Add documentation process on the Kubernetes website, specifically focusing on detailing how lists and structures are merged in the kubelet configuration file. |
There was a problem hiding this comment.
let's do it in Beta. Changes after beta will be hard to do. Ideally this kind of API description must be done in alpha, in case we need to change how those structured will be merged
There was a problem hiding this comment.
thinking about it - @mrunalp we need to decide whether we want to promote to beta before the merge algorithm is documented and (ideally) validated on variety of existing configs. Changing this after beta will be a challenge
There was a problem hiding this comment.
I think we should document the merge behavior for beta.
There was a problem hiding this comment.
I think this documentation should be for beta as well. part of the moving to beta will be testing, and in testing we will establish what can be relied on in documentation
There was a problem hiding this comment.
Okay, I will move that info to Beta, so what could be the GA criteria then?
There was a problem hiding this comment.
Here's a suggestion: Set the --config-dir option to specify the default configuration directory, ensuring it is enabled by default and points to an appropriate path `
There was a problem hiding this comment.
GA could getting enough feedback from users as well as establishing defaults if any.
sohankunkerkar
force-pushed
the
update-3983
branch
from
ff0addc to
6fb5c31
Compare
k8s-ci-robot
removed
the
lgtm
sohankunkerkar
force-pushed
the
update-3983
branch
from
6fb5c31 to
4463e2b
Compare
| - Implement the capability to query the kubelet's full effective configuration via API, covering both standard mode and standalone kubelet mode. Thoroughly test this functionality, ensuring accurate reporting of kubelet.conf.d directory and contents in the configz endpoint. | ||
| - Remove the environment variable `KUBELET_CONFIG_DROPIN_DIR_ALPHA`, introduced during the Alpha phase, to streamline the user experience by simplifying configuration management. | ||
| - Keep the feature disabled by default in the Beta phase. Explicit opt-in activation is required to enable the feature. | ||
| - Document the process on the Kubernetes website, specifically focusing on detailing how lists and structures are merged in the kubelet configuration file. |
There was a problem hiding this comment.
And very last thing - we need to document the /healthz endpoint - making it official. I didn't find any official documentation on it, I think we should fix it as we will depend on it now.
There was a problem hiding this comment.
Do you mean the /configz endpoint?
Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
sohankunkerkar
force-pushed
the
update-3983
branch
from
4463e2b to
aefad4c
Compare
| - [] Feature gate | ||
| N/A | ||
|
|
||
| In addition to configuring the KUBELET_CONFIG_DROPIN_DIR_ALPHA environment variable, administrators must explicitly set the --config-dir flag in the kubelet's command-line interface (CLI) to enable this feature.. Starting from the beta phase, specifying the --config-dir flag is the only way to enable this feature. The default value for --config-dir is an empty string, which means the feature is disabled by default. |
There was a problem hiding this comment.
ok, I can live with that. it still doesn't address one thing - the fact that people pull example commands from the web, and if those commands include the --config-dir flag, the users of those will not actually be acknowledging that they know they are using a beta feature.
There was a problem hiding this comment.
How about we add a note about that in the example?
There was a problem hiding this comment.
Something that was done elsewhere is to have alpha/beta feature gates. It's an error to use an alpha feature unless the alpha feature gate is on (which it isn't by default). Same for beta, except that it is on by default. Cautious users can turn it off to ensure that they don't accidentally use beta features.
Not sure how applicable that could be here, just my two cents... 😄
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnbelamaric, mrunalp, sohankunkerkar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
