app/vlstorage/netinsert: drain buffered data on graceful shutdown ins… - #1572
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…tead of dropping it vlinsert dropped in-flight buffered log blocks whenever a pod terminated (autoscaling scale-in or rolling restarts): the shutdown flush reused the already-canceled stopCh context, so requests failed immediately with "context canceled" and the data was dropped. Thread an explicit context through the send path so the shutdown flush uses a fresh, bounded context. Buffered data is now drained to storage nodes on shutdown, bounded by the new -insert.drainTimeout flag (default 10s).
b64086a to
6eec825
Compare
|
Hi, thanks for looking at this. Meanwhile we are trying to mitigate this with a prestop sleep and short http.connTimeout on vlinsert to close clients keep-alive connections before the sleep ends. |
func25
left a comment
func25
left a comment
There was a problem hiding this comment.
Thanks for reporting this issue!
…ad of the shutdown context The final flush during graceful shutdown reused the already-canceled shutdown context, so the last buffered data block failed instantly with "context canceled" and was dropped. Send every data block with its own timeout instead, consistent with -remoteWrite.sendTimeout in vlagent.
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
*Note: after changing the original direction of this PR to sending timeout, it seems to also incidentally fix #1512. Assigned to owner of #1512. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
# Conflicts: # docs/victorialogs/CHANGELOG.md
|
Thanks @github-vincent-miszczak! After an internal discussion, we settled on a configurable |
Signed-off-by: Cuong Le <cuongleqq@gmail.com>
…insert.drainTimeout on shutdown
| insertDrainTimeout = flag.Duration("insert.drainTimeout", 5*time.Second, "The maximum duration for draining the in-memory buffered logs to -storageNode nodes on graceful shutdown. "+ | ||
| "It must be smaller than the container termination grace period minus -http.shutdownDelay, otherwise the process may be killed before the drain completes and the buffered logs are lost") |
There was a problem hiding this comment.
Wdyt about setting it to 12s by default? I know it's a bit of an edge case, but unavailable nodes are temporarily disabled for 10s, so setting it to 5s may not be an efficient retry interval
| insertDrainTimeout = flag.Duration("insert.drainTimeout", 5*time.Second, "The maximum duration for draining the in-memory buffered logs to -storageNode nodes on graceful shutdown. "+ | ||
| "It must be smaller than the container termination grace period minus -http.shutdownDelay, otherwise the process may be killed before the drain completes and the buffered logs are lost") |
There was a problem hiding this comment.
I think it's better to document that the grace period must cover: preStop + http.shutdownDelay + http.maxGracefulShutdownDuration + insert.drainTimeout + a safety margin
3 participants
…tead of dropping it
vlinsert dropped in-flight buffered log blocks whenever a pod terminated (autoscaling scale-in or rolling restarts): the shutdown flush reused the already-canceled stopCh context, so requests failed immediately with "context canceled" and the data was dropped.
Thread an explicit context through the send path so the shutdown flush uses a fresh, bounded context. Buffered data is now drained to storage nodes on shutdown, bounded by the new -insert.drainTimeout flag (default 10s).