In reply to @dandellion:dodsorf.as

ma27: Could you elaborate on what you meant by

The entire generation of units for workers somehow works around the module-system in a very weird way that I really don't want to maintain.

ok I wasn't too precise there. I meant the import of the worker-module which creates these units, sorry.

The module-system consists of functions with certain arguments (defined by _module.args - such as pkgs/lib/config), but all the other stuff should be built with (internal) options. On a destructured attribute-set as argument the attribute names are not lazy, so you depend on at least the bindings already (though the values seem to be unevaluated thunks). However, while evaluating imports of a declaration (synapse.nix in this case) it's possible that too much of config will be unintentionally evaluated at some point causing weird behavior (prime example of a current issue is that specialArgs.nixosModules works whereas config._module.args.nixosModules causes an infinite recursion). Yes, it seems to just evaluate at the moment, but this code working around the design so much seems like a time-bomb to me which will send somebody on a long debugging journey, so I don't want to see that merged (and internals of the module system may change in a subtle way at some point).

additionally, are there that many differences left now? the other PR also did services.matrix-synapse.workers.foobar = { /* workerConfig */ }; (i.e. attrsOf) when I first looked at it which seems to be your main issue. And generally the other PR seemed closer to being mergable (because of dealing with systemd hardening, logger config and using sockets with redis - by default there's no real reason to use a tcp socket here since everything is on the same machine anyways).

fwiw I'm not sure if more opinionated things such as nginx config generation for load-balancing of workers should be in a nixos module (at least as long as we don't have rfc42 for nginx - otherwise I'm not too worried about such an idea) because it's far too easy then to run into a problem that isn't covered by the module and then the config will be very hard to customize from my experience with maintaining nextcloud which actually ships with a full-blown nginx config. I may be wrong though, so if you think you have a good design for that, feel free to file a patch, I'm happy to take a look :)

Thanks for clarifying.
I agree the import with the function is a little stinky.

I was not aware that this PR does attrsOf now, that's great! I'm not going to block this at all then

The only difference left is probably that I use workers.instances and this one does just pure workers like was suggested. This is fine, I can just use autoWorkers or something for part 2

I'll do a more thourough review, but I generally support merging this more complete implementation

I don't think it's that bad: I mean, if we'd talk about the sizes of *-packages.nix, then I'd agree (because most editors and LSPs are getting horribly slow then), but this doesn't seem to be the case here. Also, in languages with rather underdeveloped editor integrations I think splitting things off prematurely only makes it harder to find stuff IMHO. I think there's a reasonable boundary though: the current module is responsible for setting up synapse itself (including workers) which is tightly coupled (because e.g. the systemd units are quite similar). If we come up with a good design for setting up load balancing w/ nginx of HTTP locations to workers and/or the main process, then we'd have something that clearly belongs into its own file. Also, if the module becomes too complex eventually, we can also consider moving the options into its own file (right now we wouldn't gain much from it because the majority of the file consists of options).

The only difference left is probably that I use workers.instances and this one does just pure workers like was suggested. This is fine, I can just use autoWorkers or something for part 2

It used to be a level deeper, but I suggested to remove it (there was an enable option as well, but instead workers != {} is used as enable condition now).
And yeah, autoWorkers for more fancy / high-level stuff seems reasonable, that makes the distinction quite clear: one is there to actually define a worker, the other is there to declare what kinds of workers you need (i.e. one level higher) if I understood your proposal correctly. Not sure if there's better naming, but that'd be a detail to figure out then.

I'm still interested in your additional patches (just declaring that one needs N workers doing X and M workers doing Y seems pretty cool. Not sure if we'll find a reasonable abstraction for the nginx part w/o rfc42 for nginx, but we'll see and I'm happy to discuss this in a github issue first if you're not sure - asynchronous discussion is preferred over chat for that).

I think this is a very big step forward now (after supporting synapse for a long time, one can trivially set up workers on NixOS now without knowledge about the inner workings of the synapse module - knowing how to write Nix and (basic) knowledge about synapse configuration is sufficient; it's type-checked, validated & well-documented) and these kinds of ideas are clearly follow-up work, so I'll stick to my plan and merge.

I think Ma27 seemed quite negative about this generally, But personally I think just putting the maps inside services.nginx.commonHttpConfig is more than adequate even without RFC-42 nginx (Which I think is basically impossible without introducing some kind of DAG system similar to home-manager)

But I'll be changing my module to match the nixpkgs implementation and work towards upstreaming the autoconfig stuff

In reply to @hexa:lossy.network
synapse will fail to boot when you configure a list of workers where it expects a just a single one, ask me how I know