Add systemd instantiated service file and per-session environment files #4
Conversation
fuhry
force-pushed
the
systemd-instantiated-support
branch
from
cb7503b to
b6a3784
Compare
|
Testing notes: I have been using the server wrapper scripts on my personal minecraft setup for several weeks without issue, but I have not had a chance to test the backup scripts - my minecraft server lives on a ZFS volume so I have my own backup process. |
|
Thank you very much for this contribution! Supporting multiple servers has been a recurring issue. However, I have two objections regarding this or similar approaches:
While I still think one should use containers to orchestrate multiple minecraft servers, I am fine with disregarding this topic for now and will merge and help with implementing a patch that fulfills my second objection. I think an implementation should provide an easy command line switch to specify the targeted minecraft session (e.g. a |
|
Thanks for the feedback! I think having separate configuration files is not only acceptable, but ideal and better aligned with traditional unix design. For me personally, I am managing my Minecraft instances with puppet, and individual configuration files translates to fewer global restarts. Moreover, I think trying to implement an ini parser in bash is just asking for bugs. So I'm pretty set on keeping an instance's configuration within its own environment file. I'm more open to where to put that file though. As for file paths, we should be careful not to step on any other packages or tightly couple to any particular filesystem scheme. This was my reasoning for looking for an environment file in I'll spend some time this evening adding a commandline switch to Containerizing is probably way out of scope for this project. It sounds like we're on the same page with this. There could be a way to achieve the stated security goals without moving to a fully containerized solution thoughl using systemd's Sandboxing options. This allows for directories on the system to be set as read-write, read-only or inaccessible entirely. While this doesn't provide the same level of guarantees as running the server in a container, and furthermore relies on the server being started from systemd to achieve the additional security guarantees, it's better than nothing. |
|
I am glad to see that you intend to follow up on this! :) Maybe it is just me but I never was too fond of having the configuration file be in bash. I think an ini parser should be pretty simple but I fully agree that it could be another source of error for a number of future bugs if not done correctly. Just to be clear though, just sticking to bash for now is absolutely fine! I am indifferent to having sections versus different files. I agree that a system with individual files for servers is easier to manage for a large number of servers. I did not meant to suggest to hardcode Having the configuration file right next to the server yields two problems. First of all, if it is not named in a safe way, it might collide with files from the server. Secondly, it increases the scope from where variables are read in without being obvious to the user. Especially the last part seems extremely problematic to me. Currently, the server management script is very minimalistic and fully documented by its help page. Small behavioral niceties that are not documented are standard behavior of many UNIX or GPL tools (configuration in |
|
Hey, I haven't forgotten about this, the week has just gotten away from me with work etc. I should have some time to update this weekend. Thanks again for looking everything over! |
fuhry
force-pushed
the
systemd-instantiated-support
branch
2 times, most recently
from
a3dded6 to
aae9aaf
Compare
Currently the management scripts allow for only a single instance of the minecraft server to be run, but it's actually not hard at all to support multiple instances. This commit makes the following changes: - Adds a new systemd service file, `[email protected]`, which is an instantiated variant that overrides `SERVER_ROOT` and `SESSION_NAME` - Modifies `minecraftd.sh` to support overriding the path to the environment file via the `CONFIG_PATH` environment variable. The instantiated variant of the systemd unit also uses the environment file `/etc/@GAMe@/%i` if it exists. - Instantiated versions of the minecraftd-backup service and timer. - Enables systemd's sandboxing at the instance level. A running server instance cannot modify backups or data/config/etc. files for other worlds. The intended use case for this change is fairly common: using the same package to run multiple instances of a minecraft server on a single host. This would commonly be seen with, for example, multi-server bungeecord deployments. Documentation is included. Miscellaneous improvements: - Fixed a few locations in the startup script that did not have `${SUDO_CMD}` - Added a `.gitignore` file for generated files Supporting changes: - The path to the tmux socket has been changed. It now lives under `/run/@GAMe@/tmux`. This allows it to be bind mounted inside the per-instance namespace. Testing performed: - Clean install, new server - single - Clean install, new servers - instantiated - Clean install backup - single - Clean install backup - instantiated - Production system, upgrade - instantiated
fuhry
force-pushed
the
systemd-instantiated-support
branch
from
aae9aaf to
f9c55cc
Compare
|
We are almost there. Currently having issues achieving both isolation between instances, and correct startup behavior of creating instances on first use. I will probably need to split it into 2 units, the first being a dependency that is non-isolated. Yuck. |
|
If I should do a review of anything, just ping me. Glossing over the code, I already have one point which I would like to reiterate, namely I do not like advertising the hackish approach of controlling individual users via environment variables. There should be a convenient command line switch to do that. |
I'm assuming you mean controlling individual servers via environment variables - please correct me if I'm wrong! That's totally fair. In my work tree I actually went crazy implmenting something like python's argparse in bash, in order to accept the instance name as a commandline argument which would override the default Not sure which approach I'll take at this point, but the end result (being able to pass the instance on the command line) will be the same. |
add a real shell argument parser, add --instance arg, documentation, refactoring, cleanup
fuhry
force-pushed
the
systemd-instantiated-support
branch
from
88a9376 to
d6d6c0d
Compare
|
@Edenhofer I spent a bit more time this evening working on this, and I got it all working. I did change how embedded variables are done, pivoting from There's quite a bit more changed but the core functionality has remained the same and I believe the executable's command line is also backward compatible to existing usage. Also parametrized the server memory setting... I think it's ready to be looked over. I'll be honest, I'm not the most proud of the argument parsing stuff or how the config variable layering is done but it makes the most sense from a management standpoint to do it this way. And finally, I can create a new server instance with just |
|
This patch is pure awesomeness! I really like how you paid attention to backwards compatibility and overall preserving the flexibility of the script. With all the additional fencing in the service files I now actually feel confident in recommending people to use multiple instances outside of containers.
Yes, I meant servers :|
TBH I think you went a bit overboard with your argparse implementation :) Previously I was thinking about just checking whether the very first argument specifies the instance via a long or short option, i.e. check for Having said that, I am overall still happy with your implementation. Though one really feels that bash is not the right tool here for this kind of stuff...
Using overriding lists seems much nicer but I don't like the way variable substitution works now. See my review.
Backwards compatibility is very important and I think it is super cool that you paid special attention to it. However, I am a bit torn on whether it might be better to make the default server just another instance itself. This would make the directory structure in
I don't like this option because it is not sufficiently generic. For example, cuberite can not make use of these settings.
I just looked over the code (without testing it thoroughly just yet though) and overall this patch looks really really nice! |
| SERVER_MEMORY_INITIAL = 512 | ||
| SERVER_MEMORY_MAXIMUM = 1024 |
There was a problem hiding this comment.
These flags are not meaningful for servers which do not allow for configuring the memory via the command line.
There was a problem hiding this comment.
I'll need to look into this further. You mentioned cuberite which I am not familiar with. -Xms and -Xmx are arguments to the JVM executable so they should work regardless of what server implementation is being used. Can you help me understand what I'm missing?
There was a problem hiding this comment.
Yes all java based implementations should honor this flag or more precisely, it will be enforced by the JVM. However, cuberite is a C++ rewrite and does not care about any of this.
| OBJECTS = $(SOURCES:.in=) | ||
|
|
||
| GAME = minecraft | ||
| INAME = minecraftd | ||
| SERVER_ROOT = /srv/$(GAME) | ||
| CONFIG_PATH = /etc/conf.d/$(GAME) |
There was a problem hiding this comment.
If you swap this line with the one below, you can use SYSCONFDIR in CONFIG_PATH.
| OBJECTS = $(SOURCES:.in=) | ||
|
|
||
| GAME = minecraft | ||
| INAME = minecraftd | ||
| SERVER_ROOT = /srv/$(GAME) | ||
| CONFIG_PATH = /etc/conf.d/$(GAME) | ||
| SYSCONFDIR = /etc | ||
| INSTANCE_CONFIG_DIR = $(SYSCONFDIR)/$(GAME) |
There was a problem hiding this comment.
What do you think about merging CONFIG_PATH into INSTACE_CONFIG_DIR. IMO it would make more sense to have one central directory where to configure everything at once.
Specifically what do you think about /etc/minecraftd and /etc/minecraftd.d (respectively with INAME and config dir)? IMHO this would be a neat solution.
| ExecStart=/usr/bin/@INAME@ -i %i init | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target |
There was a problem hiding this comment.
Missing new line at end of file
| local varname="$3" | ||
| local help="$4" | ||
| local required="${5:-false}" | ||
| [ -v 6 ] && local default="$6" |
There was a problem hiding this comment.
indentation, plus missing double brackets
| if [[ "$varname" =~ $re_subcmd_vn ]]; then | ||
| local subc _vn | ||
| IFS=":" read subc _vn <<< "$varname" | ||
| [[ -n "${subcmds[$subc]}" ]] |
There was a problem hiding this comment.
Is this an implicit test that this subcommand is defined? If so please terminate explicitly here instead of relying on set -u.
| @@ -0,0 +1,12 @@ | |||
| [Unit] | |||
There was a problem hiding this comment.
Is this service file really necessary? It runs exactly once and is not needed afterwards, right?
There was a problem hiding this comment.
I tried everything, but unfortunately, systemd's sandboxing affects all ExecStartPre= commands too.
Instead of this approach, we can update the launcher script to exit with an error after instructing the user to run the init command themselves prior to attempting to start a new server instance. They need to create a eula.txt file anyway, so that's not too terrible. Which do you prefer?
There was a problem hiding this comment.
Personally, I would much rather print an error and require user intervention than have a systemd service lying around that runs exactly once.
| @@ -0,0 +1,189 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Please just inline this script. It is pretty fancy and not specifically related to the minecraft server. However, in its current form it still couples tightly to it. Furthermore, I really like that the server is just a single bash script without any additional library files.
There was a problem hiding this comment.
This file adds a lot of LOC and I don't feel that it's tightly coupled. Can you share why you said it couples tightly to the launcher script?
With the launcher script adding a lot of LOC in this change, do you think it makes sense to go the opposite direction, and break out the launcher script into individual files? It's not yet at the point where it really needs that, but if it keeps growing that will eventually be a tech debt item and maybe it could be better to just get that out of the way now. Completely up to you though.
There was a problem hiding this comment.
DESCRIPTION and COPYRIGHT are just assumed to be set in the calling script, and the signature of the management script is not that common in other CLI tools. Thus, compared to tools like getopt or getopts this feels somewhat more tightly coupled.
I would much rather have one larger script than many small libraries stuck together (at least in bash). The scoping in bash is a mess and I find it easier to grasp larger bash scripts. I don't think we have hit the point yet that the script has become unwieldy.
| for var in ${!default_config[@]}; do | ||
| value="${environment_overrides[$var]:-${!var}}" | ||
| for ivar in ${!default_config[@]} INAME GAME_USER; do | ||
| value="${value//@$ivar@/${!ivar}}" |
There was a problem hiding this comment.
This feels wrong. Please don't do that!
There was a problem hiding this comment.
See above - I am open to going back to eval.
eval brings with it some security implications whereas this is safe.
However, I'm also using source on ${CONFIG_PATH} (here) and ${INSTANCE_CONFIG_PATH} (here) without an ownership or permissions check, which is the same level of unsafe.
Should we:
- Get rid of this style of substitution and use
eval? or - Take the more security-minded approach and add an ownership + permissions check to ensure only root and
@GAME_USER@can write to the config paths? or - Something else? Again I'm open to suggestions.
There was a problem hiding this comment.
The only place eval-style evaluation was used up until now, was in IDLE_SESSION_NAME and this variable will cease to exist anyway and I am very happy about that. Personally I would much rather have a configuration that does not allow for this.
As outlined in this comment, I dislike adding java specific environment variables. I am happy to see any kind of eval or inplace substitution being removed from the script.
Your security concerns are very valid though! Checking for the permissions sounds like a great idea! I don't know whether you want to implement that too but I would welcome a much simpler key-value storage too which extracts the values via a simple regex.
|
@Edenhofer Sorry I did not get notified of this review for 2 weeks. I'm traveling at the moment, but I have some time over the next couple of weeks to address your comments above. Anything marked with a 👍 I will change without further discussion, 👀 means I need to look at it but should not need further input from you, and I've left comments on anything where I feel adopting your suggestions as written could deviate from the other stated goals (backward compatibility, etc.) so further discussion is warranted. Thanks for the thorough review! I'll be able to focus on this more over the next couple of days. |
|
No worries! The patch already looks quite mature. I'll be on vacation the next three weeks myself so I might respond with some delay. |
|
I would really like to see this get merged soon-ish! The new features are super useful and the implementation is almost there. Is there anything I can help you with to get this done? |
|
Hi @Edenhofer - apologies again, my day job has been pretty busy and anything minecraft related has taken a back seat. My server is also quiet over the summer (I run it for my youth group and many of them are on vacation, etc.). Regarding the heap size options - what if we just have a build time option to remove any mention of those from the script output? That way we still make it easy for users of java servers to change the heap size, and cuberite just needs that option set - and even that's done in such a way that the package doesn't break if this isn't done, those options are just ignored. |
|
No worries! As you can imagine developing this minecraft management script is not part of my day job either :) I am very much in favor of having no java specific options not even if they are removed during the build process. IMO having to change the whole invoked command line is not big enough of a hassle to warrant introducing new flavor specific options. |
Currently the management scripts allow for only a single instance of the minecraft server to be run, but it's actually not hard at all to support multiple instances. This commit makes the following changes:
[email protected], which is an instantiated variant that overridesSERVER_ROOTandSESSION_NAMEminecraftd.shto support sourcing anenvironmentfile fromSERVER_ROOT, in addition to the existing system-wide file/etc/conf.d/GAME.The one potentially unsafe assumption I'm making is that the
MAIN_EXECUTABLElives in the original compile-timeSERVER_ROOT, which I know is the case for at least papermc and spigot. For installations where this is not the case, the environment file can overrideSERVER_START_CMD.The intended use case for this change is fairly common: using the same package to run multiple instances of a minecraft server on a single host. This would commonly be seen with, for example, multi-server bungeecord deployments.