-
Notifications
You must be signed in to change notification settings - Fork 58
Add lazymc (on-demand) functionality to nix-minecraft #145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Maybe it would have been better to not use the server-port - 1 and just let the user do all the config themeselves, there is quite a bit of logic bc of that, but there would always be some abstraction because openFirewall would have to automatically open the port in lazymc instead of in serverproperties if it was active. And I do think that having a one click solution like this is more user friendly as long as we can make it work well And if a user sets two mc instances in adjacent ports with lazymc, there is an assertion for that |
|
Please avoid closing and opening pull requests. You can force push to your branch to edit commits. You can change the branch a PR is targeting in the PR edit menu on GitHub. |
|
With regards to the actual content of the PR:
|
|
Thanks for reviewing, I didn't realize that deleting and renaming my repo would also close my PRs, I'll try to keep further changes in this one. |
|
Dont use a llm for anything. opens the project up to legal issues. |
|
Will this eventually be merged with the main branch? This would be an awesome addition cause I'm having a tough time trying to get lazymc working with nix-minecraft manually, but having it built in would be incredible |
|
@CalmSeoul this is a clunky implementation, as it stands it should be rewriten to get merged. I will try to look at it in the summer but i cant promise. I'm not the most proficiant nixer as well. |
bdeefe5 to
95fb235
Compare
|
Rewrote the implementation, sorry for the fuss earlier, no more vibe code now, hopefully it can get merged |
|
I tested this and it works! Good job. Only comment is that, shouldn't lazymc's public port be 25565 and the internal port be 25566? It should be transparent to the end user and not be another port imo. |
|
I thought shifting the port up or down would be kinda arbitrary and code heavy, so I decided to leave the assertion to force the user to always pick their lazymc public port instead. Maybe you're right that that should suggest port 25565, and now that I'm thinking about that, there should probably be another assertion for when the internal server port and the lazymc public port are the same |
|
Or maybe, to do it this way, the right method would be to force the user to set the serverProperties.server-port instead when lazymc is enabled |
|
There you go, I think this is better now if a user goes servers.testerido = {
enable = true;
lazymc.enable = true;
};they'll get: |
Titaniumtown
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments, mostly good!
|
Hi, are there any blockers left preventing this PR being merged? I'm interested in running lazymc with this flake. |
@Titaniumtown @Infinidoge let me know if there are any more blockers, I can try to work on them to get this done! The automatic github action to update package lock files has also been failing recently, not sure why. |
That was recently fixed by #176, which got merged a few hours ago. Also, if you need someone to test it, I can do so, once you've rebased your flake. I'm running a single Paper server. Fair warning, I've never used lazymc before 😛. |
That'd be delightful :), sorry I didn't see your response earlier! |
c80eb05 to
27d2dc5
Compare
|
I just tested it, and it works! My setup is just a single Paper server with lazymc enabled for it, with Velocity as a proxy. I saw a slow connection time the first time I connected to the server (after a while), but every other time, it was fast enough. I think this means that lazymc is working, right? |
if you have time, check if it works with |
What's |
Sorry, It's a nix-minecraft option, seems like it isn't immediately found in the documentation ahaha I saw it in the code, managementSystem.systemd-socket.enable = true;
managementSystem.tmux.enable = false;it'll basically not use tmux, but there will also be no easy way to give input to the server. Either way I did it on my own server and it seems to work as well, so at least on my side everything's working :) |
|
I tried it out, and it works on my system too. EDIT: I just mean that the server runs with lazymc. I tested neither the systemd nor the tmux sockets. |
Infinidoge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am honest, I don't really want this functionality to be baked into the module. This is something the user can theoretically manage for themself. However considering the complexity of that, I am not (yet) rejecting this outright.
With that said, in addition to the comments on files, please rebase out all automated lockfile commits, and change your branch to something other than master. Automated commits pollute the PR. (Note to self, add that to CONTRIBUTING.md)
| "lazymc.toml".value = lib.recursiveUpdate { | ||
| server = { | ||
| command = "${getExe conf.package} ${conf.jvmOpts}"; | ||
| directory = "."; | ||
| address = "127.0.0.1:${toString conf.serverProperties.server-port or 25565}"; | ||
| }; | ||
| } conf.lazymc.config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will create the lazymc.toml file even if not configured. Please add a mkIf guard.
The other values don't have a mkIf guard as their values will be an empty attribute set (or otherwise empty value) when not configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't chuck a mkIf in there, ended up solving it like this:
// (optionalAttrs conf.lazymc.enable {
"lazymc.toml".value = lib.recursiveUpdate {
server = {
command = "${getExe conf.package} ${conf.jvmOpts}";
directory = ".";
address = "127.0.0.1:${toString conf.serverProperties.server-port or 25565}";
};
} conf.lazymc.config;
})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, I don't see why a mkIf wouldn't work.
modules/minecraft-servers.nix
Outdated
| ${ | ||
| if server.lazymc.enable then | ||
| '' | ||
| # Won't gracefully stop if server is running unfortunately |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server must stop gracefully if running.
Stopping while the server is running is an expected situation,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the main problem. I've tried a couple ways but there doesn't seem to be any good way talk with lazymc. Or a cross minecraft version way to check if the server is running (that should be lazymcs job).
If the server is running we can send the stop command through stdin and it will stop. But the service won't quit because when it stops lazymc will keep going... The best way to solve this would probably be for this to get merged to lazymc: timvisee/lazymc#81.
Right now I can try to check if maybe we can see if something still running on the port but I've looked at this a bunch and didn't find any good way to solve this.
Paper and Velocity servers do close properly but that's besides the point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, apparently I can use something like this ps -o state= -p $(pgrep -f java) to check if the server is sleeping or not, I'll try to make use of that to implement graceful exits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm I wouldn't rely on ps and pgrep, especially based on name. ps based on PID would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, after looking a lot more into this I think I can safely say that lazymc does take care of the shutdown process gracefully and we don't need to be messing with it
Apparantly, vanilla servers don't show anything in the log when shut down with a SIGTERM or SIGINT, but it saves everything and exits safely. It's only in windows where the stop command being issued either manually or through rcon is requiered. I thought flavours like papper were working because they don't disable the logger and display that they're exiting normally.
Furthermore I was spooked by exceptions like these that would be thrown if the server was on:
[12:04:04] [Server console handler/ERROR]: Exception handling console input java.io.IOException: Input/output error at java.base/java.io.FileInputStream.readBytes(Native Method) ~[?:?] at java.base/java.io.FileInputStream.read(FileInputStream.java:287) ~[?:?] at java.base/java.io.BufferedInputStream.read1(BufferedInputStream.java:345) ~[?:?] at java.base/java.io.BufferedInputStream.implRead(BufferedInputStream.java:420) ~[?:?] at java.base/java.io.BufferedInputStream.read(BufferedInputStream.java:399) ~[?:?] at java.base/sun.nio.cs.StreamDecoder.readBytes(StreamDecoder.java:350) ~[?:?] at java.base/sun.nio.cs.StreamDecoder.implRead(StreamDecoder.java:393) ~[?:?] at java.base/sun.nio.cs.StreamDecoder.lockedRead(StreamDecoder.java:217) ~[?:?] at java.base/sun.nio.cs.StreamDecoder.read(StreamDecoder.java:171) ~[?:?] at java.base/java.io.InputStreamReader.read(InputStreamReader.java:188) ~[?:?] at java.base/java.io.BufferedReader.fill(BufferedReader.java:160) ~[?:?] at java.base/java.io.BufferedReader.implReadLine(BufferedReader.java:370) ~[?:?] at java.base/java.io.BufferedReader.readLine(BufferedReader.java:347) ~[?:?] at java.base/java.io.BufferedReader.readLine(BufferedReader.java:436) ~[?:?] at aro$1.run(SourceFile:189) [server-1.21.10.jar:?]
And even more like that appear on servers like paper because they seem to have more safeguards, but that happens because we loose the connection to tmux and is harmless as far as I can see
I also noticed that only in unix lazymc suspends the process, and tmux is also suspended, but that's expected behavior, it keeps working as soon as someone enters.
So the logic for exiting when using tmux should be even simpler as lazymc takes care of it. I'll push an update when I can, but the functionality is the same as it's already implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have guessed that lazymc would have had this into consideration, but if this is the case, the sending stop commands etc. functionality of this module can also probably be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplified a bit, mostly left your stop code untouched, sending the two CTRL+C to tmux was redundant so I removed that. But I believe everything is working well now, the exception on lost input device might still happen on the server bc tmux dies first but that's harmless as far as I can see and doesn't affect the shutdown, and for us to prevent that it would be a lot of complicated bash logic to detect server states and PIDs and stuff to know when to close tmux, I was going down that path and trust me it's not pretty. I think how it is now is as good an implementation as we'll get, or at least as I can make
|
Cool thanks! I rebased, but instead of renaming the branch (that would close the PR and we would loose the discussion) I disabled github actions on my fork, hopefully that prevents the clutter, I'll go through the file comments soon as I can |
adaf45d to
b8b92d3
Compare
|
Is there anything else left to do? I'd like to use this without manually using @Yeshey's fork. |
I'll try to explain the situation as it is right now I see why there's friction, but if @Infinidoge sees a path for this to get merged I'd also love to be able to finally get this done |
modules/minecraft-servers.nix
Outdated
| function server_running { | ||
| ${tmux} -S ${sock} has-session | ||
| } | ||
| ${optionalString (!server.lazymc.enable) '' | ||
| function server_running { | ||
| ${tmux} -S ${sock} has-session | ||
| } | ||
| if ! server_running ; then | ||
| exit 0 | ||
| fi | ||
| if ! server_running ; then | ||
| exit 0 | ||
| fi | ||
| ${tmux} -S ${sock} send-keys C-u ${escapeShellArg server.stopCommand} Enter | ||
| ${tmux} -S ${sock} send-keys C-u ${escapeShellArg server.stopCommand} Enter | ||
| while server_running; do sleep 1s; done | ||
| while server_running; do sleep 1s; done | ||
| ''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub is being dumb and not letting me wring a proper suggestion
Instead of wrapping everything in an embedded optional string, just do
stop = optionalString (!server.lazymc.enable) ''
# original shell script here
'';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, fixed, but couldn't that be done in here as well tho, or is there a reason there's an embedded optional string here:
stop = ''
${optionalString (server.stopCommand != null) ''
echo ${escapeShellArg server.stopCommand} > ${escapeShellArg (ms.systemd-socket.stdinSocket.path name)}
while kill -0 "$1" 2> /dev/null; do sleep 1s; done
''}
'';in the ms.systemd-socket.enable part
|
Sorry for the lack of responses, currently midterm season at university so I haven't been able to be active on GitHub. |
freeformType and strMatching
Don't worry I know the feeling, I'm looking into this now instead of sleeping as well |
Solves #142
This adds the options:
Under each instance. This makes servers go to sleep when there are no players and wake up when one player tries to join lazymc
servers.<name>.lazymcIntegrates lazymc, putting server to sleep when idle and waking it upon player connection.
enable:boolean, defaultfalsepackage:package, defaultpkgs.lazymcThe
lazymcpackage to use. You might have to change lazymc version according to your minecraft server version, for example lazymc v0.2.10 supports Minecraft Java Edition 1.7.2+, while for Minecraft Java Edition 1.20.3+ you'll need lazymc v0.2.11config:attribute set, default{}Allows defining and overriding settings in the
lazymc.toml, for all options see here. The auto generated config options are:server.command: Automatically set to use the serverpackageandjvmOpts;server.directory;server.address: Automatically set to127.0.0.1:<serverProperties.server-port or 25565>";Firewall
lazymc.enable = true, theopenFirewalloption for this server instance will open the port specified inlazymc.config.public.address(or its default), not the internal MinecraftserverProperties.server-port.Example: