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

kubeadm: Add kubelet instance configuration to configure CRI socket for each node #4658

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HirazawaUi
Copy link
Contributor

  • One-line PR description: Add kubelet instance configuration to configure CRI socket for each node.
  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: HirazawaUi
Once this PR has been reviewed and has the lgtm label, please assign cecilerobertmichon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels May 23, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 23, 2024
@HirazawaUi HirazawaUi force-pushed the add-instanceconfig branch 2 times, most recently from 746488e to 53e0fb5 Compare May 23, 2024 16:38
@HirazawaUi
Copy link
Contributor Author

/cc @neolit123 @SataQiu @pacoxu

@HirazawaUi HirazawaUi force-pushed the add-instanceconfig branch 2 times, most recently from ddc3eb1 to 40a8912 Compare May 26, 2024 11:55
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

@HirazawaUi thanks for starting work on the KEP.

  • the formatting is inconsistent. sometimes you use quotes around names sometimes not. some quotes are not closed. sometimes cri is not uppercase. please make everything consistent.
  • we shouldn't talk about the --cri-scoket flag, you can instead just the kubeadm CRI socket option, the kubelet CRI socket option, the CRI socket node annotation
  • this KEP has two important goals that are not so clear 1) don't write the --container-runtime-endpoint flag in the kubeadm-flags.env file 2) stop writing the annotation on the Node object, please make these clearer in the Goals section
  • i think the plan for init/join/upgrade is not very clear across the different kubeadm versions until this feature graduates

feature freeze is in two weeks June 14th
https://github.com/kubernetes/sig-release/tree/master/releases/release-1.31


## Drawbacks

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

how about using a feature gate, and why is that not our first choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially thought that this feature would be invisible to ordinary users, and feature gate would increase its complexity (and I'm not sure whether it makes sense to use feature gate on kubeadm), so I implemented it in multiple versions to control version skew and compatibility.

If we use feature gate, then the Design Details will become relatively simple. We only need to implement the feature and use feature gate to control whether it is enabled. There is no need to use multiple versions implemented to consider compatibility and version skew.

Let me think and redesign it based on feature gate.

Copy link
Member

Choose a reason for hiding this comment

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

if the feature gate is the main approach what alternatives do we have? this section "alternatives" must include one or more alternatives that are not the primary choice of the KEP.

@neolit123 neolit123 requested review from carlory and removed request for vincepri and fabriziopandini May 30, 2024 14:29
@HirazawaUi HirazawaUi force-pushed the add-instanceconfig branch 4 times, most recently from 55ea8f6 to 8b0eaf4 Compare June 4, 2024 13:59
@HirazawaUi
Copy link
Contributor Author

HirazawaUi commented Jun 4, 2024

@neolit123 Thanks, I have fixed according to your suggestion.

@carlory
Copy link
Member

carlory commented Jun 5, 2024

#3983 adds support for a drop-in kubelet configuration directory. It allows override the configuration for the Kubelet located at /etc/kubernetes/kubelet.conf.

In this KEP, we may need to consider how to work well with the feature in the context of the kubeadm tool.

cc @neolit123 @HirazawaUi

@neolit123
Copy link
Member

#3983 adds support for a drop-in kubelet configuration directory. It allows override the configuration for the Kubelet located at /etc/kubernetes/kubelet.conf.

In this KEP, we may need to consider how to work well with the feature in the context of the kubeadm tool.

cc @neolit123 @HirazawaUi

i prefer to not mix core functionality in kubeadm with 3983, because kubeadm already has the patches mechanism and it's an older feature.

if the user uses also 3983, those files will take precedence.

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

this is getting in a better shape.


## Drawbacks

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

if the feature gate is the main approach what alternatives do we have? this section "alternatives" must include one or more alternatives that are not the primary choice of the KEP.

@HirazawaUi HirazawaUi force-pushed the add-instanceconfig branch 4 times, most recently from 0c9ff56 to 82459f9 Compare June 5, 2024 15:40
@HirazawaUi
Copy link
Contributor Author

@neolit123 ready for review.

@HirazawaUi
Copy link
Contributor Author

HirazawaUi commented Jun 11, 2024

ping @neolit123, because the freeze is coming

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

@HirazawaUi thank you for the updates.
i'm aware that we are approaching feature freeze but this KEP is not in a clear state.
and i'm the only reviewer.

ideally we should have another approver from the kubeadm OWNER file to also +1/-1 the plan.


* A new e2e test will be added by using the kinder tool.

### Graduation Criteria
Copy link
Member

Choose a reason for hiding this comment

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

so there is a problem with the KEP currently

feature gates have two states: TRUE and FALSE

but the "Design details" and "Graduation Criteria" sections are organized as we both using and not using a feature gate, which is confusing.

your doc should be organized like this:

  • in "Design details" explain the new feature gate
  • say in which releases it will be alpha, beta, GA
  • explain when it will be enabled by default
  • explain what happens when it's enabled and not enabled to the different kubeadm commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some changes, please check if this is what we expected.


* A new e2e test will be added by using the kinder tool.

### Graduation Criteria
Copy link
Member

Choose a reason for hiding this comment

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

this section Graduation Criteria must explain what is required for the feature to graduate to Beta and GA . there is no graduation to Alpha.

@HirazawaUi
Copy link
Contributor Author

i'm aware that we are approaching feature freeze but this KEP is not in a clear state.

Ok, if the current KEP can't be approved before the freeze, I will continue to push it in v1.32.

@HirazawaUi
Copy link
Contributor Author

@SataQiu @pacoxu Do you have the time and willingness to review this KEP?

Copy link
Member

@pacoxu pacoxu left a comment

Choose a reason for hiding this comment

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

Overall LGTM.

+1 for this approach. Sorry for the late response.

### Goals

* kubeadm currently adds an annotation with the key `kubeadm.alpha.kubernetes.io/cri-socket` to each Node object. We will deprecate and remove it.
* Provide an instance configuration file named `/var/lib/kubelet/instance-config.yaml` for each node, in which the `ContainerRuntimeEndpoint` field is defined. During the `kubeadm init/join/upgrade` process, the instance configuration will be read and the `ContainerRuntimeEndpoint` field in `/var/lib/kubelet/config.yaml` will be overwritten.
Copy link
Member

@pacoxu pacoxu Jun 13, 2024

Choose a reason for hiding this comment

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

Just a naming question.
We have another file in the same dir named kubeadm-flags.env. Should this file be named with kubeadm prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not sure if we should follow the same naming convention as kubeadm-flags.env, let’s hear what @neolit123 thinks.

Copy link
Member

Choose a reason for hiding this comment

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

config.yaml is also kubeadm generated but is not prefixed with "kubeadm" 🤔
because of that i'm leaning towards using "instance-config.yaml", but happy to discuss more and hear more ideas around the naming.


- Gather feedback from developers and surveys.
- Implement changes in kubeadm upgrade apply/node GA phase.
- Update the phases documentation.
Copy link
Member

Choose a reason for hiding this comment

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

After GA, we may not add the old alpha annotation.

  • We may deprecate the annotation in Beta?

We can keep the annotation if the node is upgraded from older version, and it can be removed manually if they want then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can keep the annotation if the node is upgraded from older version, and it can be removed manually if they want then.

Yes, this is what we want to do, once the feature gate is enabled, we will not set the CRI socket annotation during kubeadm init/join, nor remove it on upgrade.

So I think it has nothing to do with which phase we are in, because we already have feature gates to help us control version skew.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants