Navigation Menu

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

Jump to bottom

nixos/munin: New options (and some bugfixes) for service configuration #51980

Merged
merged 9 commits into from Feb 5, 2019
Merged

nixos/munin: New options (and some bugfixes) for service configuration #51980

merged 9 commits into from Feb 5, 2019

Conversation

ToxicFrog
Copy link
Contributor

Motivation for this change

The old Munin configuration had some outright bugs (wrong fonts used for graph generation, documentation links out of date) and was missing a lot of useful knobs. These changes fix those bugs, and add options for plugin configuration, loading extra plugins, disabling builtin plugins, and applying custom styles to the HTML that Munin generates.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ToxicFrog ToxicFrog mentioned this pull request Dec 14, 2018
Open
@ToxicFrog
Copy link
Contributor Author

On further testing, the extraAutoPlugins option doesn't work reliably due to the way Munin searches for plugins. I'll see about fixing that this evening.

@florianjacob
Copy link
Contributor

nixos/tests/munin.nix disables a plugin via ignore_file ^apc_nis$, it would make sense to switch that over to the new module option. Regarding previous ignore_file way vs. deleting the plugins like your option does, will that make any difference for munin? There is no other way for plugins to be enabled than linking them to /etc/munin/plugins, right?

Overall, this gives the munin the necessary portion of care, and shrinks my extraConfig to 1/8th. Did not test extraAutoPlugins, though.

@ToxicFrog ToxicFrog force-pushed the munin-plugins branch 2 times, most recently from b0c2e64 to 4a263ad Compare December 15, 2018 02:16
@ToxicFrog
Copy link
Contributor Author

I've updated tests/munin.nix to use the new disabledPlugins setting, and it seems to work.

@florianjacob
Copy link
Contributor

@GrahamcOfBorg build nixosTests.munin

@ToxicFrog
Copy link
Contributor Author

Made a small tweak to the CSS commit (so that the example code doesn't look hideous on warning/critical graphs) and added a new commit to bump the Munin version to the latest stable release (and fix a small bug in the build).

@FRidh
Copy link
Member

FRidh commented Dec 31, 2018

@GrahamcOfBorg build nixosTests.munin

@ToxicFrog
Copy link
Contributor Author

Is there anything else I need to do here (sync and resolve conflicts?) or is it just waiting for a maintainer to merge it?

@florianjacob
Copy link
Contributor

@FRidh do you have time for a review, or can recommend someone else?

@infinisil
Copy link
Member

I can agree with everything in this PR, but d2de4ccab1551fc04d206fa1ab967d7e09e4665a is a bit of an oddball. I've never seen an option do such auto-patching in the hopes that it works, but I guess it can't hurt to try it. Are you using those options yourself and have found those patches to be enough for your stuff?

@ToxicFrog
Copy link
Contributor Author

I am using those options, yes, and I found that to be sufficient. I have a checkout of munin-contrib that I point extraAutoPlugins at, and while hardcoded /bin/ and /sbin/ paths aren't really used in the mainline munin plugins, they are extremely common in munin-contrib (either directly, or by manually setting PATH=/bin/:/usr/bin/:/sbin/:/usr/sbin/ at the start of the file or similar).

This quick and dirty patch probably doesn't catch everything, but it's sufficient to get the plugins I wanted working and it doesn't seem to break anything that wasn't already not working.

That said, I think the right approach here is to have a separate munin-contrib Nix package and apply these fixes as part of the build process, and/or try to get patches upstreamed to remove hardcoded executable paths from contrib. This was just much faster to do, since I was already working in this file.

I don't feel particularly strongly about internAndFixPlugins; if you think it's too gross and/or dangerous to be part of the options, I can apply it to my checkout of munin-contrib in place and remove the autopatching behaviour from extraPlugins and extraAutoPlugins.

@infinisil
Copy link
Member

Alright I think it's fine to stay. When my other comment is addressed and the merge conflict fixed this is fine to merge.

@ToxicFrog
nixos/munin: fix documentation links
c02564e 
Since this module was written, Munin has moved their documentation from
munin-monitoring.org/wiki to guide.munin-monitoring.org. Most of the
links were broken, and the ones that weren't went to "please use the new
site" pages.
@ToxicFrog
nixos/munin: add extraPluginConfig option
c74abf7 
This lets you specify additional plugin-specific configuration to go in
plugin-conf.d, and complements the extraConfig and extraGlobalConfig
options.
@ToxicFrog
nixos/munin: add disabledPlugins option
0c3208a 
This is just a set of globs to remove from the active plugins directory
after autoconfiguration is complete.

I also removed the hard-coded disabling of "diskstats", since it seems
to work just fine now.
@ToxicFrog
nixos/munin: require DejaVu fonts if enabled
b5b82b2 
munin-graph is hardcoded to use DejaVu Mono for the graph legends; if it
can't find it, there's no guarantee it finds a monospaced font at all,
and if it can't find a monospaced font the legends come out badly
misformatted.
@ToxicFrog
nixos/munin: add extraPlugins and extraAutoPlugins options [NixOS#17895]
6c90785 
extraAutoPlugins lets you list plugins and plugin directories to be
autoconfigured, and extraPlugins lets you enable plugins on a one-by-one
basis. This can be used to enable plugins from contrib (although you'll
need to download and check out contrib yourself, then point these
options at it), or plugins you've written yourself.
@ToxicFrog
nixos/munin: add extraCSS option
c4437fe 
This permits custom styling of the generated HTML without needing to
build your own Munin package from source. Also comes with an example
that works as a passable dark theme for Munin.
@ToxicFrog
nixos/munin: add types to Munin options
e7c1449 
Some options were missing their types.
@ToxicFrog
nixos/munin: enable munin_update and disable munin_stats
ace4855 
munin_update relies on a stats file that exists, but isn't found in the
default location on NixOS; the appropriate plugin configuration is
added.

munin_stats relies on munin-cron writing a logfile, which the NixOS
build of munin does not. (This is probably fixable in the munin package,
but I don't have time to dig into that right now.)
@ToxicFrog
munin: 2.0.37 -> 2.0.43
85cdd06 
Also creates the RELEASE file in preBuild so that munin knows its own
version number at runtime.
@ToxicFrog
Copy link
Contributor Author

Rebased against latest master, conflicts resolved.

@domenkozar
Copy link
Member

@GrahamcOfBorg build tests.munin

@infinisil
Copy link
Member

infinisil commented Feb 5, 2019 •
edited

@GrahamcOfBorg test munin

Edit: Ah that's the same apparently

@infinisil infinisil merged commit dfce20e into NixOS:master Feb 5, 2019
@flokli flokli mentioned this pull request Dec 4, 2019
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florianjacob florianjacob florianjacob left review comments

@infinisil infinisil infinisil approved these changes

@bjornfor bjornfor Awaiting requested review from bjornfor

@domenkozar domenkozar Awaiting requested review from domenkozar

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ToxicFrog @florianjacob @FRidh @infinisil @domenkozar @GrahamcOfBorg @Janik-Haag