nixos/redis: unbreak module, add test #67845

flokli · 2019-08-31T18:40:53Z

Motivation for this change

The nixos redis module seems to be pretty broken, most likely due to a chown in preStart. I updated it to use StateDirectory (cf #56265), added a nixos vm test and a changelog entry.

Things done

flokli · 2019-08-31T18:41:53Z

@GrahamcOfBorg test redis

flokli · 2019-08-31T19:03:38Z

This needs a second iteration, keeping the static user, but using StateDirectory=redis - we can't use DynamicUser with file socket support.

flokli · 2019-08-31T20:09:50Z

updated, now uses StateDirectory and RuntimeDirectory, but keeps a non-dynamic user. Also updated the test to try connecting over the socket as well.

flokli · 2019-08-31T21:40:44Z

@GrahamcOfBorg test redis

nixos/modules/services/databases/redis.nix

aanderse · 2019-09-01T01:39:38Z

@GrahamcOfBorg test redis

worldofpeace

LGTM, not qualified to review the test but it looks pretty typical for these types of things.

peti · 2019-09-01T06:51:36Z

nixos/modules/services/databases/redis.nix

@@ -218,7 +187,7 @@ in
    };

    users.users.redis =
-      { name = cfg.user;
+      { name = "redis";


name doesn't need to be set here. AFAIK, it's implied by users.users.redis.

peti · 2019-09-01T06:54:23Z

nixos/modules/rename.nix

+    (mkRemovedOptionModule [ "services" "redis" "dbFilename" ] "The redis module now uses /var/lib/redis/dump.rdb as database dump location.")
+    (mkRemovedOptionModule [ "services" "redis" "appendOnlyFilename" ] "This option was never used.")
+    (mkRemovedOptionModule [ "services" "redis" "pidFile" ] "This option was removed.")
+


Personally, I am in favor of removing those options. I fear there might be push-back, though.

I don't think people care about the pidFile option, and appendOnlyFilename wasn't used at all.

Some might care about user, dbpath and dbfilename, but only if they changed the defaults - we previously also used /var/lib/redis/dump.rdb by default, as the redis user.

So in that case, nix will complain during rebuild (don't just spin up an empty redis) and people will be able to move the dump.rbd file to the new location.

peti · 2019-09-01T06:56:07Z

nixos/modules/services/databases/redis.nix

@@ -8,7 +8,6 @@ let
  condOption = name: value: if value != null then "${name} ${toString value}" else "";

  redisConfig = pkgs.writeText "redis.conf" ''
-    pidfile ${cfg.pidFile}


Where will Redis write the PID file once this statement is removed?

So far, we did ran redis in non-daemonized mode, meaning it didn't create a pidfile if not explicitly told so (and I removed the line in the version you specified).
In daemonized mode, it'll default to put it in /run/redis.pid, but won't complain if it can't at all.

If a pid file is specified, Redis writes it where specified at startup
and removes it at exit.

When the server runs non daemonized, no pid file is created if none is
specified in the configuration. When the server is daemonized, the pid file
is used even if not specified, defaulting to "/var/run/redis.pid".

Creating a pid file is best effort: if Redis is not able to create it
nothing bad happens, the server will start and run normally.

As we don't need the pidfile to manage the service, I removed the nixos module option.

I now hardcoded it to /run/redis/redis.pid (which is inside the RuntimeDirectory), enabled daemonizitation, and also enabled Type=notify support to signal readyness.

globin · 2019-09-01T10:33:43Z

@GrahamcOfBorg test redis

The redis module currently fails to start up, most likely due to running a chown as non-root in preStart. While at it, I hardcoded it to use systemd's StateDirectory and DynamicUser to manage directory permissions, removed the unused appendOnlyFilename option, and the pidFile option. We properly tell redis now it's daemonized, and it'll use notify support to signal readiness.

flokli · 2019-09-01T12:42:34Z

@peti by looking at the nixos test logs, I still see the following two warnings:

machine# [    7.105380] redis[657]: WARNING: The TCP backlog setting of 511 cannot be enforced because /proc/sys/net/core/somaxconn is set to the lower value of 128.
machine# [    7.111772] redis[657]: WARNING overcommit_memory is set to 0! Background save may fail under low memory condition. To fix this issue add 'vm.overcommit_memory = 1' to /etc/sysctl.conf and then reboot or run the command 'sysctl vm.overcommit_memory=1' for this to take effect.

Can you comment on the two options? Should we set these aswell?

flokli · 2019-09-01T16:30:45Z

Alright, this can be done in a followup PR.

dasJ · 2019-10-10T20:22:23Z

@flokli What about isSystemUser?

flokli · 2019-10-11T16:17:07Z

@flokli What about isSystemUser?

I'm missing some context here…

dasJ · 2019-10-12T20:27:44Z

@flokli: Nvm, I created #71055

flokli requested review from peti, aanderse and worldofpeace August 31, 2019 18:40

This was referenced Aug 31, 2019

nixos/redis: disable transparent huge pages (TLP) before starting Redis #67768

Merged

gitlab: 12.1.6 -> 12.2.3 #67770

Closed

ofborg bot added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 31, 2019

flokli force-pushed the redis-module branch from d47c6d2 to 8518889 Compare August 31, 2019 20:07

aanderse requested changes Sep 1, 2019

View reviewed changes

nixos/modules/services/databases/redis.nix Outdated Show resolved Hide resolved

flokli force-pushed the redis-module branch from 8518889 to e42512d Compare September 1, 2019 01:19

aanderse approved these changes Sep 1, 2019

View reviewed changes

flokli force-pushed the redis-module branch from e42512d to 67e5535 Compare September 1, 2019 01:53

worldofpeace approved these changes Sep 1, 2019

View reviewed changes

peti reviewed Sep 1, 2019

View reviewed changes

flokli force-pushed the redis-module branch from 67e5535 to 197bf3a Compare September 1, 2019 11:53

flokli force-pushed the redis-module branch from 197bf3a to 276ae10 Compare September 1, 2019 12:08

flokli added 2 commits September 1, 2019 14:12

nixos/redis: add changelog for NixOS#67768

8680f72

nixos/redis: add test

c00c4b1

flokli force-pushed the redis-module branch from 276ae10 to c00c4b1 Compare September 1, 2019 12:13

flokli merged commit 15f3e4e into NixOS:master Sep 1, 2019

flokli deleted the redis-module branch September 1, 2019 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/redis: unbreak module, add test #67845

nixos/redis: unbreak module, add test #67845

flokli commented Aug 31, 2019 •

edited

flokli commented Aug 31, 2019

flokli commented Aug 31, 2019

flokli commented Aug 31, 2019 •

edited

flokli commented Aug 31, 2019

aanderse commented Sep 1, 2019

worldofpeace left a comment

peti Sep 1, 2019

flokli Sep 1, 2019

peti Sep 1, 2019

flokli Sep 1, 2019

peti Sep 1, 2019

flokli Sep 1, 2019 •

edited

globin commented Sep 1, 2019

flokli commented Sep 1, 2019 •

edited

flokli commented Sep 1, 2019

dasJ commented Oct 10, 2019

flokli commented Oct 11, 2019

dasJ commented Oct 12, 2019

nixos/redis: unbreak module, add test #67845

nixos/redis: unbreak module, add test #67845

Conversation

flokli commented Aug 31, 2019 • edited

Motivation for this change

Things done

flokli commented Aug 31, 2019

flokli commented Aug 31, 2019

flokli commented Aug 31, 2019 • edited

flokli commented Aug 31, 2019

aanderse commented Sep 1, 2019

worldofpeace left a comment

Choose a reason for hiding this comment

peti Sep 1, 2019

Choose a reason for hiding this comment

flokli Sep 1, 2019

Choose a reason for hiding this comment

peti Sep 1, 2019

Choose a reason for hiding this comment

flokli Sep 1, 2019

Choose a reason for hiding this comment

peti Sep 1, 2019

Choose a reason for hiding this comment

flokli Sep 1, 2019 • edited

Choose a reason for hiding this comment

globin commented Sep 1, 2019

flokli commented Sep 1, 2019 • edited

flokli commented Sep 1, 2019

dasJ commented Oct 10, 2019

flokli commented Oct 11, 2019

dasJ commented Oct 12, 2019

flokli commented Aug 31, 2019 •

edited

flokli commented Aug 31, 2019 •

edited

flokli Sep 1, 2019 •

edited

flokli commented Sep 1, 2019 •

edited