pterodactyl / panel

Pterodactyl® is a free, open-source game server management panel built with PHP, React, and Go. Designed with security in mind, Pterodactyl runs all game servers in isolated Docker containers while exposing a beautiful and intuitive UI to end users.
https://pterodactyl.io
Other
6.77k stars 1.72k forks source link

Wings mishandles signals #4783

Closed danny6167 closed 4 months ago

danny6167 commented 1 year ago

Current Behavior

Wings currently mishandles stop signals in a number of ways.

^C isn’t actually ^C

^C should issue a SIGINT to the container (as per convention and the panels text) however it is handled specially by the panel. The panel translates this into {‘type’: ‘stop’, ‘value’: null } thus instructing wings to handle what should be a SIGINT as a generic container stop. Meaning when Stop() is called in power.go is skips most of the handling and just calls ContainerStop() with no signal specified. When no signal is specified ContainerStop() defaults to SIGTERM, not the SIGINT that was expected. For some applications this will result in a quick death without completing on exit actions such as saving game.

Most ^SIGNALS don’t work at all

Users are supposed to be able to enter ^SIGABRT, ^SIGINT, ^SIGTERM, etc however issues with wings codes prevents the signal from being processed in a format that is correct for the docker API calls. The syscall.<SIGNAL> is passed to the Terminate() function where it is converted to a string representation, and then (after some trimming) is passed to the ContainerKill() function. The docker API reference (https://docs.docker.com/engine/api/v1.18/#kill-a-container) says the API that this function calls expects a string in the format of “SIGINT”, “SIGTERM”, “SIGKILL”, but my testing also shows that it accepts strings such as “int”, “term”, “kill”. syscall.<SIGNAL>.String() returns strings such as “killed”, “interrupt”, or “terminate”. Current wings code trims the “ed” off “killed” and allows that signal to work, however none of the others are turned into valid strings for the API.

All signals are handled by Terminate()

The intended flow of wings code uses Terminate() to handle all signals however Terminate() make the assumptions that once it sends a signal that the container is dead. Not all signals would result in immediate termination of the container and (once the above issues are fixed) the container can stick around long after wings has marked the container as stopped.

Expected Behavior

The correct signal should be sent to the container. Wings should not assume that a container stopped after a signal was sent to it.

Steps to Reproduce

Use different signals as stop actions and observe signals received by the container using bash traps. Also witness errors in wings logs for ^SIGINT, ^SIGABRT, ^SIGTERM

Panel Version

1.11.3

Wings Version

1.11.6

Games and/or Eggs Affected

No response

Docker Image

No response

Error Logs

Output when using ^SIGINT
------
Stacktrace:
Error response from daemon: Cannot kill container: 5839fcb0-190e-4edf-88d1-605a657c2578: invalid signal: interrupt
github.com/pterodactyl/wings/environment/docker.(*Environment).Terminate
        /root/wings/environment/docker/power.go:302
github.com/pterodactyl/wings/environment/docker.(*Environment).Stop
        /root/wings/environment/docker/power.go:165
github.com/pterodactyl/wings/environment/docker.(*Environment).WaitForStop
        /root/wings/environment/docker/power.go:233
github.com/pterodactyl/wings/server.(*Server).HandlePowerAction
        /root/wings/server/power.go:141
github.com/pterodactyl/wings/router/websocket.(*Handler).HandleInbound
        /root/wings/router/websocket/websocket.go:363
github.com/pterodactyl/wings/router.getServerWebsocket.func3
        /root/wings/router/router_server_ws.go:85
runtime.goexit
        /usr/lib/go-1.18/src/runtime/asm_amd64.s:1571

Is there an existing issue for this?

yahell100 commented 1 year ago

I've got the same issue with my panel, Running the same versions for panel and wings. Docker is on version 24.0.2

parkervcp commented 1 year ago

My best guess is still that this is related to the fact that we use eval instead of exec in the entrypoint. It just took this long to get to where it was noticeable.

danny6167 commented 1 year ago

My best guess is still that this is related to the fact that we use eval instead of exec in the entrypoint. It just took this long to get to where it was noticeable.

The use of eval is an issue, but it is separate to the issues described in this report. The issues I've reported are entirely valid on their own. Swapping to an image that uses exec will result in the signal reaching the process, but you will still only ever be able to send it a SIGTERM due to the issues I reported in this issue.

I've had a few people say it worked a few versions back but nobody could tell me exactly what version they claim it broke. My best guess is that are referring to the version before https://github.com/pterodactyl/panel/issues/4555 was fixed. If people aren't really understanding what's going on they would have seen the container stop and believe it was working as expected.

gOOvER commented 1 year ago

I've had a few people say it worked a few versions back but nobody could tell me exactly what version they claim it broke. My best guess is that are referring to the version before #4555 was fixed. If people aren't really understanding what's going on they would have seen the container stop and believe it was working as expected.

It worked with the last version from Dane. After Matthew starts with updates, it stop working. But it could also be a Docker update. So according to my feeling after

But it is absolutely slowly a NoGo, that Matthew here nothing fixed or times writes something to it.

likes to take the donations and then sticks his head in the sand. Just my 2 cents

danny6167 commented 1 year ago

It worked with the last version from Dane. After Matthew starts with updates, it stop working. But it could also be a Docker update. So according to my feeling after

Just to clear, you're saying Panel version 1.10.1 with wings 1.7.0 is the Pterodactyl release you claim worked as you expected it to ? If so, I'll spin up an environment running this version in the next couple of days and test it.

But it is absolutely slowly a NoGo, that Matthew here nothing fixed or times writes something to it.

likes to take the donations and then sticks his head in the sand. Just my 2 cents I can understand how some might feel that way, and I totally respect that opinion, but leaving it out of the issue tracker may be more productive for us all.

We have a half ready patch that resolves these issues, Once I can get it clean and PR ready Ill post it.

gOOvER commented 1 year ago

It worked with the last version from Dane. After Matthew starts with updates, it stop working. But it could also be a Docker update. So according to my feeling after

Just to clear, you're saying Panel version 1.10.1 with wings 1.7.0 is the Pterodactyl release you claim worked as you expected it to ? If so, I'll spin up an environment running this version in the next couple of days and test it.

But it is absolutely slowly a NoGo, that Matthew here nothing fixed or times writes something to it. likes to take the donations and then sticks his head in the sand. Just my 2 cents I can understand how some might feel that way, and I totally respect that opinion, but leaving it out of the issue tracker may be more productive for us all.

We have a half ready patch that resolves these issues, Once I can get it clean and PR ready Ill post it.

problem is, Matthew is the only one, which can merge sth

danny6167 commented 1 year ago

Yep, it know that's a pain. I'd love to see my unrar fix merged as well.

I'm happy to provide a build myself that includes my fixes when my PR is ready.

gOOvER commented 1 year ago

Yep, it know that's a pain. I'd love to see my unrar fix merged as well.

I'm happy to provide a build myself that includes my fixes when my PR is ready.

im happy to help you testing it :)

But even if nobody wants to hear it and I make myself unpopular every time; @matthewpi is the wrong man for this project. Any sponsor should immediately stop supporting it (unless there is a secret project where sponsors get fixes, which I don't really believe).

He should think about handing over the project to someone who is willing to keep it going, or the community should fork the whole thing and continue without Matthew. With Matthew the project is doomed to die and this project is too bad for that. Many people have invested time here and just because one person is not interested, it should not die.

gOOvER commented 1 year ago

It worked with the last version from Dane. After Matthew starts with updates, it stop working. But it could also be a Docker update. So according to my feeling after

Just to clear, you're saying Panel version 1.10.1 with wings 1.7.0 is the Pterodactyl release you claim worked as you expected it to ? If so, I'll spin up an environment running this version in the next couple of days and test it.

sorry; had overlooked that. It could also be a docker update; they changed something with the signals in 22 ? or 23?; but there we come out again on it, that it is a bug with ptero.

danny6167 commented 1 year ago

It worked with the last version from Dane. After Matthew starts with updates, it stop working. But it could also be a Docker update. So according to my feeling after

Just to clear, you're saying Panel version 1.10.1 with wings 1.7.0 is the Pterodactyl release you claim worked as you expected it to ? If so, I'll spin up an environment running this version in the next couple of days and test it.

sorry; had overlooked that. It could also be a docker update; they changed something with the signals in 22 ? or 23?; but there we come out again on it, that it is a bug with ptero.

Thank you for the additional info, I'll incorporate that version of docker into my test environment when I can, but I'm a little swamped next few days.

danny6167 commented 1 year ago

I have a patch + build here for testing/comments https://github.com/danny6167/wings/releases/tag/signal-test Keep in mind that only resolves the issue of the incorrect signal being sent to the container - Your images must also handle the signal properly by using exec, or an init system, or other valid signal handlers.

@gOOvER tagged because I know you wanted to test

danny6167 commented 6 months ago

Reopening as I haven't seen anything that fixes this. I'll reconfirm and write a PR for it next week.

bobbyl140 commented 3 months ago

@danny6167 I notice that this was merged 3 weeks ago and I wanted to ask if it's in the latest release of Wings. When I run docker stop on a container while observing its console on the Panel, it's detected as crashed and I see no output from the game server indicating a graceful stop. I just installed Pterodactyl last night.

If this isn't in the release yet, how would I get support for this? I'm hoping to migrate my Minecraft servers to Pterodactyl but I need the guarantee of graceful shutdowns if I simply run shutdown on my Linux host.

danny6167 commented 3 months ago

@danny6167 I notice that this was merged 3 weeks ago and I wanted to ask if it's in the latest release of Wings. When I run docker stop on a container while observing its console on the Panel, it's detected as crashed and I see no output from the game server indicating a graceful stop. I just installed Pterodactyl last night.

If this isn't in the release yet, how would I get support for this? I'm hoping to migrate my Minecraft servers to Pterodactyl but I need the guarantee of graceful shutdowns if I simply run shutdown on my Linux host.

Hey @bobbyl140, This hasn't made it to a released version yet (hopefully soon) but this doesn't have anything to do with how docker stops a container when using docker stop or during system shutdown.

I haven't been working on a shutdown hook script that would do what you want, no guarantees, but if it ever reaches a half usable state I'll let you know.

bobbyl140 commented 3 months ago

Ah, I’m sorry. If it’s okay to ask, is there anything I could programmatically call to safely shut everything down? Or if I was to try making one myself, where could I start?

danny6167 commented 3 months ago

My solution I'm building is using systemd shutdown triggers, that calls a script that calls the wings API directly (to wings, not the panel) to request a shutdown and poll till they are all stopped.

bobbyl140 commented 3 months ago

I see, thank you!