Replace disable_swap variable with kubelet_fail_swap_on #10036

Manuelraa · 2023-04-30T12:13:00Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Replace disable_swap variable with kubelet_fail_swap_on.
Swap will no longer be disabled if kubelet_fail_swap_on: false.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:

Replace `disable_swap` variable with `kubelet_fail_swap_on`

linux-foundation-easycla · 2023-04-30T12:13:05Z

The committers listed above are authorized under a signed CLA.

✅ login: Manuelraa / name: Manuelraa (e97f346)

k8s-ci-robot · 2023-04-30T12:13:08Z

Welcome @Manuelraa!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-04-30T12:13:08Z

Hi @Manuelraa. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

MrFreezeex

Hi thanks for the contribution!

I checked the code around accepting swap and actually there is already a "support" for that with disable_swap although it feel kinda weird because you have to combine it with kubelet_fail_swap_on.

I think we should just get rid of disable_swap and replace it by kubelet_swap_on IMO. We could also keep those two variables separate but I don't see any occurrence where it would make a lot of sense, so I don't think that would be necessary...

Also you would need to cleanup the kubelet_fail_swap_on | default(true) replacing them by a simple kubelet_fail_swap_on as this would be defined in kubespray-defaults/defaults/main.yaml (if disable_swap get replaced)

Manuelraa · 2023-05-01T12:49:13Z

Oh sorry I missed disable_swap.

Would you rather have both disable_swap and kubelet_fail_swap_on replaced with a simple kubelet_swap_on or just disable_swap?

MrFreezeex · 2023-05-01T13:05:26Z

Oh sorry I missed disable_swap.

Would you rather have both disable_swap and kubelet_fail_swap_on replaced with a simple kubelet_swap_on or just disable_swap?

IMO keeping only kubelet_fail_swap_on should do for now.

yankay · 2023-05-04T02:21:22Z

/ok-to-test

floryut

Double approval 😱

floryut

@Manuelraa Thank you, looks good to me 👍

Manuelraa · 2023-05-09T09:56:27Z

@Manuelraa Thank you, looks good to me 👍

Thanks. I still want to implement suggestion of @MrFreezeex #10036 (comment)

At least I can't think of a usecase there disable_swap and kubelet_fail_swap_on would be required to set independently from each other.

Haven't come around yet run the test molecule on my hacky local dev environment. Bit of libvirt struggle.

MrFreezeex · 2023-05-09T10:05:37Z

Haven't come around yet run the test molecule on my hacky local dev environment. Bit of libvirt struggle.

Feel free to let the CI does that for you btw (never done it myself tbh, I am usually using the CI to test or testing manually on a random test cluster beforehand) or even add this variable in a test in kubespray/tests/files not sure we really need a new test adding this to one of the job in deploy-part2 should probable do I guess.

Manuelraa · 2023-05-09T21:00:15Z

roles/kubernetes/node/defaults/main.yml

@@ -20,9 +20,6 @@ kubelet_kubelet_cgroups: "/{{ kube_service_cgroups }}/kubelet.service"
 kubelet_runtime_cgroups_cgroupfs: "/system.slice/{{ container_manager }}.service"
 kubelet_kubelet_cgroups_cgroupfs: "/system.slice/kubelet.service"

-### fail with swap on (default true)
-kubelet_fail_swap_on: true


Hope this is fine. As far as I could see kubespray-defaults is imported always before kubernetes/node in the playbooks.

MrFreezeex

Thanks for this nice cleanup :D (the CI is broken atm, not your fault btw)
/lgtm

MrFreezeex · 2023-05-09T21:19:18Z

roles/kubernetes/preinstall/tasks/0010-swapoff.yml

@@ -18,12 +18,10 @@
  command: /sbin/swapoff -a
  when:
    - swapon.stdout
-    - kubelet_fail_swap_on | default(True)


good catch 👍

k8s-ci-robot · 2023-05-09T21:20:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, Manuelraa, MrFreezeex

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:

~~OWNERS~~ [floryut]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Manuelraa · 2023-05-09T21:28:50Z

Thanks you too. In case there is still something to change/test let me know.

// I also updated the PR title and description to match the change

MrFreezeex · 2023-05-11T11:40:55Z

Hi @Manuelraa the CI should be fixed now, could you rebase your branch please? 🙏

MrFreezeex · 2023-05-11T15:53:18Z

/lgtm

…#10036)

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 30, 2023

k8s-ci-robot requested review from cyclinder and MrFreezeex April 30, 2023 12:13

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 30, 2023

MrFreezeex suggested changes Apr 30, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 4, 2023

floryut approved these changes May 9, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2023

Manuelraa force-pushed the fix-swap-wrongly-removed-from-fstab branch from e97f346 to bce9a4d Compare May 9, 2023 20:57

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 9, 2023

Manuelraa commented May 9, 2023

View reviewed changes

MrFreezeex approved these changes May 9, 2023

View reviewed changes

k8s-ci-robot assigned MrFreezeex May 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2023

Manuelraa changed the title ~~Do not remove swap from fstab if swap is allowed~~ Replace disable_swap variable with kubelet_fail_swap_on May 9, 2023

Replace swap vars with single kubelet_fail_swap_on

7cfba40

Manuelraa force-pushed the fix-swap-wrongly-removed-from-fstab branch from bce9a4d to 7cfba40 Compare May 11, 2023 15:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2023

k8s-ci-robot merged commit 2b75552 into kubernetes-sigs:master May 11, 2023
62 checks passed

yankay mentioned this pull request May 15, 2023

Release Proposal v2.22 #10075

Closed

floryut added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels May 15, 2023

pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024

Replace swap vars with single kubelet_fail_swap_on (kubernetes-sigs…

56c3153

…#10036)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace disable_swap variable with kubelet_fail_swap_on #10036

Replace disable_swap variable with kubelet_fail_swap_on #10036

Manuelraa commented Apr 30, 2023 •

edited

linux-foundation-easycla bot commented Apr 30, 2023 •

edited

k8s-ci-robot commented Apr 30, 2023

k8s-ci-robot commented Apr 30, 2023

MrFreezeex left a comment •

edited

Manuelraa commented May 1, 2023

MrFreezeex commented May 1, 2023

yankay commented May 4, 2023

floryut left a comment •

edited

floryut left a comment

Manuelraa commented May 9, 2023

MrFreezeex commented May 9, 2023

Manuelraa May 9, 2023

MrFreezeex left a comment

MrFreezeex May 9, 2023

k8s-ci-robot commented May 9, 2023

Manuelraa commented May 9, 2023

MrFreezeex commented May 11, 2023

MrFreezeex commented May 11, 2023

Replace disable_swap variable with kubelet_fail_swap_on #10036

Replace disable_swap variable with kubelet_fail_swap_on #10036

Conversation

Manuelraa commented Apr 30, 2023 • edited

linux-foundation-easycla bot commented Apr 30, 2023 • edited

k8s-ci-robot commented Apr 30, 2023

k8s-ci-robot commented Apr 30, 2023

MrFreezeex left a comment • edited

Choose a reason for hiding this comment

Manuelraa commented May 1, 2023

MrFreezeex commented May 1, 2023

yankay commented May 4, 2023

floryut left a comment • edited

Choose a reason for hiding this comment

floryut left a comment

Choose a reason for hiding this comment

Manuelraa commented May 9, 2023

MrFreezeex commented May 9, 2023

Manuelraa May 9, 2023

Choose a reason for hiding this comment

MrFreezeex left a comment

Choose a reason for hiding this comment

MrFreezeex May 9, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented May 9, 2023

Manuelraa commented May 9, 2023

MrFreezeex commented May 11, 2023

MrFreezeex commented May 11, 2023

Manuelraa commented Apr 30, 2023 •

edited

linux-foundation-easycla bot commented Apr 30, 2023 •

edited

MrFreezeex left a comment •

edited

floryut left a comment •

edited