Infinidoge / nix-minecraft

An attempt to better support Minecraft-related content for the Nix ecosystem
MIT License
245 stars 26 forks source link

module: allow management via systemd socket #67

Closed frantisekhanzlikbl closed 1 week ago

frantisekhanzlikbl commented 3 months ago

Supersedes #52. Partially fixes #63 (per-server user & group configuration missing).

This PR adds a way to make servers managed via a systemd stdin socket and the journal, allowing for a better integrated experience with the rest of the system.

This PR largely builds on top of @Misterio77's excellent #52 and retains the nice additions:

I did change the default systemd socket path from #52 to be aligned with the current tmux socket path and renamed the lifecycle hooks to preserve the systemd naming convention, though.

I've added a few extras, not all of which I'm sure want to be kept. Feedback welcome. :slightly_smiling_face:

I'm not too sure I like the way server overrides of the global management system config works, but it was all I could conjure with my limited knowledge of advanced usage of the module system.

This PR should be backwards compatible, although some field-testing would be much appreciated as I don't have any real setup to test it on (and frankly no time either :)).

frantisekhanzlikbl commented 3 months ago

Thanks so much for the review!

I just pushed the requested changes. Please let me know if there is something else to change.

frantisekhanzlikbl commented 3 months ago

Hmm, I just tried to migrate my config to this and got bitten in my ass by the extra lifecycle hooks not having set -euo pipefail, therefore "succeeding" even if parts of them fail. Would you mind me using writeShellApplication instead of writeShellScript for them?

This would theoretically be a breaking change, as setups previously "working" might now start failing, but I'd argue that these were broken from the start. I could add an option for that, but I don't think it would be worth the extra code and maintenance.

frantisekhanzlikbl commented 3 months ago

Alright, I just pushed the next iteration. Sorry about the period change in the changelog, that might've been a little too pedantic of me. I updated the descriptions of socket path options and moved to writeShellApplication.

I thought about using a string enum for the preferred management system too, but decided against it, as it would make it non-practical to add more management systems (such as RCON), that don't conflict with others. i.e. both systemd-socket and tmux could be used along with RCON, just not with each other.

I kept the language-level assertions for now, for the reasons described in my comment on the appropriate review comments, but I don't feel that strongly about the first one. If you do, I can change that.

frantisekhanzlikbl commented 3 months ago

I just remembered that systemd has its own options for timeouts, so I pushed a new commit that lets systemd handle server stop timeouts instead of the ad-hoc implementation that was in place previously in the systemd-socket management system (and completely missing in the tmux one).

marek-rychly commented 1 week ago

What is the status of this merge request? Can it be merged?

Logging into journald is much more better than tmux for monitoring a server, so I am using the last commit from this merge request now. Unfortunately, there are newer fabric versions in the master branch which are not available here. Rebase or merge to the master would be appreciated. Thank you.

frantisekhanzlikbl commented 1 week ago

What is the status of this merge request? Can it be merged?

Logging into journald is much more better than tmux for monitoring a server, so I am using the last commit from this merge request now. Unfortunately, there are newer fabric versions in the master branch which are not available here. Rebase or merge to the master would be appreciated. Thank you.

I believe all the brought up concerns have been resolved on my side.

I've been using the latest commit here on a few small-scale servers in production for about three months and have faced no problems, but my deployments are also probably fairly simple.

Currently, I'm severely short on time, so if @Infinidoge has more change requests before this is mergeable, the PR will most likely have to wait for a while or get taken up by somebody else. I'll do a rebase in the meantime, though.

Infinidoge commented 1 week ago

Note to self: GitHub action that runs nix flake check

frantisekhanzlikbl commented 1 week ago

thanks for the reviews and merge, it is greatly appreciated! :)

Misterio77 commented 1 week ago

Thanks so much for picking this up! Looks awesome, great work!