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.

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

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

Merged

Conversation

iholder101
Copy link
Contributor

@iholder101 iholder101 commented Aug 28, 2024

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

Copy link

netlify bot commented Aug 28, 2024

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.

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.

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

@pacoxu
Copy link
Member

pacoxu commented Aug 29, 2024

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

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?

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Development

Successfully merging this pull request may close these issues.

NodeSwap feature supports in kubeadm
6 participants