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/netdata: support adding extra packages to service PATH #33439

Closed

Conversation

kevin-hanselman
Copy link
Contributor

Motivation for this change

Fixes #33366, although a fair number of python plugins seem to fail
for subsequent reasons (as triaged by viewing netdata's error.log).

cc @jtojnar @davidak @cransom

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

Fixes NixOS#33366, although a fair number of python plugins seem to fail
for subsequent reasons (as triaged by viewing netdata's error.log).
@jtojnar
Copy link
Contributor

jtojnar commented Jan 5, 2018

I would still prefer patching the plugin loader – that way, you can run the file manually. If closure size is an issue, there are split outputs.

@kevin-hanselman
Copy link
Contributor Author

@jtojnar I'm not convinced that will fix the problem. Can you offer any clarification? Also, that's not as flexible as my approach, which allows for either Python 2.x or Python 3.x.

@jtojnar
Copy link
Contributor

jtojnar commented Jan 6, 2018

@kevlar1818 If you change shebang to /usr/bin/env python3, patchShebangs fixup output hook will change it to the actual path. I am not sure why would anyone need to use Python 2, the plugins should support Python 3 just fine.

The updated patch would look like this (not tested):

--- a/pkgs/tools/system/netdata/default.nix
+++ b/pkgs/tools/system/netdata/default.nix
@@ -1,6 +1,10 @@
-{ stdenv, fetchFromGitHub, autoreconfHook, zlib, pkgconfig, libuuid }:
+{ stdenv, fetchFromGitHub, autoreconfHook, zlib, pkgconfig, libuuid, python3 }:
 
-stdenv.mkDerivation rec{
+
+let
+  # https://github.com/firehol/netdata/wiki/Installation#1-prepare-your-system
+  python = with python3.pkgs; python.withPackages [ pyyaml dnspython pymongo pymysql psycopg2 ];
+in stdenv.mkDerivation rec{
   version = "1.9.0";
   name = "netdata-${version}";
 
@@ -12,10 +16,13 @@
   };
 
   nativeBuildInputs = [ autoreconfHook pkgconfig ];
-  buildInputs = [ zlib libuuid ];
+  buildInputs = [ zlib libuuid python ];
 
   # Allow UI to load when running as non-root
-  patches = [ ./web_access.patch ];
+  patches = [
+    ./web_access.patch
+    ./python-plugins.patch
+  ];
 
   # Build will fail trying to create /var/{cache,lib,log}/netdata without this
   postPatch = ''
--- /dev/null
+++ b/pkgs/tools/system/netdata/python-plugins.patch
@@ -0,0 +1,10 @@
+--- a/plugins.d/python.d.plugin
++++ b/plugins.d/python.d.plugin
+@@ -1,6 +1,4 @@
+-#!/usr/bin/env bash
+-'''':; exec "$(command -v python || command -v python3 || command -v python2 ||
+-echo "ERROR python IS NOT AVAILABLE IN THIS SYSTEM")" "$0" "$@" # '''
++#!/usr/bin/env python3
+ 
+ # -*- coding: utf-8 -*-
+ # Description:

@kevin-hanselman
Copy link
Contributor Author

@jtojnar How do you envision an end-user modifying the extra plugin packages? I think it should be as frictionless as possible for someone to choose between a basic netdata install and a customized and/or fully-loaded netdata install.

@jtojnar
Copy link
Contributor

jtojnar commented Jan 6, 2018

If we want to allow installing plugin-less netdata, we could move the relevant plugins into split outputs. Or we could add an withPythonPlugins argument for the derivation and delete $out/libexec/netdata/python.d/ and $out/libexec/netdata/plugins.d/python.d.plugin if it is false, that way Python will not be in the closure. But IMHO, if the plugins are in the output, they should be working without any additional module configuration.

@kevin-hanselman
Copy link
Contributor Author

@jtojnar What about built-in plugins that use other programs, such as lm_sensors or IFPS? Do we install them too? I don't think it's sustainable to install all the programs for all the plugins.

@jtojnar
Copy link
Contributor

jtojnar commented Jan 6, 2018

Maybe something like this?

--- a/pkgs/tools/system/netdata/default.nix
+++ b/pkgs/tools/system/netdata/default.nix
@@ -1,6 +1,16 @@
-{ stdenv, fetchFromGitHub, autoreconfHook, zlib, pkgconfig, libuuid }:
+{ stdenv, fetchFromGitHub, autoreconfHook, zlib, pkgconfig, libuuid, makeWrapper
+, withPython ? false, python3, withNode ? false, nodejs, withLmSensors ? false, lm_sensors }:
 
-stdenv.mkDerivation rec{
+let
+  inherit (stdenv.lib) optional optionalString;
+  # https://github.com/firehol/netdata/wiki/Installation#1-prepare-your-system
+  python = python3.withPackages (p: with p; [ pyyaml dnspython pymongo pymysql psycopg2 ]);
+  binDependencies =
+    optional withPython python
+    ++ optional withNode nodejs;
+  libDependencies =
+    optional withLmSensors lm_sensors;
+in stdenv.mkDerivation rec{
   version = "1.9.0";
   name = "netdata-${version}";
 
@@ -11,11 +21,13 @@
     sha256 = "1vy0jz5lxw63b830l9jgf1qqhp41gzapyhdr5k1gwg3zghvlg10w";
   };
 
-  nativeBuildInputs = [ autoreconfHook pkgconfig ];
+  nativeBuildInputs = [ autoreconfHook pkgconfig makeWrapper ];
   buildInputs = [ zlib libuuid ];
 
   # Allow UI to load when running as non-root
-  patches = [ ./web_access.patch ];
+  patches = [
+    ./web_access.patch
+  ];
 
   # Build will fail trying to create /var/{cache,lib,log}/netdata without this
   postPatch = ''
@@ -30,8 +42,19 @@
   # The upstream installer does prepare an empty file too
   postInstall = ''
     touch $out/etc/netdata/netdata.conf
+
   '';
 
+  postFixup = ''
+    wrapProgram $out/bin/netdata \
+      --prefix PATH : "${stdenv.lib.makeBinPath binDependencies}" \
+      --prefix LD_LIBRARY_PATH : "${stdenv.lib.makeLibraryPath libDependencies}"
+  ''
+  # Remove unwanted plugins so that they cannot be accidentally enabled
+  + optionalString (!withPython) "rm -rf $out/libexec/netdata/python.d/*"
+  + optionalString (!withNode) "rm -rf $out/libexec/netdata/node.d/*"
+  + optionalString (!withLmSensors) "rm -f $out/libexec/netdata/python.d/sensors.chart.py";
+
   meta = with stdenv.lib; {
     description = "Real-time performance monitoring tool";
     homepage = http://netdata.firehol.org;

@kevin-hanselman
Copy link
Contributor Author

kevin-hanselman commented Jan 9, 2018

@jtojnar I guess what I'm getting at is that this pattern would have to be duplicated a hundred times or more to enable all the plugins in netdata. Because netdata prides itself on it's wealth of auto-detected plugins, I would argue for a simpler way to enable plugins (ideally, zero configuration). That is why I prefer my approach. If there's some middle ground to our two solutions, such as a more generic way to call wrapProgram with a bunch of binaries/libraries, I could get behind that. Thoughts?

@jtojnar
Copy link
Contributor

jtojnar commented Jan 9, 2018

The problem is autoconfiguration is just against Nix way of doing things. We sometimes compromise on this when it is too complicated to fix, but in this case, it is fairly simple, even though slightly verbose.

@mmahut
Copy link
Member

mmahut commented Aug 3, 2019

What is the status of this pull request?

@kevin-hanselman
Copy link
Contributor Author

I'm no longer working on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

netdata: All python plugins broken
4 participants