Skip to content
This repository has been archived by the owner on Oct 28, 2023. It is now read-only.

warn players before stopping the container #4

Merged
merged 2 commits into from
Jun 5, 2020
Merged

warn players before stopping the container #4

merged 2 commits into from
Jun 5, 2020

Conversation

catzoo
Copy link
Contributor

@catzoo catzoo commented Jun 4, 2020

I wasn't sure if everyone wanted this.

This pull request will add a --warn to the stop function. This will warn players when you're stopping the container, rather than stopping the whole server without a warning.

@thmhoag thmhoag self-requested a review June 5, 2020 04:21
@thmhoag
Copy link
Owner

thmhoag commented Jun 5, 2020

Hey @catzoo, I think a warning here would be great. However, not sure if we should use the --warn flag here.

The catch is, the --warn flag will delay the server stop by the arkwarnminutes specified in the arkmanager.cfg. Since this function is executing after catching a SIGTERM, that means a container stop has been requested (likely using docker stop or docker restart). By default, Docker will only wait 10s before killing the container. This can lead to an odd situation where players in the server would get a warning that a shutdown is occurring in "x minutes" (based on the arkwarnminutes setting) but then the container would abruptly get killed anyway after 10s.

I think what we could do here is add an arkmanager broadcast instead, just before executing arkmanager stop, notifying the players that an immediate shutdown was required. Maybe use an environment variable on the container to customize the message that would be sent.

@thmhoag thmhoag added the enhancement New feature or request label Jun 5, 2020
@catzoo
Copy link
Contributor Author

catzoo commented Jun 5, 2020

I see. I didn't know docker forced it to close after 10 seconds.

I removed --warn, added broadcast and notify. Doing both since broadcast will let the users know in the server and, notify will let the users in discord know the server shut down.

Copy link
Owner

@thmhoag thmhoag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, good catch on the notify!

LGTM. I'll merge and cut a release.

@thmhoag thmhoag merged commit 38685e6 into thmhoag:master Jun 5, 2020
@thmhoag
Copy link
Owner

thmhoag commented Jun 5, 2020

@catzoo fyi, this should be in the new thmhoag/arkserver:1.4.1 image. Thanks for the contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants