Receiving a SIGUSR2 makes Vault log the running goroutines' stacks. #6240
Conversation
command/server.go
Outdated
| signalCh := make(chan os.Signal, 1) | ||
| signal.Notify(signalCh, syscall.SIGUSR2) | ||
| go func() { | ||
| var buf = make([]byte, 65536) |
There was a problem hiding this comment.
This seems extremely small to me given panics we've seen from live running systems. I'd suggest it needs to be 1MB at the very minimum and should possibly be significantly more, up to 32 or 64MB.
There was a problem hiding this comment.
Fair enough. How would you feel about a new config option? Since we recommend disabling swap I'm reluctant to pre-allocate enough to handle the biggest systems, and then foist that on the smaller ones.
There was a problem hiding this comment.
I don't really see a one-time allocation of somewhere between 1 and say 32MB that can be GC'd almost immediately and is a result of a direct user action as a real burden. Especially since this will almost never be used outside of a direct debugging context with our team.
I forgot you're doing the allocation outside of the signal handler. Just put it inside the for loop and you don't have to have it preallocated if it's not being used.
There was a problem hiding this comment.
I put it outside the for loop thinking that if the user is in a situation where they need to look at stack traces, there might not be memory available to allocate.
There was a problem hiding this comment.
If they're in a state where there is no memory and no swap available they're probably edging on OOM anyways so at that point they can probably SIGQUIT :-)
There was a problem hiding this comment.
In case 64MB or whatever isn't enough.
There was a problem hiding this comment.
I guess if you want. I'd be shocked if 64MB isn't enough. I think in the majority of cases 1MB will be plenty. I think 32MB is a reasonable upper limit in all cases.
There was a problem hiding this comment.
Ok, let's start with 64MB then and we'll see whether it's an issue in practice before adding complexity.
There was a problem hiding this comment.
64 is fine, personally I'd go 32.
|
Is there a reason to put this goroutine where it is instead of simply adding this as a case at https://github.com/hashicorp/vault/pull/6240/files#diff-593cd0d199cb70fe727e6b64eb150baaL1175 ? |
No, that sounds like a good idea. |
|
Actually, now I remember why I didn't put it in the place you suggested. It's hard to get a logger from there. Any suggestions, or should I just leave it where I had it? |
|
I would just use |
signals are handled. Use a 32MB buffer allocated on demand instead of a fixed 64KB buffer.
|
I'd misunderstood, I thought you wanted me to follow the pattern used for the other signal handlers. |
|
Now I'm confused. I see |
|
Apologies for confusing you with my confusion. Fixed, I hope. |
Co-Authored-By: ncabatoff <nick.cabatoff@gmail.com>
| @@ -1238,6 +1239,11 @@ CLUSTER_SYNTHESIS_COMPLETE: | |||
| if err := c.Reload(c.reloadFuncsLock, c.reloadFuncs, c.flagConfigs); err != nil { | |||
| c.UI.Error(fmt.Sprintf("Error(s) were encountered during reload: %s", err)) | |||
| } | |||
|
|
|||
| case <-c.SigUSR2Ch: | |||
| buf := make([]byte, 32*1024*1024) | |||
There was a problem hiding this comment.
You could allocate this only the first time SigUSR2 is called and then just reslice on each call, but I don't think it's a super important optimization.
Fixes #6180.