fix: add timeouts to all production requests calls - #2042
Conversation
Without explicit timeouts, any requests.post() or requests.get() call can block indefinitely when a remote server hangs. This caused the ingester on db.kernelci.org to stall for 14 hours on a hung HTTP upload, leaving 543k submission files unprocessed. Fixes: kernelci#2041 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tales da Aparecida <tales.aparecida@redhat.com>
6de37d1 to
0d82e38
Compare
|
@felipebergamin can I merge this? I'm unsure what's the current process |
|
I will merge, i believe it is emergency measure/hotfix. I hope i can be deployed to production ASAP(today). |
mentonin
left a comment
There was a problem hiding this comment.
We need to handle exception requests.ConnectTimeout - probably by logging, maybe also retrying a limited number of times
Also, we might want to follow the advice on requests docs: pass timeout=(connection_timeout, read_timeout) and make connection_timeout slightly more than a multiple of 3
I was under the impression that the workers are already log with their stacktraces when they die. |
First time seeing that, probably a rare corner case |
I am honestly not familiar enough with this part of the code to know for sure. But the existing
That's fair. I think retrying the requests would be better (less work for simple errors), but it's not necessary for this PR. |
I believe separating timeouts is used to make the connection timeout faster, as a single connection can be retried a few times, and each gets its own timeout (also establishing a connection should be fast, usually). I am not experienced on this, which is why I went to the docs. I am fine going with what you wrote, if you think it is adequate. |
It's good enough. We can convert all timeouts to tuples later to be consistent and implement the documented recommendation |
|
I went ahead with the merge and added issues for the discord webhook error handling and the requests timeout recommendation |
|
I do not know how we stand regarding to master/prod differences nor hotfix deployment workflows and @alanpeixinho is out until monday, so my unfortunate recommendation for deploying this (and later #2043) as a hotfix is to manually log into the machine, cherry-pick the fixes, then build the images/run the services |
The main loop waited for the queue to drain (while not process_queue.empty()), which blocks forever when all workers die without consuming their poison pills. Now the loop checks on every iteration whether any worker is still alive and breaks with an error log if all workers have exited while items remain in the queue. This is the second part of the fix for kernelci#2041: PR kernelci#2042 adds request timeouts to prevent workers from hanging indefinitely, and this PR detects the case where workers exit without draining the queue. After joining, non-zero exit codes are logged and a Prometheus counter (kcidb_ingester_worker_failures) is incremented with reason="exception" or reason="signal". The reason label is more actionable for alerting (e.g. rate(worker_failures{reason="signal"}) > 0 catches OOM kills) while the exact exit code in the log line provides detail for debugging. Also adds docs/ingester.md documenting the parallel ingestion architecture, worker-queue protocol, and error handling. Related: kernelci#2041 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tales da Aparecida <tales.aparecida@redhat.com>
|
Unfortunately, I don't think this resolves the problem without #2043 In the last time we had a problem with ingestion, the container was still posting the progress report, which means the problem was the main loop stuck with a process_queue with a poison pill that would never be ingested... before from a worker stuck in a request without a timeout; now, after one of the workers died via timeout exception (unless workers' exceptions are killing the main process for some reason, in this scenario I'm not sure whether the container would auto-restart) |
…rnelci#2043) The main loop waited for the queue to drain (while not process_queue.empty()), which blocks forever when all workers die without consuming their poison pills. Now the loop checks on every iteration whether any worker is still alive and breaks with an error log if all workers have exited while items remain in the queue. This is the second part of the fix for kernelci#2041: PR kernelci#2042 adds request timeouts to prevent workers from hanging indefinitely, and this PR detects the case where workers exit without draining the queue. After joining, non-zero exit codes are logged and a Prometheus counter (kcidb_ingester_worker_failures) is incremented with reason="exception" or reason="signal". The reason label is more actionable for alerting (e.g. rate(worker_failures{reason="signal"}) > 0 catches OOM kills) while the exact exit code in the log line provides detail for debugging. Also adds docs/ingester.md documenting the parallel ingestion architecture, worker-queue protocol, and error handling. Related: kernelci#2041 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Tales da Aparecida <tales.aparecida@redhat.com> Co-authored-by: Tales da Aparecida <tales.aparecida@redhat.com>
Without explicit timeouts, any requests.post() or requests.get() call can block indefinitely when a remote server hangs. This caused the ingester on db.kernelci.org to stall for 14 hours on a hung HTTP upload, leaving 543k submission files unprocessed.
Fixes: #2041
Assisted-by: Claude Opus 4.6 noreply@anthropic.com