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

nixos/swapspace: init module #348588

Merged
merged 2 commits into from
Nov 6, 2024
Merged

Conversation

phanirithvij
Copy link
Member

@phanirithvij phanirithvij commented Oct 14, 2024

Adds a swapspace nixos module.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

cc @Luflosi

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 14, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 14, 2024
Copy link
Contributor

@Luflosi Luflosi left a comment

Choose a reason for hiding this comment

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

This already looks very good and I only have a couple small improvement suggestions.

nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
@Luflosi
Copy link
Contributor

Luflosi commented Oct 14, 2024

There was a previous attempt in #88093 but the original author seems to have lost interest.
I compared the two and think this PR is a strict improvement over the previous one.

@phanirithvij phanirithvij force-pushed the swapspace-module branch 4 times, most recently from c8a1ea3 to 24fb007 Compare October 15, 2024 08:09
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Oct 15, 2024
Copy link
Contributor

@Luflosi Luflosi left a comment

Choose a reason for hiding this comment

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

I think extraArgs should be a list of strings instead of a single string. This seems to be what most NixOS modules do. It allows setting different options in different places without overriding previous ones.

nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Outdated Show resolved Hide resolved
nixos/modules/services/system/swapspace.nix Show resolved Hide resolved
@Luflosi
Copy link
Contributor

Luflosi commented Oct 19, 2024

Your latest changes removed

      after = [
        "local-fs.target"
        "swap.target"
      ];
      requires = [
        "local-fs.target"
        "swap.target"
      ];
      wantedBy = [ "multi-user.target" ];
      documentation = [ "man:swapspace(8)" ];
      description = "Swapspace, a dynamic swap space manager";

While it is true, that those changes are in the upstream unit config and thus will be readable by systemd, the same is not true for Nix itself. Nix can't read the upstream unit config during evaluation. Having information such as description available for Nix to read, could allow tools to provide useful information about NixOS modules without having to compile or download any software. If I remember correctly, wantedBy = ... is even required for the unit to be installed but I could be wrong on that. Feel free to test.
As a rule of thumb, I remove everything from systemd.services.myCoolService.serviceConfig, which is already present in the upstream unit config, while leaving everything else in systemd.services.myCoolService as it is. Unfortunately I don't know how to check which values are actually used by other NixOS modules.

@phanirithvij phanirithvij force-pushed the swapspace-module branch 2 times, most recently from 13a56c1 to 8a0eb6e Compare October 19, 2024 16:26
@phanirithvij
Copy link
Member Author

phanirithvij commented Oct 19, 2024

Sorry for the force push spam, I have another branch in my fork to test with my system flake, mistakenly pushed to this a few times before testing.

I've re-added those I've removed, and also fixed serviceConfig.ExecStart https://discourse.nixos.org/t/clear-list-nix-syntax-or/20580

Copy link
Contributor

@Luflosi Luflosi left a comment

Choose a reason for hiding this comment

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

The code looks good. There is one small non-blocking nit-pick that can be safely ignored.

nixos/modules/services/system/swapspace.nix Show resolved Hide resolved
@Luflosi
Copy link
Contributor

Luflosi commented Oct 19, 2024

Sorry for the force push spam, I have another branch in my fork to test with my system flake, mistakenly pushed to this a few times before testing.

No worries.

and also fixed serviceConfig.ExecStart

Good catch! I forgot that this was needed.

@phanirithvij phanirithvij marked this pull request as ready for review October 20, 2024 03:24
@r-vdp
Copy link
Contributor

r-vdp commented Oct 26, 2024

That being said, I'm also not sure that we have the unit file in the right location in the derivation output. I thought they had to be in lib, I'm not sure if they are also picked up correctly from etc.

I just had a look at the source, and lib/ and etc/ are treated in the same way, so I think this is fine.

Copy link
Contributor

@r-vdp r-vdp left a comment

Choose a reason for hiding this comment

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

I didn't run the module, but the diff LGTM.

@r-vdp
Copy link
Contributor

r-vdp commented Oct 26, 2024

@phanirithvij maybe a nixos VM test would be useful? To check that everything starts correctly.

@wegank wegank added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Oct 26, 2024
@phanirithvij phanirithvij removed 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Oct 27, 2024
@r-vdp
Copy link
Contributor

r-vdp commented Oct 27, 2024

@ofborg test swapspace

@phanirithvij

This comment was marked as resolved.

@phanirithvij

This comment was marked as resolved.

@phanirithvij phanirithvij marked this pull request as draft October 27, 2024 13:56
phanirithvij and others added 2 commits October 30, 2024 22:41
Signed-off-by: phanirithvij <phanirithvij2000@gmail.com>
Co-authored-by: Luflosi <luflosi@luflosi.de>
Signed-off-by: phanirithvij <phanirithvij2000@gmail.com>
@phanirithvij phanirithvij marked this pull request as ready for review October 30, 2024 17:22
@phanirithvij
Copy link
Member Author

@ofborg test swapspace

@phanirithvij
Copy link
Member Author

phanirithvij commented Oct 30, 2024

Tested locally tons of times

nix-build -A nixosTests.swapspace; for i in {1..100}; do echo $i > run; nix-build -A nixosTests.swapspace --check || break; done

It is not flaky anymore on my x86_64-linux. And runs on aarch64-linux, So it is good to go.


The test can be improved later.

I've tried for a while to simplify the test but it was getting killed randomly due to oom (very flaky).
So if someone is able to write a better/leaner test, I think it can follow in a later PR.

Requesting review again @r-vdp @Luflosi @Pandapip1

@r-vdp r-vdp merged commit d06e176 into NixOS:master Nov 6, 2024
35 checks passed
@phanirithvij phanirithvij deleted the swapspace-module branch November 6, 2024 16:08
@2xsaiko 2xsaiko added the backport release-24.05 Backport PR automatically label Nov 7, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

Backport failed for release-24.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.05
git worktree add -d .worktree/backport-348588-to-release-24.05 origin/release-24.05
cd .worktree/backport-348588-to-release-24.05
git switch --create backport-348588-to-release-24.05
git cherry-pick -x 80ea320fe72bd16d9add8a34a6b6aa3880bb9f58 e4c898c8071c5333cc283620dd14a5638a09957e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: tests This PR has tests 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 backport release-24.05 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants