-
Notifications
You must be signed in to change notification settings - Fork 40.7k
kubeadm: Graduate NodeLocalCRISocket Feature gate to beta #131981
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
base: master
Are you sure you want to change the base?
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HirazawaUi 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, the two issues from the Alpha phase have already been addressed:
- [NodeLocalCRISocket]: Remove
container-runtime-endpoint
flag when kubeadm upgrade #129278: Remove the flag--container-runtime-endpoint
from the/var/lib/kubelet/kubeadm-flags.env
file during kubeadm upgrade.- [NodeLocalCRISocket]: remove kubeadm.alpha.kubernetes.io/cri-socket annotation when kubeadm upgrade #129279: Remove the
kubeadm.alpha.kubernetes.io/cri-socket
annotation from a given node during kubeadm upgrade.
these are missing in
- Add kubelet instance configuration to configure CRI socket for each node kubeadm#3042
please add them
kubeadm: Graduate NodeLocalCRISocket to beta.
better provide one additional sentence for context what the FG does e.g. what will happen for new clusters and on upgrade to 1.34.
Therefore, there is no additional work required for the Beta phase. Promoting the feature gate to Beta and enabling it by default should suffice.
- we need to update the website
kubeadm init
page where this FG is documented. dev-1.34 branch. - we also need to flip the kinder e2e to test the disabled case, during alpha we explicitly enabled the FG.
Added.
Added to the release note. Could you please review if the wording accurately?
Will be added.
Will be added, but we do not support disabling a feature gate in the beta stage after enabling it during alpha (creating/adding/upgrading). I vaguely recall we previously confirmed this practice is prohibited, but I cannot find this restriction documented in the KEP. Could it be that I inadvertently overlooked this section while implementing the KEP? |
/release-note-edit
just some minor formatting / edits. |
no, actually it's a standard practice for kubeadm e2e to flip the FG to disabled once the FG goes beta. |
/hold |
for 1.34, it's important to allow the user to disable the FG in the kubeadm-config CM and then call kubeadm upgrade which will not introduce the changes which the FG does. if a 1.32/1.33 cluster had the FG enabled for alpha and during upgrade to 1.34 (beta) they want to disable it, that's not so important and we can claim it as unsupported. you can still check if it's easy to manage on our side or not. |
/assign |
252a410
to
83154d3
Compare
83154d3
to
a3a0cea
Compare
a3a0cea
to
a036c4f
Compare
criSocket, ok = node.ObjectMeta.Annotations[constants.AnnotationKubeadmCRISocket] | ||
if !ok { | ||
return missingAnnotationError | ||
kubeletConfig, err := readKubeletConfig(constants.KubeletRunDirectory, constants.KubeletInstanceConfigurationFileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't matter too much but if we want to be precise any code path for reading the kubelet instance config should be only done if the feature gate is enabled.
- only throw error on missing instance config if the FG is enabled
- only try reading the instance config if the FG is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we do this, we won't be able to handle the scenario where the Feature gate is enabled in the alpha stage but disabled when upgrading to beta. This is because, during the alpha phase, the kubeadm.alpha.kubernetes.io/cri-socket annotation on the Node object would have already been removed. Then, if the Feature gate is disabled in the beta stage, the code would throw an error when execution reaches this point :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the problem is that we shouldn't have started deleting the annotation in the alpha phase.
so with the fg enabled with could write the file and prefer to read it instead of the annotation, but fallback reading the annotation. then during ga when the fg is locked to enabled we can start to remove the annotation from the node object.
either way, even if we messed the planning up a bit, the important aspect would be to have a smooth transition for the users and not to get any complaints. the annotation is widely used, unfortunately.
/lgtm
/cc pacoxu
do you have any comments before this merges?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we already removed the annotation if the FG is enabled in v1.32/v1.33, I can accept the current change.
we won't be able to handle the scenario where the Feature gate is enabled in the alpha stage but disabled when upgrading to beta.
Do we need to add a warning for this scarnerio? (Even there is a kubelet instance config file).
Besides, only 1 nit and a question(should we include the config path in the error message, or already included.)
LGTM label has been added. Git tree hash: 3dba4941108d39e8c444ccd0049ff47f9f0102ea
|
if features.Enabled(clusterCfg.FeatureGates, features.NodeLocalCRISocket) { | ||
_, err = os.Stat(filepath.Join(constants.KubeletRunDirectory, constants.KubeletInstanceConfigurationFileName)) | ||
criSocket, ok := node.Annotations[constants.AnnotationKubeadmCRISocket] | ||
if !ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok=false
means that user already runs upgrade with feature gate NodeLocalCRISocket=true.
If NodeLocalCRISocket=false
and ok=false
, should we log a warning message?
- Below is an error, if no annotation and kubelet instance config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can print this warning log, but it might be better to:
- When the Feature gate is not enabled, we print a warning log if the cri-socket annotation is not found.
- When the Feature gate is enabled, we print a warning log if the kubelet instance config is not found.
Alternatively, we could avoid printing warning logs altogether and handle the issue for the user, making the process imperceptible to them.
Considering this issue stems from our mistake, is it necessary to burden the user with it? The user did nothing wrong but still receives a warning ╮( ̄▽ ̄)╭.
return missingAnnotationError | ||
kubeletConfig, err := readKubeletConfig(constants.KubeletRunDirectory, constants.KubeletInstanceConfigurationFileName) | ||
if err != nil { | ||
return errors.Wrapf(err, "could not read kubelet instance configuration on node %q", nodeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we show the absolute path in the message? (Maybe not needed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we generally avoid printing absolute paths in logs, whether it's the kubelet configuration or the kubelet instance config.
a036c4f
to
ad3a13e
Compare
New changes are detected. LGTM label has been removed. |
/retest-required |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I reviewed the goals required for the Beta phase outlined in the KEP:
I believe maintaining the current existing behavior is sufficient because some users who did not adopt this feature during the Alpha phase may still rely on
kubeadm-flags.env
to generate/var/lib/kubelet/instance-config.yaml
, which ultimately merges into/var/lib/kubelet/config.yaml
.Additionally, the two issues from the Alpha phase have already been addressed:
[NodeLocalCRISocket]: Remove
container-runtime-endpoint
flag when kubeadm upgrade #129278: Remove the flag--container-runtime-endpoint
from the/var/lib/kubelet/kubeadm-flags.env
file during kubeadm upgrade.[NodeLocalCRISocket]: remove kubeadm.alpha.kubernetes.io/cri-socket annotation when kubeadm upgrade #129279: Remove the
kubeadm.alpha.kubernetes.io/cri-socket
annotation from a given node during kubeadm upgrade.Therefore, there is no additional work required for the Beta phase. Promoting the feature gate to Beta and enabling it by default should suffice.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: