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

sigtrap only triggers on INT #1993

Closed
miekg opened this issue Jan 11, 2018 · 6 comments
Closed

sigtrap only triggers on INT #1993

miekg opened this issue Jan 11, 2018 · 6 comments

Comments

@miekg
Copy link
Collaborator

miekg commented Jan 11, 2018

killing caddy (and coredns) should trigger OnFinalShutdown callbacks, while looking at the code (on why this isn't happening), I see https://github.com/mholt/caddy/blob/master/sigtrap.go#L40 is only trapping on SIGINT.

Is this intentional? Add SIGTERM here would make a lot of sense of kill sends this by default.

@mholt
Copy link
Member

mholt commented Jan 11, 2018

Yeah, this is intentional, and is as-documented: https://caddyserver.com/docs/cli#signals -- SIGINT and SIGQUIT execute the shutdown hooks. I suppose this could be changed if necessary, but I just haven't had a good reason to (yet?).

@miekg
Copy link
Collaborator Author

miekg commented Jan 11, 2018

fair enough
(/me should RTFM)

Thanks!

@miekg miekg closed this as completed Jan 11, 2018
@mholt
Copy link
Member

mholt commented Jan 11, 2018

No problem! Let me know if it causes any trouble as-is.

@miekg
Copy link
Collaborator Author

miekg commented Jan 11, 2018

thinking some more. SIGTERM should do this as well, can prolly find in the posix spec. But graceful shutdown usually is done via TERM (and then a KILL).

Kubernetes uses TERM as well, not sure I can find a way to change this for a specific deployment.

@miekg miekg reopened this Jan 11, 2018
@miekg
Copy link
Collaborator Author

miekg commented Jan 11, 2018

Yes, please change; as SIGTERM and then a SIGKILL is pretty normal.

Alternatively I could prepare (or move invasive PR) to makes this configurable, so CoreDNS can behave differerently than Caddy in this regard.

@miekg
Copy link
Collaborator Author

miekg commented Jan 12, 2018

looked at the wrong file. I (only?) need the sigtrap_posix.go on. This is trivial if you want to make TERM call the callback as well.
If not a caddy.<Variable> could be exported that when set will just call fallthrough from the syscall.SIGTERM case.

miekg added a commit to miekg/caddy that referenced this issue Jan 13, 2018
The signal is already trapped; make it do the same thing as SIGQUIT to
be more inline with Unix/Linux shutdown expectations.

Fixes caddyserver#1993
mholt pushed a commit that referenced this issue Jan 16, 2018
* shutdown: allow graceful shutdown for SIGTERM on posix

The signal is already trapped; make it do the same thing as SIGQUIT to
be more inline with Unix/Linux shutdown expectations.

Fixes #1993

* Implement comment feedback ideas
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants