-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[KEP-2400] Node swap updates, GA criterias and clarifications #4701
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iholder101 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 |
05552e0
to
4905593
Compare
16c4878
to
b3f9708
Compare
Please update https://github.com/kubernetes/enhancements/blob/master/keps/prod-readiness/sig-node/2400.yaml and update missing bits of the PRR questionaire. |
b3f9708
to
443db7f
Compare
Thanks @deads2k!
I see you're the assigned approver for alpha/beta.
Done! PTAL :) |
f4af444
to
4124649
Compare
/retitle [KEP-2400] Node swap ppdates, GA criterias and clarifications |
D'oh /retitle [KEP-2400] Node swap updates, GA criterias and clarifications |
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.
Thanks for the PR.
Here's a mix of feedback; I hope it is all useful.
@@ -33,7 +33,7 @@ latest-milestone: "v1.30" | |||
milestone: | |||
alpha: "v1.22" | |||
beta: "v1.30" | |||
stable: "TBD" | |||
stable: "v1.32" |
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.
stable: "v1.32" | |
#stable: "v1.32" |
(AIUI)
We can put in a suggestion, but until we're planning that release, it's not a commitment. If we are planning that release, we should change latest-milestone
and stage
I think.
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 that we should indeed plan it for next release.
The latest-milestone
comments read:
# The most recent milestone for which work toward delivery of this KEP has been
# done. This can be the current (upcoming) milestone, if it is being actively
# worked on.
The milestone's documentation reads:
# The milestone at which this feature was, or is targeted to be, at each stage.
Since we target 1.32, miltstone
seemed like the right place. I've added it now to latest-milestone
.
What's important to me is that we indeed plan and commmit to releasing in 1.32, which there's a broad consensus of doing. I've even volunteered to not pursue GAing in 1.31 to reduce pressure from the sig-node approvers and ensure we do so in 1.32 with no pressure.
4124649
to
a85dd1c
Compare
8e73c9e
to
9f8f8b8
Compare
* If the kernel version equals or is above 6.4, the tmpfs noswap option is being used when necessary. | ||
* Else, kubelet would try to mount a dummy volume with the tmpfs noswap option to understand whether the option is | ||
backported. If the mount succeeds, the tmpfs noswap option is being used when necessary. | ||
* Else, kubelet would raise a warning log entry about the option not being supported and the possible risk. |
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 it will mount it anyway, right?
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.
AFAIK kubelet will fail to mount if the kernel does not support this feature. So it won’t be mounted in the “correct” way due to a limitation of the kernel.
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 it will mount it anyway, right?
Not sure I understand, mount what?
This "dummy mount" will fail if the tmpfs noswap is not supported. It is basically a method to determine whether the noswap option is supported or not. If it is supported, it'll be used to mount memory-backed volumes. If not, a warning log entry will be raised, and memory-backed volumes will be mounted without the noswap option.
Hope this answers your question, let me know if you need more info
@sftim @deads2k @haircommander @SergeyKanzhelev @mrunalp Would you kindly have another look at this? Is this anything missing? I believe that most of this PR has already been agreed upon in previous sig-node calls. Thanks! |
We have been using this feature for a while in production and it generally performs well. We still need to set |
/milestone 1.32 |
@haircommander: The provided milestone is not valid for this repository. Milestones in this repository: [ Use In response to this:
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. |
/milestone v1.32 |
9f8f8b8
to
c62df05
Compare
Signed-off-by: Itamar Holder <iholder@redhat.com>
…avior Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
Signed-off-by: Itamar Holder <iholder@redhat.com>
c62df05
to
ed2503b
Compare
I wish to update that we agreed [1] that NFD support for swap brings enough debugability for this KEP. We'll definitely circle back to improving this as part of the follow-up API extension KEP. Thanks @kannon92 for bringing swap support to NFD, and thanks @dchen1107 for helping pushing the discussion forward! With this, I believe that this PR is entirely ready to get in. [1] The discussion started at the sig-node meeting, then continued to this comment. |
Add updates, GA criterias and clarifications
This PR updates the KEP in the following ways:
Emphasize that this KEP is about basic swap enablement
The original KEP indicated that pod-level swap APIs are out of scope:
enhancements/keps/sig-node/2400-node-swap/README.md
Lines 163 to 166 in 155a949
enhancements/keps/sig-node/2400-node-swap/README.md
Lines 142 to 144 in 155a949
However, the lack of APIs and the implicit nature of the current implementation sometimes brings suggestions to extend the API under this KEP.
This KEP focuses on a basic swap enablement. Follow-up KEPs regarding several topics (e.g. customization, zram/zswap suport, and more) will be introduced in the near future, in which we would be able to design and implement each extension in a focused way.
To ensure we're on the same page, this topic was recently raised in a sig-node meeting. In this meeting there was a very broad consensus that this approach makes sense, especially since the NodeSwap feature is important to many different parties which want it to "just work".
This PR updates the KEP to emphasize this approach.
GA criterias
The PR adds GA criterias, alongside the intent to GA in version 1.32.
Make sure PRR is ready
Updates
Since the last KEP updates, many improvements were made and many concerns were addressed. For example:
This PR updates the KEP to reflect these updates.