Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 4, 2024
Merged

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

merged 1 commit into from
Oct 4, 2024

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 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. 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 mentioned this pull request May 23, 2024
Open
6 tasks
@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
neolit123
neolit123 reviewed May 30, 2024
View reviewed changes
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

neolit123
neolit123 reviewed May 30, 2024
View reviewed changes
keps/sig-cluster-lifecycle/kubeadm/4656-add-instance-configuration/README.md Outdated

## 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 •
edited
Loading

@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.

@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 HirazawaUi force-pushed the add-instanceconfig branch from 82459f9 to 080c92f Compare June 12, 2024 17:05
@HirazawaUi
Copy link
Contributor Author

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

pacoxu
pacoxu reviewed Jun 13, 2024
View reviewed changes
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.

@HirazawaUi HirazawaUi force-pushed the add-instanceconfig branch from 080c92f to e069c54 Compare June 13, 2024 14:31
@neolit123
Copy link
Member

@HirazawaUi release cycle 1.32 just started. would you be able to continue working on this?

@HirazawaUi
Copy link
Contributor Author

Yes, I can still work on it and am waiting for further review of this PR

@neolit123
Copy link
Member

Yes, I can still work on it and am waiting for further review of this PR

thanks, i think end date for KEPs is 10th of October.

neolit123
neolit123 reviewed Sep 27, 2024
View reviewed changes
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.

thanks for the updates @HirazawaUi !
i added a few final comments from me but this seems good to become alpha in 1.32.

keps/sig-cluster-lifecycle/kubeadm/4656-add-kubelet-instance-configuration/kep.yaml Outdated
authors:
- "@HirazawaUi"
owning-sig: sig-cluster-lifecycle
status: provisional
Copy link
Member

Choose a reason for hiding this comment

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

please mark as implementable

Copy link
Member

Choose a reason for hiding this comment

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

still applies ^

should be:
status: implementable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that we cannot mark the status as implementable, because if the status of a KEP is marked as implementable, we will verify that its related PRR file exists.
ref:

enhancements/pkg/kepval/approval.go

Lines 100 to 107 in 48f928a

func isPRRRequired(kep *api.Proposal) (required bool, err error) {
logrus.Debug("checking if PRR is required")
required = kep.Status == api.ImplementableStatus || kep.Status == api.ImplementedStatus
if !required {
return false, nil
}

@neolit123
Copy link
Member

ping @HirazawaUi we are not as strict about the deadline of 10th Oct, but that's the official enhancement freeze.
https://github.com/kubernetes/sig-release/tree/master/releases/release-1.32

@HirazawaUi
Copy link
Contributor Author

@neolit123 the update has been made. Please review it again. If it gets merged, I will try to recall some details about it from memory and complete the implementation of the PR as soon as possible :)

@neolit123
Copy link
Member

@neolit123 the update has been made. Please review it again. If it gets merged, I will try to recall some details about it from memory and complete the implementation of the PR as soon as possible :)

did you forget to add the latest changes?
seems like they were not made.

(check my latest comments on the various threads)

@HirazawaUi
Copy link
Contributor Author

Ah, I'm sorry for my carelessness, I don't even know why I did that yesterday, but when I marked the status of the KEP as implementable, the validation procedure of the KEP will not pass because it requires a PRR file
ref: #4658 (comment)

@neolit123
Copy link
Member

Ah, I'm sorry for my carelessness, I don't even know why I did that yesterday, but when I marked the status of the KEP as implementable, the validation procedure of the KEP will not pass because it requires a PRR file ref: #4658 (comment)

we don't do PRR for kubeadm.

check this existing KEP;
https://github.com/kubernetes/enhancements/blob/master/keps/sig-cluster-lifecycle/kubeadm/4214-separate-super-user-kubeconfig/kep.yaml#L18-L19

use 0.0 for latest milestone.
and keep it at stage alpha.

neolit123
neolit123 reviewed Oct 3, 2024
View reviewed changes
keps/sig-cluster-lifecycle/kubeadm/4656-add-kubelet-instance-configuration/README.md Outdated

## Proposal

We will add a new file `/var/lib/kubelet/instance-config.yaml` to customize the CRI socket of each node. This file will be merged with `/var/lib/kubelet/config.yaml` in the process of kubeadm init/join in a similar way to `kubeletconfiguration patch target` (refer to its code implementation), If the user uses the `--patch` args, the patch file provided by the user will be given priority.
Copy link
Member

@neolit123 neolit123 Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
We will add a new file `/var/lib/kubelet/instance-config.yaml` to customize the CRI socket of each node. This file will be merged with `/var/lib/kubelet/config.yaml` in the process of kubeadm init/join in a similar way to `kubeletconfiguration patch target` (refer to its code implementation), If the user uses the `--patch` args, the patch file provided by the user will be given priority.
We will add a new file `/var/lib/kubelet/instance-config.yaml` to customize the CRI socket of each node. This file will be merged with `/var/lib/kubelet/config.yaml` in the process of kubeadm init/join by using the `kubeletconfiguration` patch target. If the user uses the `kubeletconfiguration` with `--patches`, the patch file provided by the user will be given priority.

some minor issues to fix.

neolit123
neolit123 reviewed Oct 4, 2024
View reviewed changes
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.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, neolit123

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2024
@neolit123 neolit123 mentioned this pull request Oct 4, 2024
Open
@k8s-ci-robot k8s-ci-robot merged commit f48dba5 into kubernetes:master Oct 4, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@pacoxu pacoxu pacoxu left review comments

@SataQiu SataQiu Awaiting requested review from SataQiu

+1 more reviewer

@carlory carlory carlory left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@neolit123 neolit123

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 lgtm "Looks good to me", indicates that a PR is ready to be merged. 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
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.
5 participants
@HirazawaUi @carlory @neolit123 @k8s-ci-robot @pacoxu