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.

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

[install-kubeadm]: Update instructions to disable or tolerate swap #47710

Merged
merged 3 commits into from
Sep 3, 2024
Merged

[install-kubeadm]: Update instructions to disable or tolerate swap #47710

merged 3 commits into from
Sep 3, 2024

Conversation

iholder101
Copy link
Contributor

@iholder101 iholder101 commented Aug 28, 2024 •
edited
Loading

Description

Addresses kubernetes/kubeadm#2563 (comment).

Updates the instructions regarding swap configuration:

  • Outlines the different options more clearly: either disable swap or tolerate it.
  • Improves description of how to tolerate swap.

Issue

Closes: kubernetes/kubeadm#2563
Addresses kubernetes/kubeadm#2563 (comment).

Changes overview

image

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 28, 2024
@k8s-ci-robot k8s-ci-robot requested a review from sftim August 28, 2024 08:47
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Aug 28, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 28, 2024
@iholder101
Copy link
Contributor Author

/cc @pacoxu @neolit123

@netlify Netlify
Copy link

netlify bot commented Aug 28, 2024 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 3b09c30
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/66d042b1b6f1d4000818bdd1
😎 Deploy Preview https://deploy-preview-47710--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

neolit123
neolit123 reviewed Aug 28, 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.

is this change specific to v1.32?
if so the change should be for the dev-1.32 branch. not main.

content/en/docs/setup/production-environment/tools/kubeadm/install-kubeadm.md Outdated Show resolved Hide resolved
content/en/docs/setup/production-environment/tools/kubeadm/install-kubeadm.md Outdated Show resolved Hide resolved
@iholder101 iholder101 force-pushed the install-kubeadm/disable-swap branch from 6ca8520 to 0d9289b Compare August 28, 2024 11:04
@iholder101
Copy link
Contributor Author

is this change specific to v1.32?
if so the change should be for the dev-1.32 branch. not main.

Nope, this is relevant from 1.30 and onwards 👍

@sftim
Copy link
Contributor

sftim commented Aug 28, 2024

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Aug 28, 2024
@sftim
Copy link
Contributor

sftim commented Aug 28, 2024

/sig cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 28, 2024
sftim
sftim reviewed Aug 28, 2024
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Overall, I don't think that turning swap off is a prerequisite any more; rather than list that as a prerequisite, let's have a new heading about Linux nodes and swap where we explain how to either check things are right, or advise on what to change.

(I'd reiterate Linux here even though Linux is a prerequisite, because Windows nodes have supported running with swap active for many years).

content/en/docs/setup/production-environment/tools/kubeadm/install-kubeadm.md Outdated Show resolved Hide resolved
content/en/docs/setup/production-environment/tools/kubeadm/install-kubeadm.md Outdated Show resolved Hide resolved
@pacoxu
Copy link
Member

pacoxu commented Aug 29, 2024

Just find that kernel requirement of kubernetes/kubernetes@2a174d0 is not mentioned in doc.

pacoxu
pacoxu approved these changes Aug 29, 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.

LGTM

@iholder101
Copy link
Contributor Author

Just find that kernel requirement of kubernetes/kubernetes@2a174d0 is not mentioned in doc.

Thanks @pacoxu!

Yeah I've looked at this document, great job!
I'm not sure though if there's anything that needs to be added there. Perhaps I can better describe the implications of not being able to use noswap?

In any case, this is a bit out of scope for this PR :) do you mind discussing this offline or elsewhere?

@iholder101 iholder101 force-pushed the install-kubeadm/disable-swap branch from 0d9289b to a74e030 Compare August 29, 2024 09:43
@iholder101
minor gramattical fixes
42dd7e5 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101
Update instructions to disable or tolerate swap
3b95b00 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101
Move swap into a its own header
3b09c30 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101 iholder101 force-pushed the install-kubeadm/disable-swap branch from a74e030 to 3b09c30 Compare August 29, 2024 09:43
@pacoxu
Copy link
Member

pacoxu commented Aug 29, 2024

In any case, this is a bit out of scope for this PR :) do you mind discussing this offline or elsewhere?

Agree.

The scope and change of this PR LGTM.

neolit123
neolit123 reviewed Aug 29, 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
thanks

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

LGTM label has been added.

Git tree hash: 722a7459fed78e674361dedfc90c3c3fe5b21217

@reylejano
Copy link
Member

Swap is not required to be turned off any more. This change makes sense
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pacoxu, reylejano

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 Sep 3, 2024
@k8s-ci-robot k8s-ci-robot merged commit dc9aeb5 into kubernetes:main Sep 3, 2024
6 checks passed
@neolit123 neolit123 mentioned this pull request Sep 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@neolit123 neolit123 neolit123 left review comments

@pacoxu pacoxu pacoxu approved these changes

@shannonxtreme shannonxtreme Awaiting requested review from shannonxtreme

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. language/en Issues or PRs related to English language 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. sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NodeSwap feature supports in kubeadm
6 participants
@iholder101 @sftim @pacoxu @k8s-ci-robot @reylejano @neolit123