IslandzVW / halcyon

InWorldz Halcyon 3d virtual reality world simulator
BSD 3-Clause "New" or "Revised" License
21 stars 26 forks source link

Background processing #354

Closed mjheilmann closed 7 years ago

mjheilmann commented 7 years ago

Adding functionality to enable the halcyon grid services to run smoothly in docker containers. Current Grid Service code will sit at 100% CPU usage when idle. I traced this to the interactive console, which cannot block appropriately when launched as a docker service. By adding the "--background true" CLI option that Halcyon already has, the processes do not prompt and idle at 0% CPU.

Experimenting with background modes in the console, I discovered that the ctrl-c does not exist the console. It instead requests the users to enter SHUTDOWN to safely exit the process, or force-terminate the process. When run with the 'background' switch set, the user can only forcefully abort the process. To support non-interactive consoles, and to align with normal ctrl-c to exit console applications on most operating systems, ctrl-c in the console now safely exits the process by calling shutdown.

This pull request has the following changes:

  1. Changes the behavior of ctrl-c to the console that is used for Halcyon, and the grid services. Entering ctrl-c while in a console would print a message asking the user to enter SHUTDOWN to exit the process, or close the terminal to abort the process.

  2. Added a '--background true' command line argument to the GridServer, UserServer, and MessagingServer so they can be started in 'background mode' similar to Halcyon.exe.

appurist commented 7 years ago

Experimenting with background modes in the console, I discovered that the ctrl-c does not exit the console.

This was an intentional change some years ago to prevent the repeated accidental shutdowns by support staff that were happening. This was particularly nasty if it was the central grid services. I can't remember the cause, but it may have been attempts to copy text from the console, or something of that nature.

Is Ctrl-C to shutdown dependent on something new here, such as only triggering a shutdown if it's running in background mode (or under Linux)? Under Linux, it may not be good for a server process to terminate on a SIGINT (Ctrl-C); it may be better to use SIGTERM for that (and perhaps also to recognize SIGQUIT and maybe even SIGABRT) and attempt a graceful but abortive (fast) shutdown. I'm not entirely resistant to this change, but I'm worried here about returning to the days when unintended shutdowns of server apps were common. Requiring a Ctrl-\ on Linux to trigger a shutdown might be better. (Ideally, we eventually move these to Windows and/or Linux services, and require a service shutdown since there wouldn't be an interactive console to press Ctrl-C in anyway.)

Current Grid Service code will sit at 100% CPU usage when idle.

This is only true when run under Docker? Was it launched as a console-less background process? This doesn't seem to be true for our current (non-Docker) usage.

mdickson commented 7 years ago

To get Docker to start a terminal session for the container (so things with a terminal work) you need to add "-i -t" (--interactive to get an interactive session and --tty to allocate a terminal) to the run command when starting it. IDK how this works in a Windows Container or if that's even being contemplated. Without it yes very likely the code that interfaces with the terminal will spin trying to open a connection.

Another option is to use the rest-console code. This then requires no terminal device. I managed to run windows executables as windows services with it under Maestro when I did that development. It could probably use some attention since there have been changes around it recently for remote admin and I have no idea if the console interaction was tested. But its close.

appurist commented 7 years ago

I was preparing tentative release notes which forced me to reread this for the key points, and what I was missing was that the Halcyon server already had this kind of background mode for when it was running headless. Got it. Looks good.

I'm still a bit concerned that we might see an increase in unwanted server terminations, but that is both something admins just shouldn't be doing anyway, and perhaps we should just see if it starts happening again. If it does, we can always change the Ctrl-C to be disabled only when the server isn't running in background mode.

Bottom line: This can be merged at any time now, and I'm already documenting this PR as part of the tentative 0.9.33 update notes...

kf6kjg commented 7 years ago

Simple: if the server's in a non-interative console, then use the TERM signal (aka CTRL-C) to init shutdown. If the local console is active, do the old way of popping a message. After all, if you can see a terminal, you can type shutdown, but if it's REST or background, then signals are the way to go.

appurist commented 7 years ago

@kf6kjg That's the same conclusion I came to when I suggested "we can always change the Ctrl-C to be disabled only when the server isn't running in background mode." But I'm not against giving this a go to see if there are actually any practical cases of accidental termination. My concern with making it conditional is that automated processes may not be aware of whether the server is running interactively or not, and that could increase complexity in trying to determine how to terminate the server. The benefit of having a single method for both cases may outweigh the cost of accidental terminations, even though we did run into that in the past. But that was years ago. So it's a bit of a trade-off and I'm not sure which way that would fall. Let's get this merged and see how it goes?

Edit to add: I guess this is yet another reason to move to a service interface as soon as we get a chance.