cli: forward SIGTERM to child process of 'lock' and 'watch' subcommands #4737
Conversation
…ds on unix This also removes the signal handler for SIGKILL as it's impossible to receive these signals.
agent/util_test.go
Outdated
|
|
||
| func TestForwardSignals(t *testing.T) { | ||
| for _, s := range forwardSignals { | ||
| testForwardSignal(t, s) |
There was a problem hiding this comment.
I would wrap each one of these in a subtest t.Run() with the signal name/number.
There was a problem hiding this comment.
I totally forgot about t.Run. I'll also remove the testname log prefix stuff as it's not needed to understand the failures now.
agent/signal_unix.go
Outdated
| "syscall" | ||
| ) | ||
|
|
||
| var forwardSignals []os.Signal = []os.Signal{os.Interrupt, syscall.SIGTERM} |
There was a problem hiding this comment.
nitpick but I don't think you need the first []os.Signal here. Surprised fmt doesn't remove this actually.
|
This looks good to me although the test fail was 3 consecutive 8/9min timeouts which I've never seen before. I've restarted that job to see if there was just a bad moment for CPUs on Travis when that ran but it's possible that one of these tests actually blocks now because something is not being sent the right signal etc. Did you see agent package to pass locally? |
|
Nevermind it passed on a retry 🎉 🤦♂️ |
|
Updated PR with feedback. |
|
😍 |
This PR ensures that the
consul lockandconsul watchsubcommands forward the SIGTERM signal to the child process that they supervise.This also removes the existing forwarding logic for SIGKILL as it's impossible to receive these signals.
Fixes #3754