Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

nixos/earlyoom: bring the module up to date #163663

Merged
merged 4 commits into from
Mar 24, 2022

Conversation

ncfavier
Copy link
Member

@ncfavier ncfavier commented Mar 11, 2022

Removes deprecated option ignoreOOMScoreAdjust, introduces killHook as a replacement for notificationsCommand, and adds an extraArgs option for things not covered by the module.

Fixes #132783
Fixes #83504

Untested.

My example killHook script sucks, suggestions welcome.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 11, 2022
@ncfavier
Copy link
Member Author

cc @deliciouslytyped as I can't add you to reviewers for some reason

@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 Mar 11, 2022
Copy link
Member

@Profpatsch Profpatsch left a comment

Choose a reason for hiding this comment

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

Tried on my very simple configuration

    services.earlyoom = {
      enable = true;
      freeMemThreshold = 5; # <5% free
    };

and it still seems to work.

@ncfavier ncfavier force-pushed the earlyoom-update-module branch from a123466 to 17ece32 Compare March 16, 2022 13:05
@ncfavier
Copy link
Member Author

Rebased after #130003, cc @peterhoeg

@peterhoeg
Copy link
Member

Haven't tested it, but the changes look good to me. killHook sample is fine. How about a super simple nixos test that simply verifies that it launches?

@ncfavier
Copy link
Member Author

Sounds like a good opportunity to write my first NixOS test

@ncfavier ncfavier force-pushed the earlyoom-update-module branch from 17ece32 to a0e9c15 Compare March 17, 2022 09:10
@ncfavier

This comment was marked as outdated.

@ncfavier

This comment was marked as outdated.

@ncfavier ncfavier force-pushed the earlyoom-update-module branch from a0e9c15 to cbf41e6 Compare March 17, 2022 10:11
@ncfavier ncfavier requested a review from peterhoeg March 17, 2022 10:36
@ncfavier
Copy link
Member Author

ncfavier commented Mar 21, 2022

Relaxed the type of free{Mem,Swap}Threshold to fix #83504, and added reportInterval to avoid log flooding (the interval defaults to one second when not using upstream's config file... rfjakob/earlyoom#177 (comment)).

Tested locally, works fine.
@ofborg test earlyoom

@peterhoeg
Copy link
Member

I don't like the string bit at all as we have no way of verifying that we're passing a sane value through.

Instead add 2 new integer options for the corresponding kill values and use those to generate the proper string.

Removes deprecated option `ignoreOOMScoreAdjust`, introduces `killHook`
as a replacement for `notificationsCommand`, and adds an `extraArgs`
option for things not covered by the module.
Allows setting the interval for logging a memory report. Defaults to
3600 following upstream
     (https://github.com/rfjakob/earlyoom/blob/master/earlyoom.default#L5)
to avoid flooding logs.
@ncfavier ncfavier force-pushed the earlyoom-update-module branch from 63ac78d to d2119fb Compare March 22, 2022 13:42
@ncfavier
Copy link
Member Author

You're right, I was lazy.

Tested again with the new options.

@peterhoeg
Copy link
Member

@GrahamcOfBorg build earlyoom.passthru.tests

@peterhoeg peterhoeg merged commit a8296e7 into NixOS:master Mar 24, 2022
@ncfavier ncfavier deleted the earlyoom-update-module branch March 24, 2022 08:05
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
3 participants