-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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: update warning message for the swap check #125157
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. |
/cc @neolit123 @pacoxu |
cmd/kubeadm/app/preflight/checks.go
Outdated
return []error{errors.New("swap is supported for cgroup v2 only; " + | ||
"the NodeSwap feature gate of the kubelet is beta and enabled by default; " + | ||
"the kubelet must be properly configured to use swap; " + | ||
"or disable swap on node")}, nil |
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.
Is there a document link about the NodeSwap feature gate we can add here?
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.
Yes, I find two links that are related to the swap memory configuration in Kubernetes.
-
https://kubernetes.io/docs/concepts/architecture/nodes/#swap-memory
-
(text is out-date, need to update) https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/#before-you-begin
Swap configuration. The default behavior of a kubelet was to fail to start if swap memory was detected on a node.
Swap has been supported since v1.22. And since v1.28, Swap is supported for cgroup v2 only;
the NodeSwap feature gate of the kubelet is beta but disabled by default.
...
Which link is preferred?
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.
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 remove "the NodeSwap feature gate of the kubelet is beta and enabled by default; ". once the feature is GA, we don't need to change warning messages again.
cc @pacoxu @neolit123 @SataQiu
https://kubernetes.io/docs/concepts/architecture/nodes/#swap-memory contains this information.
To enable swap on a node, the NodeSwap feature gate must be enabled on the kubelet (default is true),
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.
once the feature is GA we should remove the kubeadm check entirely.
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.
Thank you @kannon92
Any suggestions on my changes? please take another look, thank you. @pacoxu @SataQiu @neolit123
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.
continuing to default a flag with a certain value so that the kubelet fails for a supposedly supported feature would be confusing for future and existing users...
does this mean we need to maintain a warning about swap forever in kubeadm?
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.
According to #125157 (comment), I think so.
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 recall discussions in kubernetes/kubeadm#2563 about removing the kubeadm preflight check about swap completely.
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 agree it’s not the best UX. Unfortunately the fail swap on flag predates the node swap feature by a few years.
It’s important to state that one can provision kubelet with swap enabled but the feature is disabled. This can mean that kubelet runs on a swap node but no pods actually use swap. The swap feature is about how certain workloads (Burstable QoS) could use swap memory.
We haven’t really discussed the option for defaulting fail swap to false as part of this KEP.
If one provisions kubelet with swap they don’t need to set anything with swap feature unless they want workloads to actually use swap. Default is to have NodeSwap on with swap disabled for all workloads.
/lgtm |
LGTM label has been added. Git tree hash: 49b5fa83a915179358bbd925b42e5985c7ee16a7
|
this does not fall under the critical, blocking bug category, so we should try not to backport similar changes to reduce backport noise. |
/release-note-edit
|
please don't block the changes to the warning text on my comments. |
Co-authored-by: Lubomir I. Ivanov <[email protected]>
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.
/approve
/lgtm |
LGTM label has been added. Git tree hash: 76e3e8b4df9c56001058603bb9e5b2aa43c163bd
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carlory, 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
xref kubernetes/kubeadm#2563
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: