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

Use systemd for disabling swap when it's used #10587

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Use systemd for disabling swap when it's used #10587

merged 3 commits into from
Dec 7, 2023

Conversation

VannTen
Copy link
Contributor

@VannTen VannTen commented Nov 3, 2023 •
edited
Loading

  • Disable swap with systemd when it's used
  • Unconditionally disable swap
  • Don't explicitly disable swapOnZram

What type of PR is this?
/kind bug

What this PR does / why we need it:

This relies on systemd instead of fstab to disable swap + some cleanups and simplifications.
On systemd system, mount and swap configuration is achieved by .mount and .swap units, and fstab is only one source for those (they can be manually created or generated from fstab of a gpt partition table for example). Hence it's more appropriate the swap.target, on which all of theses mechanisms should depend.

Which issue(s) this PR fixes:

Fixes #10581

Does this PR introduce a user-facing change?:

Swap is now disabled using systemd (mask of swap.target)

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 3, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 3, 2023
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 3, 2023
@VannTen VannTen force-pushed the fix/swap_no_fstab branch 2 times, most recently from af155e6 to 0a9e92e Compare November 3, 2023 10:33
@VannTen
Copy link
Contributor Author

VannTen commented Nov 7, 2023

Hum, actually, does kubespray support non-systemd distributions ? If not I can just inconditionnaly use the masking and remove the fstab stuff.

@yankay
Copy link
Member

yankay commented Nov 15, 2023

/ok-to-test

@k8s-ci-robot 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 Nov 15, 2023
@VannTen
Mask systemd swap.target do disable swap
58011fb 
This is a more generic way to disable swap, since it pulls .swap units
in systemd distributions; fstab is only one way to generate .swap units.
@VannTen
Unconditionally disable swap
55b86a5 
We only care to disable it (the "swapon" registered variable is not used
anywhere else.
This allows to get rid of the ignore_errors, since this was added
because swapon.stdout does not exist in check_mode (see issue kubernetes-sigs#6642).
@VannTen
Don't explicitly disable swapOnZram
25bbfbd 
We're already masking the swap.target, which would pull the zram unit,
hence no need to handle zram-generator specifically.
@VannTen
Copy link
Contributor Author

VannTen commented Nov 27, 2023

/unassign @jayonlau

@VannTen
Copy link
Contributor Author

VannTen commented Nov 27, 2023 •
edited
Loading

/assign @cyclinder
Unassign does not seem to work for reviewers ? The linked doc from prow suggest it should 🤔

Apparently this does not work either :/ Oh well

MrFreezeex
MrFreezeex approved these changes Dec 7, 2023
View reviewed changes
Copy link
Member

@MrFreezeex MrFreezeex left a comment

Choose a reason for hiding this comment

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

Thanks for this nice cleanup! :D
/lgtm
/ok-to-test

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2023
@VannTen
Copy link
Contributor Author

VannTen commented Dec 7, 2023

/assign @mzaian

mzaian
mzaian approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@mzaian mzaian left a comment

Choose a reason for hiding this comment

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

Neat! @VannTen

Thanks you!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrFreezeex, mzaian, VannTen

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 Dec 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 2c3ea84 into kubernetes-sigs:master Dec 7, 2023
63 checks passed
@yankay yankay mentioned this pull request Dec 15, 2023
Closed
@0ekk 0ekk mentioned this pull request Dec 22, 2023
Closed
@yankay yankay mentioned this pull request Dec 28, 2023
Merged
pedro-peter pushed a commit to pedro-peter/kubespray that referenced this pull request May 8, 2024
@VannTen @pedromcpedro
Use systemd for disabling swap when it's used (kubernetes-sigs#10587)
99967dd 
* Mask systemd swap.target do disable swap

This is a more generic way to disable swap, since it pulls .swap units
in systemd distributions; fstab is only one way to generate .swap units.

* Unconditionally disable swap

We only care to disable it (the "swapon" registered variable is not used
anywhere else.
This allows to get rid of the ignore_errors, since this was added
because swapon.stdout does not exist in check_mode (see issue kubernetes-sigs#6642).

* Don't explicitly disable swapOnZram

We're already masking the swap.target, which would pull the zram unit,
hence no need to handle zram-generator specifically.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrFreezeex MrFreezeex MrFreezeex approved these changes

@mzaian mzaian mzaian approved these changes

@ErikJiang ErikJiang Awaiting requested review from ErikJiang

@jayonlau jayonlau Awaiting requested review from jayonlau

Assignees

@mzaian mzaian

@MrFreezeex MrFreezeex

@cyclinder cyclinder

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

preinstall task tries to remove entry from non-existing fstab on fedora core
6 participants
@VannTen @k8s-ci-robot @yankay @mzaian @MrFreezeex @cyclinder