chore(#9585): teach Sentinel sign language #9658
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
hey @garethbowen . Wanna do a PR review for a change? I've added signals to sentinel so we don't need to stop and start it. Appreciate your time and your ideas :) |
dianabarsan
requested review from
garethbowen and
latin-panda
and removed request for
latin-panda
|
Also fat fingered @latin-panda . Sorry for the noise! |
| @@ -1562,6 +1553,9 @@ const isMinimumChromeVersion = process.env.CHROME_VERSION === MINIMUM_BROWSER_VE | |||
|
|
|||
| const escapeBranchName = (branch) => branch?.replace(/[/|_]/g, '-'); | |||
|
|
|||
| const toggleSentinelTransitions = () => sendSignal('sentinel', 'USR1'); | |||
There was a problem hiding this comment.
I'm not sure about toggle. Wouldn't it be better to have an explicit start and an explicit stop? It would be very easy to accidentally call toggle twice (eg: test retry) which leaves sentinel in an unexpected state. This would be very hard to debug because the double call could be in one test, but the failure only show up many tests later. I think having two separate signals would make it explicit. Then we can decide if calling startProcessing when already processing would be ignored, logged, or throw an error because someone forgot to clean up.
There was a problem hiding this comment.
As above, we can't send custom signals.
|
|
||
| module.exports = { | ||
| init: () => { | ||
| process.on('SIGUSR1', () => { |
There was a problem hiding this comment.
I struggled to find any documentation on this but I think we can call the signals whatever we want, right? If so let's use clearer names. What about SIGLISTEN, SIGSTOPLISTENING, and SIGRUNTASKS? Or even SIGSENTINELLISTEN etc? It's getting long but at least it'll be clear that it's a custom signal and not a Node thing?
Also I think SIGUSR1 is reserved and according to this documentation will trigger Node into debugging mode: https://nodejs.org/en/learn/getting-started/debugging - so if we can't rename as above, let's use a different number at least.
There was a problem hiding this comment.
Unfortunately, you can't send custom signals:
➜ ~ kill -s SIGRUNTASKS 26360
kill: unknown signal: SIGRUNTASKS
kill: type kill -l for a list of signals
So you need to select one of the signals in the list. There are some random ones that might not kill the process, but most do, and it felt weird to send a SIGURG to stop feed listening.
This is why there's a toggle to stop/start processing, because we can't send custom signals and there are only two which are "user customizable" - SIGUSR1 and SIGUSR2, which I am using.
There was a problem hiding this comment.
Ugh that's disgusting. Ok. The only other way I can see to do it would be to have a single signal which then looks somewhere else (file or db) to reload config, but that almost defeats the purpose!
Let's go with what you have.
Yea. Sometimes API fails to start. No repro locally and because the test times out, we don't get an artifact for logs. |
Description
No longer start and stop sentinel to stop/start transitions and run scheduled tasks. Instead make it listen to two kill signals.
Kill the test process if API is down in the before hook. This way we might get logs when the process ends up hanging.
#9585
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.