Skip to content
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

Wings mishandles signals #4783

Closed
3 tasks done
danny6167 opened this issue May 21, 2023 · 17 comments · Fixed by pterodactyl/wings#192
Closed
3 tasks done

Wings mishandles signals #4783

danny6167 opened this issue May 21, 2023 · 17 comments · Fixed by pterodactyl/wings#192
Assignees
Labels
bug Something that's not working as it's intended to be.

Comments

@danny6167
Copy link
Member

danny6167 commented May 21, 2023

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?

  • I have searched the existing issues before opening this issue.
  • I have provided all relevant details, including the specific game and Docker images I am using if this issue is related to running a server.
  • I have checked in the Discord server and believe this is a bug with the software, and not a configuration issue with my specific system.
@danny6167 danny6167 added the not confirmed Report seems plausible but requires additional testing or 3rd part confirmation. label May 21, 2023
@yahell100
Copy link

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
Copy link
Member

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
Copy link
Member Author

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 #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

This comment was marked as abuse.

@danny6167
Copy link
Member Author

danny6167 commented Jun 24, 2023

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
Copy link
Contributor

gOOvER commented Jun 24, 2023

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
Copy link
Member Author

danny6167 commented Jun 24, 2023

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

This comment was marked as abuse.

@gOOvER
Copy link
Contributor

gOOvER commented Jun 24, 2023

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

@danny6167 danny6167 reopened this May 5, 2024
@danny6167 danny6167 added bug Something that's not working as it's intended to be. and removed not confirmed Report seems plausible but requires additional testing or 3rd part confirmation. labels May 5, 2024
@danny6167 danny6167 self-assigned this May 5, 2024
matthewpi pushed a commit to pterodactyl/wings that referenced this issue Jun 29, 2024
dannyhpy pushed a commit to dannyhpy/pterodactyl-wings that referenced this issue Jul 5, 2024
@lyra56k
Copy link

lyra56k commented Jul 19, 2024

@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
Copy link
Member Author

@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.

@lyra56k
Copy link

lyra56k commented Jul 22, 2024

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
Copy link
Member Author

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.

@lyra56k
Copy link

lyra56k commented Jul 22, 2024

I see, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as it's intended to be.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants