nixos/riemann: refactor config #44341

shmish111 · 2018-08-02T08:32:48Z

Previously it was only possible to use very simple Riemann config.
For more complicated scenarios you need a directory of clojure
files and the config file that riemann starts with should be in this
directory.

Motivation for this change

I needed a more complex Riemann setup in my NixOS project.

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)
Fits CONTRIBUTING.md.

infinisil

I'm pretty sure the config file options you added can simply be done through the configFiles option, they're not needed.

infinisil · 2018-08-02T21:51:37Z

nixos/modules/services/monitoring/riemann.nix

@@ -64,6 +93,12 @@ in {
          Extra Java options used when launching Riemann.
        '';
      };
+      environment = mkOption {


Users can just set systemd.services.riemann.environment directly in their NixOS config, will work just as expected.

I'm happy to take this out, just wasn't sure which was the more nixy way of doing things.

the reason I ask this is because I am using this as:

services.riemann = { enable = true; environment = { RIEMANN_HOME = riemannConfigDir; }; configDirectory = riemannConfigDir; };

and it would not look so nice to me if the environment line was outside of the definition of services.riemann. What do you think @infinisil @joachifm

I think options for services should provide something more powerful than just an alias for another options. But I see your point, the config would be all neat and together.

Eventually I think a better solution to that is to provide a generic service option to be used to get to a services systemd service like this:

services.riemann = { enable = true; service.environment = { RIEMANN_HOME = riemannConfigDir; }; configDirectory = riemannConfigDir; }

This should probably then be a major nixpkgs change to implement it uniformly.

For now I think using

systemd.services.riemann.environment.RIEMANN_HOME = riemannConfigDir; };

should be enough.

yeah I like the sounds of that. However in the short term, do you want me to change this or leave it as is?

I'd rather not have this (the new environment option).

infinisil · 2018-08-02T21:58:30Z

nixos/modules/services/monitoring/riemann.nix

      serviceConfig = {
        User = "riemann";
        ExecStart = "${launcher}/bin/riemann";
      };
+      serviceConfig.LimitNOFILE = 65536;
+      restartTriggers = [ configFile ];


These 2 lines are the only ones I can agree with right now.

shmish111 · 2018-08-14T23:08:07Z

@infinisil configFiles is only suitable for basic deployments since it just uses load-file at the end of the configuration. You could in theory get things done like this but it doesn't allow you to set up a proper Clojure project to do complex development of riemann config. Riemann has a built in mechanism to allow this in that it will add all Clojure files in the directory of the config file onto the classpath. The current nix module solution of load-file is not really suitable, I've left it in here as it is workable and presumably some people are already doing that so we should maintain backward compatibility however it should not be the recommended solution. I am using my fork in production which enables me to setup my Riemann config as a leiningen project and just copy the source files across to the nix config.

infinisil · 2018-08-15T12:59:44Z

Riemann has a built in mechanism to allow this in that it will add all Clojure files in the directory of the config file onto the classpath.

Okay that's an argument, there's no other way to achieve this? No direct argument to pass for adding stuff to the classpath? Doing it via this configDir/configFile way seems a bit ugly.

Also, if we keep this, you can just use a single option for configFile instead of configDirectory and configFileName, since the only place you use these new options is to pass a single file to it ("${cfg.configDirectory}/${cfg.configFileName}"). Clojure would then automatically use dirname configFile as the directory.

shmish111 · 2018-08-15T13:15:02Z

@infinisil I think it's fine to change to a single file and implicitly look in the directory that contains that file, I'll test it out.

Personally I think that adding all files explicitly is much uglier, I want to have a source directory and provide that to the config (plus the name of the main config file). Also, constructing a config file in nix is not very nice, Riemann config is not like 'config' in other context, it is a full application, 'config' is perhaps a bad name for it.

shmish111 · 2018-08-17T08:47:56Z

ok @infinisil I've made those 2 changes and squashed things, I think it looks much better thanks. Ready for review again.

shmish111 · 2018-09-05T07:54:12Z

bump @infinisil

infinisil · 2018-09-06T21:30:12Z

nixos/modules/services/monitoring/riemann.nix

@@ -84,6 +100,8 @@ in {
        User = "riemann";
        ExecStart = "${launcher}/bin/riemann";
      };
+      serviceConfig.LimitNOFILE = 65536;
+      restartTriggers = [ configFile ];


Since the launcher will change every time the config file changes, there's no need to add it to the restart triggers manually.

infinisil · 2018-09-06T21:30:23Z

nixos/modules/services/monitoring/riemann.nix

+      cfg.configFile
+    else
+      "${writeText "riemann-config.clj" riemannConfig}"
+  );


Unnecessary parens

infinisil · 2018-09-06T21:30:42Z

nixos/modules/services/monitoring/riemann.nix

+    then
+      cfg.configFile
+    else
+      "${writeText "riemann-config.clj" riemannConfig}"


Unnecessary "${..}"

infinisil · 2018-09-06T21:35:45Z

nixos/modules/services/monitoring/riemann.nix

@@ -15,11 +15,18 @@ let
    [cfg.config] ++ (map (f: ''(load-file "${f}")'') cfg.configFiles)
  );

+  configFile = (if (cfg.configFile != null)


I'll also suggest a cleaner alternative of implementing this:

{ # ... config = mkIf cfg.enable { services.riemann.configFile = mkDefault ( writeText "riemann-config.clj" riemannConfig ); }; }

And then use cfg.configFile in the launcher.

The module system will then automatically take care of making a user-specified configFile a priority. This also has the advantage that the user can see the generated config file via nix-instantiate --eval '<nixpkgs/nixos>' -A config.services.riemann.configFile

Previously it was only possible to use very simple Riemann config. For more complicated scenarios you need a directory of clojure files and the config file that riemann starts with should be in this directory.

shmish111 · 2018-09-07T09:00:13Z

ok @infinisil I have made the suggested changes and tested it on my own nixos machine, works nicely

infinisil requested changes Aug 2, 2018

View reviewed changes

infinisil reviewed Aug 2, 2018

View reviewed changes

shmish111 force-pushed the riemann-options branch from d383c7f to 0e5d60d Compare August 17, 2018 08:46

infinisil requested changes Sep 6, 2018

View reviewed changes

shmish111 force-pushed the riemann-options branch from 0e5d60d to 7f76719 Compare September 7, 2018 08:12

nixos/riemann: refactor config

1d497bb

Previously it was only possible to use very simple Riemann config. For more complicated scenarios you need a directory of clojure files and the config file that riemann starts with should be in this directory.

shmish111 force-pushed the riemann-options branch from 9c276eb to 1d497bb Compare September 7, 2018 08:47

infinisil approved these changes Sep 7, 2018

View reviewed changes

infinisil merged commit 00c6f85 into NixOS:master Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/riemann: refactor config #44341

nixos/riemann: refactor config #44341

shmish111 commented Aug 2, 2018 •

edited

Loading

infinisil left a comment •

edited

Loading

infinisil Aug 2, 2018

shmish111 Aug 14, 2018

shmish111 Aug 15, 2018

infinisil Aug 15, 2018 •

edited

Loading

shmish111 Aug 15, 2018

infinisil Aug 15, 2018 •

edited

Loading

infinisil Aug 2, 2018

shmish111 commented Aug 14, 2018

infinisil commented Aug 15, 2018

shmish111 commented Aug 15, 2018

shmish111 commented Aug 17, 2018

shmish111 commented Sep 5, 2018

infinisil Sep 6, 2018

infinisil Sep 6, 2018

infinisil Sep 6, 2018

infinisil Sep 6, 2018

shmish111 commented Sep 7, 2018

nixos/riemann: refactor config #44341

nixos/riemann: refactor config #44341

Conversation

shmish111 commented Aug 2, 2018 • edited Loading

Motivation for this change

Things done

infinisil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmish111 commented Aug 14, 2018

infinisil commented Aug 15, 2018

shmish111 commented Aug 15, 2018

shmish111 commented Aug 17, 2018

shmish111 commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmish111 commented Sep 7, 2018

shmish111 commented Aug 2, 2018 •

edited

Loading

infinisil left a comment •

edited

Loading

infinisil Aug 15, 2018 •

edited

Loading

infinisil Aug 15, 2018 •

edited

Loading