matrix-synapse: create and connect to local postgresql db #31223

corngood · 2017-11-04T02:00:12Z

Motivation for this change

A better default configuration for matrix-synapse when database_type = "psycopg2". Postgresql will be enabled, the role/database will be created, and matrix-synapse will connect using the unix socket.

TODO:

default to postgres based on stateVersion?
tests

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.

corngood · 2017-11-04T02:01:14Z

@Ralith This is still a bit messy, but I'd appreciate your thoughts.

Ralith

The idea is that the user still has to manually enable postgres, but no longer has to manually configure the database by hand on a new HS, right? That's definitely appealing. Has this been tested live?

Ralith · 2017-11-04T02:15:12Z

nixos/modules/services/misc/matrix-synapse.nix

  logConfigFile = pkgs.writeText "log_config.yaml" cfg.logConfig;
  mkResource = r: ''{names: ${builtins.toJSON r.names}, compress: ${boolToString r.compress}}'';
  mkListener = l: ''{port: ${toString l.port}, bind_address: "${l.bind_address}", type: ${l.type}, tls: ${boolToString l.tls}, x_forwarded: ${boolToString l.x_forwarded}, resources: [${concatStringsSep "," (map mkResource l.resources)}]}'';
+  # TODO: finish this and put it in lib somewhere
+  toYaml = value:


Isn't yaml a superset of JSON? That's how we're getting away with toJSON elsewhere.

Ralith · 2017-11-04T02:20:43Z

nixos/modules/services/misc/matrix-synapse.nix

        createHome = true;
        shell = "${pkgs.bash}/bin/bash";
-        uid = config.ids.uids.matrix-synapse;
+        uid = config.ids.uids."${user}";


isn't config.ids.uids a static table? I don't think this is useful, and it brings into question the whole dynamic user name option.

Yeah, I suppose it is. I didn't actually expose the user name, I was just trying to avoid duplication. i'll put it back the way it was.

Ralith · 2017-11-04T02:20:56Z

nixos/modules/services/misc/matrix-synapse.nix

-      { name = "matrix-synapse";
-        gid = config.ids.gids.matrix-synapse;
+      { name = group;
+        gid = config.ids.gids."${group}";


See comment on uid.

Ralith · 2017-11-04T02:22:43Z

nixos/modules/services/misc/matrix-synapse.nix

+            --lc-collate=C \
+            --lc-ctype=C \
+            --template=template0 \
+            ${cfg.database_name}


Are these db setup commands guaranteed to be idempotent?

They will fail if you remove db-created without dropping the db/role. It's not a very elegant solution, but it's what's used in other similar modules.

Er, right, not sure how I managed to forget the guard file even though it's right there.

corngood · 2017-11-04T02:45:39Z

Yeah, that's the idea. Postgres doesn't need to be enabled, but you do need to switch database_type. Perhaps postgres is a better default, but then we're getting into seriously breaking changes. Maybe base it on system.stateVersion?

corngood · 2017-11-04T02:49:28Z

And yeah, I'm using it on my home server. I switched my VPS to this config, shut down matrix-synapse, ran synapse_port_db, and it seems to be working properly.

The database setup is based on how other PG dependants (gitlab, mattermost) do it.

corngood · 2017-11-04T03:06:59Z

nixos/modules/services/misc/matrix-synapse.nix

+            user = cfg.database_user;
+            database = cfg.database_name;
+          };
+        }."${cfg.database_type}";


This seems wrong to me, but it's useful and seems to work. Is there a better way?

It didn't even occur to me you could do this. I'm no Nix language expert, so no strong feelings.

Well, an alternative is to have two different options for two databases, I guess. Maybe defining with mkIf, so that you cannot set the wrong one.

I am not sure that would be better.

Ralith · 2017-11-04T03:15:18Z

Defaulting based on stateVersion sounds nice to me. IMO sqlite-by-default is user-hostile even in environments that can't magic up a postgres instance without any extra effort.

Ralith

I'm generally in favor of this, and I don't see any obvious issues in the approach. It doesn't strike me as noticeably messier than what's already there, either; as far as I'm concerned, trusting that it works, it's okay to merge as soon as you're happy with it.

corngood · 2017-11-04T12:50:33Z

I'm thinking it's ok to use stateVersion >= 18.03 at this point. I see other breaking changes like this that were made after .version was updated, but before release.

corngood · 2017-11-04T18:27:28Z

I made the stateVersion change, and added it to the release notes. I also wrote a test that ensures it can start up with either db using a trivial config.

Ralith · 2017-11-04T23:17:43Z

nixos/tests/matrix-synapse.nix

+    server_postgres = server;
+
+    server_sqlite = args: server args // {
+      system.stateVersion = "17.09";


Wouldn't it be better for this to just explicitly turn on sqlite? Who knows what other side effects this specific stateVersion might come to have over time.

Yeah, I guess I originally intended to verify that it was using the correct db for each stateVersion, but I didn't actually test for it. I suppose the stateVersion logic is pretty simple, and it would be more intuitive to explicitly test the backends.

corngood · 2017-11-22T16:43:35Z

@fpletz I noticed you approved the changes this conflicts with, so would you mind taking a look at this before I rebase?

Mic92 · 2017-11-25T15:48:40Z

@GrahamcOfBorg test matrix-synapse

GrahamcOfBorg

Failure for system: x86_64-linux

Merge failed

Mic92 · 2017-11-25T15:54:23Z

@corngood can you rebase?

corngood · 2017-11-25T16:13:27Z

Yeah, just did. Testing locally now.

I wonder if I can do this:

@GrahamcOfBorg test matrix-synapse

corngood · 2017-12-20T13:28:22Z

@fpletz You merged the last matrix-synapse change, so would you mind taking a look? Thanks

corngood · 2018-01-07T16:55:39Z

Resolved conflicts. Can anyone suggest changes or push this along?

Mic92 · 2018-01-12T13:23:24Z

@GrahamcOfBorg test matrix-synapse

GrahamcOfBorg

Failure for system: x86_64-linux

Merge failed

Mic92 · 2018-01-12T13:26:50Z

@GrahamcOfBorg test matrix-synapse

GrahamcOfBorg

Failure for system: x86_64-linux

                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  5 61.1M    5 3175k    0     0  3175k      0  0:00:19  0:00:01  0:00:18 1625k
 42 9015k   42 3813k    0     0  3813k      0  0:00:02  0:00:01  0:00:01 2050k
 17 6058k   17 1086k    0     0  1086k      0  0:00:05  0:00:01  0:00:04 1065k
  9  916k    9 86669    0     0  86669      0  0:00:10 --:--:--  0:00:10  133k
 45 37.5M   45 16.9M    0     0  8699k      0  0:00:04  0:00:02  0:00:02 6368k
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
 35 6058k   35 2146k    0     0  1073k      0  0:00:05  0:00:02  0:00:03 1063k
  7 61.1M    7 4519k    0     0  2259k      0  0:00:27  0:00:02  0:00:25 1529k
 61 9015k   61 5541k    0     0  2770k      0  0:00:03  0:00:02  0:00:01 1936k
 76  916k   76  703k    0     0   703k      0  0:00:01  0:00:01 --:--:--  439k
 65  916k   65  600k    0     0   600k      0  0:00:01  0:00:01 --:--:--  420k
 68 37.5M   68 25.9M    0     0  8850k      0  0:00:04  0:00:03  0:00:01 6998k
100  916k  100  916k    0     0   916k      0  0:00:01  0:00:01 --:--:--  479k

building path(s) ‘/nix/store/xv9xbqmlzxb0b3aai73m13iy0v36r8yg-append-initrd-secrets’
killing process 4537
killing process 4533
killing process 4565
killing process 4588
killing process 4481
error: a ‘aarch64-linux’ is required to build ‘/nix/store/k6q6y9vqnhvzz1ypvw9zb8pnkza2zp7g-append-initrd-secrets.drv’, but I am a ‘x86_64-linux’

corngood · 2018-01-12T19:36:57Z

@Mic92 Any idea about this test failure?

I'll rebase and run tests locally when I have a chance.

Edit: looks like GOB has been posting similar failures on a variety of PRs recently.

corngood · 2018-01-13T01:51:02Z

Ok, I rebased on master, and tests passed locally.

Mic92 · 2018-01-13T18:39:10Z

@corngood this is a bug in the bot. It tries to build the aarch64 build on x86_64.

corngood · 2018-02-05T01:29:34Z

Rebased again... can someone give me some feedback? This will hopefully save people having to do migrations when they discover it's not using the recommended DB.

The documentation and state version will need to be changed if this doesn't make 18.03.

Ralith · 2018-02-05T01:41:40Z

Probably wouldn't hurt to add a release note regarding the (stateVersion-guarded) breaking change to defaults. Hopefully someone will merge this before 18.03 releases.

7c6f434c · 2018-02-05T01:43:15Z

@GrahamcOfBorg test matrix-synapse

7c6f434c

Looks fine in general

7c6f434c · 2018-02-05T01:45:41Z

nixos/modules/services/misc/matrix-synapse.nix

+            user = cfg.database_user;
+            database = cfg.database_name;
+          };
+        }."${cfg.database_type}";


Well, an alternative is to have two different options for two databases, I guess. Maybe defining with mkIf, so that you cannot set the wrong one.

I am not sure that would be better.

GrahamcOfBorg · 2018-02-05T01:46:09Z

Success on aarch64-linux (full log)

Partial log (click to expand)

server_sqlite: exit status 0
server_postgres: running command: sync
server_postgres: exit status 0
test script finished in 96.08s
cleaning up
killing server_sqlite (pid 627)
killing server_postgres (pid 638)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/zz4ih0kcjpw8hs6yjv8js8is1a2xwp2l-vm-test-run-matrix-synapse

GrahamcOfBorg · 2018-02-05T02:14:10Z

Failure on x86_64-linux (full log)

Partial log (click to expand)

server_postgres: running command: systemctl --no-pager show "matrix-synapse.service"
server_postgres: exit status 0
server_postgres: running command: systemctl --no-pager show "matrix-synapse.service"
server_postgres: exit status 0
server_postgres: running command: systemctl --no-pager show "matrix-synapse.service"
server_postgres: exit status 0
server_postgres: running command: systemctl --no-pager show "matrix-synapse.service"
server_postgres: exit status 0
building of ‘/nix/store/gv2w80vfbi4ji7c36f6az8c4mzhqmmry-vm-test-run-matrix-synapse.drv’ timed out after 1800 seconds
error: build of ‘/nix/store/gv2w80vfbi4ji7c36f6az8c4mzhqmmry-vm-test-run-matrix-synapse.drv’ failed

corngood · 2018-02-05T02:15:53Z

FYI I just ran the tests locally and they passed on x86_64-linux.

7c6f434c · 2018-02-05T08:09:42Z

Yes, I don't always trust borg timeouts, and I would be very surprised if you managed to write a matrix service that genuinely works better on aarch64.

Ralith reviewed Nov 4, 2017

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 4, 2017

corngood force-pushed the matrix-postgres branch from 8c009b2 to 15a089a Compare November 4, 2017 03:00

corngood commented Nov 4, 2017

View reviewed changes

Ralith approved these changes Nov 4, 2017

View reviewed changes

corngood changed the title ~~WIP: matrix-synapse: create and connect to local postgresql db~~ matrix-synapse: create and connect to local postgresql db Nov 4, 2017

Ralith reviewed Nov 4, 2017

View reviewed changes

corngood force-pushed the matrix-postgres branch from 04d8df8 to a58d1c0 Compare November 5, 2017 01:24

GrahamcOfBorg reviewed Nov 25, 2017

View reviewed changes

corngood force-pushed the matrix-postgres branch from a58d1c0 to bc57fc6 Compare November 25, 2017 16:12

corngood force-pushed the matrix-postgres branch from bc57fc6 to 1c8bd9b Compare January 7, 2018 16:54

GrahamcOfBorg reviewed Jan 12, 2018

View reviewed changes

corngood force-pushed the matrix-postgres branch from da73a16 to 013a01e Compare January 13, 2018 01:41

corngood added 4 commits February 4, 2018 21:22

matrix-synapse: create and connect to local postgresql db

1472fa8

matrix-synapse: default to postgresql on 18.03

a4b7de7

matrix-synapse: default server_name to hostname

fbba0d0

nixos/tests: add matrix-synapse test

e591f11

corngood force-pushed the matrix-postgres branch from 013a01e to e591f11 Compare February 5, 2018 01:25

GrahamcOfBorg added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) labels Feb 5, 2018

7c6f434c approved these changes Feb 5, 2018

View reviewed changes

7c6f434c merged commit 176d94f into NixOS:master Feb 5, 2018

matrix-synapse: create and connect to local postgresql db #31223

matrix-synapse: create and connect to local postgresql db #31223

Conversation

corngood commented Nov 4, 2017 • edited

Motivation for this change

Things done

corngood commented Nov 4, 2017

Ralith left a comment

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

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

corngood commented Nov 4, 2017

corngood commented Nov 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ralith commented Nov 4, 2017

Ralith left a comment

Choose a reason for hiding this comment

corngood commented Nov 4, 2017

corngood commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corngood commented Nov 22, 2017

Mic92 commented Nov 25, 2017

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

Mic92 commented Nov 25, 2017 • edited

corngood commented Nov 25, 2017

corngood commented Dec 20, 2017

corngood commented Jan 7, 2018

Mic92 commented Jan 12, 2018

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

Mic92 commented Jan 12, 2018

GrahamcOfBorg left a comment

Choose a reason for hiding this comment

corngood commented Jan 12, 2018 • edited

corngood commented Jan 13, 2018

Mic92 commented Jan 13, 2018

corngood commented Feb 5, 2018

Ralith commented Feb 5, 2018

7c6f434c commented Feb 5, 2018

7c6f434c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamcOfBorg commented Feb 5, 2018

GrahamcOfBorg commented Feb 5, 2018

corngood commented Feb 5, 2018

7c6f434c commented Feb 5, 2018

corngood commented Nov 4, 2017 •

edited

corngood commented Nov 4, 2017 •

edited

Mic92 commented Nov 25, 2017 •

edited

corngood commented Jan 12, 2018 •

edited