logrotate service enhancements #159187

martinetd · 2022-02-11T09:27:34Z

Motivation for this change

There are quite a few independent changes here, if anything generates a lot of discussion I'll be happy to drop any single patch and split it into a different PR, but it's all related overall.

nixos/logrotate: enable multiple paths per entry #152223 added wtmp/btmp logrotate rules, but since the files belong with systemd the rule probably ought to be moved there.
in nixos/logrotate: enable multiple paths per entry #152223 we also talked about enabling logrotate by default: I think it makes sense, if some rules are enabled then we probably want the service to move. I've made the default value of logrotate.enable be true if any path is defined, so it's not needlessly enabled.
while I'm at it I've copied the logrotate rule I use for nginx in services.nginx, that service creates logs and probably should take care of rotating them. Does this warrant a new option? it's always possible to disable the rule with services.logrotate.paths."/var/log/nginx/*.log".enable = false but that's more annoying to type than e.g. services.nginx.rotateLogs = false (if we add such a flag, I'd say we need to come up with a convention for other services to all use the same attribute, and also add it to systemd for wtmp/btmp -- I think it'd make sense and will be happy to gate this with a mkIf)
I tried using services.logrotate.extraConfig but it appends lines, and global options only affect paths following them so options were ignored. I've moved extraConfig first, and ~~made use of it instead of paths default extraConfig~~ moved notifempty/missingok to be similar to it as I couldn't figure how to append lines by default (setting the default value would remove lines if set elsewhere, which isn't what the previous path extraConfig was doing).
lastly I've added a simple test, mostly to make sure I didn't screw up the syntax of the enable statement and for manual testing, happy to remove it if it's not deemed useful. I know of passthru.tests for packages, is there an equivalent for nixos modules?

cc @ju1m @aanderse @ryantm who participated in previous logrotate PR
cc @Mic92 who last touched the nginx module

Things done

Built on platform(s)
- x86_64-linux
Tested, as applicable:
- NixOS test(s) (look inside nixos/tests)
- and/or package tests
- or, for functions and "core" functionality, tests in lib/tests or pkgs/test
- made sure NixOS tests are linked to the relevant packages
Fits CONTRIBUTING.md.

nixos/modules/services/logging/logrotate.nix

nixos/modules/services/web-servers/nginx/default.nix

dasJ · 2022-02-11T11:27:00Z

nixos/modules/services/logging/logrotate.nix

-  configFile = pkgs.writeText "logrotate.conf" (concatStringsSep "\n" ((map mkConf paths) ++ [ cfg.extraConfig ]));
+  configFile = pkgs.writeText "logrotate.conf" (
+    concatStringsSep "\n" (
+      [ "missingok" "notifempty" cfg.extraConfig ] ++ (map mkConf paths)


Good to know these are set by default 👀

aanderse

This PR is great 👍 Thanks for doing this!

nixos/modules/services/web-servers/nginx/default.nix

nixos/modules/system/boot/systemd.nix

wtmp and btmp are created by systemd, so the rules are more appropriate there. They can be disabled explicitly with something like services.ogrotate.paths = { "/var/log/btmp".enable = false; "/var/log/wtmp".enable = false; }; if required.

logrotate global options only affect rules following them - as such, services.logrotate.extraConfig being added last makes the option only useful for adding new paths but not for setting global options (e.g. 'dateext' so all logs are rotate with a date suffix). Moving this first solves this problem, and we can then use this instead of default paths config to append missingok/notifempty.

aanderse

LGTM 👍

Merge?

ju1m · 2022-02-12T13:23:16Z

@martinetd thanks for this PR :)
AFAICS, you can (some say should) add nixos tests to passthru.tests:

{ passthru.tests = { nixos-logrotate = nixosTests.logrotate; }; }

And then build them with nix -L build -f . logrotate.tests, or @ofborg build logrotate.passthru.tests, or nixpkgs-update.
Arguably, you might also want to add

(onFullSupported "nixos.tests.logrotate")

to the Release-critical builds for the NixOS channel to have it tested by hydra.nixos.org.

make sure the service is enabled by default and works.

martinetd · 2022-02-12T22:54:59Z

@ju1m thanks for the review!
I didn't think of adding the test to the logrotate package, that makes sense. I've amended the last commit adding tests to also add the passthru.tests, and confirmed logrotate.tests runs these.

@aanderse There's one last question pending if we want a convention like services.nginx.rotateLogs flag (could just be an alias to services.logrotate.paths.nginx.enable), but I guess that can be discussed again elsewhere if the need arises, I'm happy with the current state of the PR. It can wait a few more days though, let's give people some time to have a look :)

nixos-discourse · 2022-02-12T23:32:05Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/727

martinetd · 2022-02-23T05:28:43Z

It's been 10 days so I guess we've given it enough time, @aanderse would you like to merge this?
Thanks!

ncfavier · 2022-02-25T16:30:44Z

@ofborg test switchTest

ncfavier · 2022-02-25T16:32:05Z

Damn it. Anyway this breaks the test with

The haystack that will cause the following exception is:
---
activating the configuration...
setting up /etc...
setting up tmpfiles
restarting the following units: test.service
the following new units were started: logrotate.service
---
Test "unit file parser" failed with error: "Unexpected string 'the following new units were started:' was found"

martinetd · 2022-02-26T06:28:28Z

Thanks for the report, I think the problem is that enabling logrotate enables the service when it should only be enabling the timer; looking at the machine logs starts logrotate all the time

ius · 2022-02-26T18:30:20Z

I've made the default value of logrotate.enable be true if any path is defined, so it's not needlessly enabled.

Unless I'm mistaken, it is effectively on by default due to the btmp/wtmp paths defined for systemd?

As described in more detail in #162001 this change adds 60 MB to installation media closures as well as quite some build time (note the root cause isn't this PR per se).

Although the size problem can be remediated in multiple ways, in context of this PR I can't help but note:

Rotating system login records (btmp/wtmp) only makes sense if they've grown prohibitively large. On many machines, this doesn't occur that quickly. At least not on mine. If the average use case benefit is only rotating btmp/wtmp, then I'd prefer not to add 60 MB to rotate my 300 kB wtmp ;)
Needlessly rotating system login records may destroy valuable audit logging on multi user machines.

As for item 2, a quick check of Arch / Debian / CentOS shows:

CentOS does not rotate btmp/wtmp by default
Arch and Debian use the same interval, but specify minsize 1M on wtmp to not rotate succesful logins unless needed.

I suggest at least following up with a PR to set minsize on wtmp, less sure about the first issue (#162001)

martinetd · 2022-02-26T22:19:39Z

Unless I'm mistaken, it is effectively on by default due to the btmp/wtmp paths defined for systemd?

Indeed. The goal is also to integrate logrotate more closely with other services, in my case the real reason for wanting to enable it was nginx logs growing out of control -- services producing logs should take care of cleaning them up.
In the default system that does means btmp/wtmp though as you've pointed out.

As described in more detail in #162001 this change adds 60 MB to installation media closures as well as quite some build time (note the root cause isn't this PR per se).

Thanks for bringing this up, I didn't realize logrotate was effectively this big... I've never used the mail sending feature of logrotate myself so didn't even know it had one :/
I'll reply on that ticket separately, I think it deserves some discussion on both regards (install media and not install media)

wmp/btmp

oh, I was looking at a very old debian for reference which doesn't have the minsize 1M rule. I think it makes sense to add. FWIW fedora does the same (it's rotated in /etc/logrotate.d/wtmp or btmp owned by logrotate package).
I've opened #162022 for this.

align with upstream logrotate which added the minsize rule at some point. This avoids needlessly rotating the files too often as brought up in NixOS#159187 (comment)

Artturin · 2022-02-27T00:11:57Z

nixos/modules/system/boot/systemd.nix

+    services.logrotate.paths = {
+      "/var/log/btmp" = mapAttrs (_: mkDefault) {
+        frequency = "monthly";
+        keep = 1;
+        extraConfig = ''
+          create 0660 root ${config.users.groups.utmp.name}
+        '';
+      };
+      "/var/log/wtmp" = mapAttrs (_: mkDefault) {
+        frequency = "monthly";
+        keep = 1;
+        extraConfig = ''
+          create 0664 root ${config.users.groups.utmp.name}
+        '';
+      };
+    };


imo these should only be enabled if logrotate is enabled otherwise this just pulls in unnecessary dependencies

i don't have mailutils or guile on my system and i have lots of packages and modules enabled

We can't (afaik) make logrotate enable depend on the presence of any path defined and make these (adding paths) depend on logrotate.enable as that'd bring a circular dependency. It could probably be possible by kludging this but the fact that logrotate is so big is a problem in itself that should be fixed, and we shouldn't use the size as a reason to disable this in my opinion. It could make sense to minimize the number of moving parts on minimal systems, but I don't think that's what you are arguing for?

logrotate itself is tiny (121kB), please see my suggestion in #162001 for mailutils so we won't bring it in needlessly and let's continue the discussion there.
This is what it looks like after moving the mailutils dependency from the package's to an optional service option:

$ nix path-info -rSsh ./result /nix/store/8ckxc8biqqfdwyhr0w70jgrcb4h7a4y5-libunistring-0.9.10 1.6M 1.6M /nix/store/w2id1hwv4vv7hvp4slgsyrydrjbfqdxc-libidn2-2.3.2 254.7K 1.8M /nix/store/4s21k8k7p1mfik0b33r2spq5hq7774k1-glibc-2.33-108 29.9M 31.7M /nix/store/9igigiz42g7w2i605dd5k1spxy9nkf48-attr-2.5.1 78.7K 31.8M /nix/store/1xjldbdb814annhpmcwz7h6z6y3lay15-acl-2.3.1 108.9K 31.9M /nix/store/ix20p57nyz6hdcfbnbhxwsmdh8a2r6rh-gzip-1.11 148.1K 31.8M /nix/store/zwrw0fv1n4hdk017xqxy6s80kmn6g52q-popt-1.18 168.7K 31.8M /nix/store/bifjan2fvp6k9p3jk4fyv1bykynsj623-logrotate-3.19.0 118.5K 32.3M

martinetd · 2022-03-12T23:19:13Z

OT: extra-container does not work for me in unstable, looks like due to logrotate: move wtmp/btmp rules to systemd · NixOS/nixpkgs@9917af7 · GitHub.

@cmm thanks for pointing this out!

Interesting side effect... It looks like extra-container/eval-config.nix only uses a subset of modules which doesn't include the user/groups . . . but even adding the user/group just fails with "services.logrotate doesn't exist" next, so it'd need some kind of knob, or moving back to logrotate.
This move was mostly a cosmetic change so I don't mind either way, what do you think?

cmm · 2022-03-13T17:26:42Z

OT: extra-container does not work for me in unstable, looks like due to logrotate: move wtmp/btmp rules to systemd · NixOS/nixpkgs@9917af7 · GitHub.

@cmm thanks for pointing this out!

Interesting side effect... It looks like extra-container/eval-config.nix only uses a subset of modules which doesn't include the user/groups . . . but even adding the user/group just fails with "services.logrotate doesn't exist" next, so it'd need some kind of knob, or moving back to logrotate. This move was mostly a cosmetic change so I don't mind either way, what do you think?

I just cursorily tried to play with a neat tool and it didn't work, no idea beyond that. perhaps the author (that'd be @erikarvstedt) has some?

martinetd requested a review from dasJ as a code owner February 11, 2022 09:27

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 Feb 11, 2022

martinetd force-pushed the logrotate branch 2 times, most recently from 2ca7fc4 to 6bef29d Compare February 11, 2022 09:42

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 11, 2022

dasJ requested a review from ajs124 February 11, 2022 11:22

dasJ requested changes Feb 11, 2022

View reviewed changes

aanderse requested changes Feb 11, 2022

View reviewed changes

nixos/modules/services/web-servers/nginx/default.nix Outdated Show resolved Hide resolved

nixos/modules/system/boot/systemd.nix Outdated Show resolved Hide resolved

nixos/modules/system/boot/systemd.nix Outdated Show resolved Hide resolved

logrotate: move wtmp/btmp rules to systemd

9917af7

wtmp and btmp are created by systemd, so the rules are more appropriate there. They can be disabled explicitly with something like services.ogrotate.paths = { "/var/log/btmp".enable = false; "/var/log/wtmp".enable = false; }; if required.

martinetd force-pushed the logrotate branch from 6bef29d to 2b5ce0e Compare February 11, 2022 11:53

martinetd added 3 commits February 11, 2022 21:07

logrotate: default to enable if any rule is active

61c70db

nginx: add logrotate rule for nginx logs

c7618fb

martinetd force-pushed the logrotate branch from 2b5ce0e to 7450fb6 Compare February 11, 2022 12:07

github-actions bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation labels Feb 11, 2022

martinetd requested review from dasJ and aanderse February 12, 2022 01:16

aanderse approved these changes Feb 12, 2022

View reviewed changes

ju1m mentioned this pull request Feb 12, 2022

nixos/tests: add step-ca test #145695

Merged

12 tasks

nixos tests: add logrotate test

a05f1c9

make sure the service is enabled by default and works.

martinetd force-pushed the logrotate branch from 7450fb6 to a05f1c9 Compare February 12, 2022 22:30

dasJ merged commit e5823f7 into NixOS:master Feb 23, 2022

ncfavier mentioned this pull request Feb 25, 2022

nixos/tests: fix switchTest #161859

Closed

ius mentioned this pull request Feb 26, 2022

Logrotate adds bloat to installation media closure #162001

Closed

martinetd mentioned this pull request Feb 26, 2022

logrotate/systemd: add 'minsize = 1M' to wtmp/btmp rotation #162022

Merged

3 tasks

Artturin reviewed Feb 27, 2022

View reviewed changes

martinetd mentioned this pull request Feb 28, 2022

logrotate: update to freeform #162063

Merged

5 tasks

martinetd mentioned this pull request Mar 15, 2022

extra-container broke on nixos-unstable erikarvstedt/extra-container#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logrotate service enhancements #159187

logrotate service enhancements #159187

martinetd commented Feb 11, 2022 •

edited

Loading

dasJ Feb 11, 2022

aanderse left a comment

aanderse left a comment

ju1m commented Feb 12, 2022 •

edited

Loading

martinetd commented Feb 12, 2022

nixos-discourse commented Feb 12, 2022

martinetd commented Feb 23, 2022

ncfavier commented Feb 25, 2022

ncfavier commented Feb 25, 2022

martinetd commented Feb 26, 2022

ius commented Feb 26, 2022 •

edited

Loading

martinetd commented Feb 26, 2022

Artturin Feb 27, 2022

martinetd Feb 27, 2022 •

edited

Loading

martinetd commented Mar 12, 2022

cmm commented Mar 13, 2022

logrotate service enhancements #159187

logrotate service enhancements #159187

Conversation

martinetd commented Feb 11, 2022 • edited Loading

Motivation for this change

Things done

dasJ Feb 11, 2022

Choose a reason for hiding this comment

aanderse left a comment

Choose a reason for hiding this comment

aanderse left a comment

Choose a reason for hiding this comment

ju1m commented Feb 12, 2022 • edited Loading

martinetd commented Feb 12, 2022

nixos-discourse commented Feb 12, 2022

martinetd commented Feb 23, 2022

ncfavier commented Feb 25, 2022

ncfavier commented Feb 25, 2022

martinetd commented Feb 26, 2022

ius commented Feb 26, 2022 • edited Loading

martinetd commented Feb 26, 2022

Artturin Feb 27, 2022

Choose a reason for hiding this comment

martinetd Feb 27, 2022 • edited Loading

Choose a reason for hiding this comment

martinetd commented Mar 12, 2022

cmm commented Mar 13, 2022

martinetd commented Feb 11, 2022 •

edited

Loading

ju1m commented Feb 12, 2022 •

edited

Loading

ius commented Feb 26, 2022 •

edited

Loading

martinetd Feb 27, 2022 •

edited

Loading