engine_dispatch: re-schedule retry when chunk content cannot be read - #12260
Open
ku524 wants to merge 2 commits into
Open
Conversation
flb_input_chunk_flush() returning NULL is usually transient, so dropping the retry discards records a later attempt could still deliver. Spend a delivery attempt on it instead and only give up once the configured retry limit is reached, where the existing accounting already applies. Behaviour is unchanged with the default retry_limit of 1: the first read failure still drops immediately. Signed-off-by: ku524 <yeonjuyeong@gmail.com>
Asserts that with retry budget left the task keeps its task-map slot, its chunk and a pending retry, and that no drop accounting is recorded. Signed-off-by: ku524 <yeonjuyeong@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #12254, so the base branch is
cosmo0920-plug-stale-failed-tasks-on-dispatchand the diff here is only my two commits.
#12254 fixes the task leak reported in #12252, and I verified that it does. This PR keeps
that fix and changes one thing: when the chunk content cannot be read, spend a delivery
attempt on it before giving up, instead of dropping on the first failure. With the default
retry_limitof 1 the behaviour is identical to #12254, so the change only takes effectwhen the user has explicitly raised their retry budget.
Builds on #12254, which closes #12252.
Problem
Releasing the task on the first read failure deletes the chunk file. The call chain is
flb_task_users_release()->flb_task_destroy(task, FLB_TRUE)->flb_input_chunk_destroy(ic, FLB_TRUE)->cio_chunk_close(ch, CIO_TRUE)->cio_file_native_delete().flb_input_chunk_flush()returnsNULLwhencio_chunk_up()orcio_chunk_get_content()fails. Those failures are typically transient under memory or descriptor pressure rather
than permanent corruption, so the records would very likely have been delivered by a later
attempt. The leak being fixed here is bad, but it does at least keep the records on disk
for a restart to flush, which is the behaviour I measured while investigating #12252.
Summary
The give-up path is unchanged from #12254, including its drop accounting. What is new is
that a read failure first consumes a delivery attempt and re-schedules through
flb_task_retry_reschedule(), which is what the failure case a few lines above alreadydoes for the same reason. On both paths the task is never left with no users and no
retries, so the leak stays fixed.
Reusing
Retry_Limitkeeps the decision where the user already expressed it, so no newconfiguration option or constant is introduced.
Retry_Limit1(default)no_retries(0)5no_limitsVerification
The three tests added by #12254 pass unchanged, so this does not alter the cases they
cover.
flb-it-task_mappasses as well. I added one test for the re-scheduled path thatasserts the task keeps its task-map slot, its chunk and a pending retry, and that no drop
accounting is recorded.
End to end on a build with 8
dummyinputs and 8forwardoutputs, 20 read failuresinjected with gdb produced 20 re-schedules and 0 drops, and the busy chunk gauge settled
back to 0 over three hours. The same injection against unpatched v5.1.0 pinned 20 chunks
permanently. Throughput was 3998 records per second against 3971 for the unpatched build,
and no crash, assertion or signal appeared in roughly three hours of continuous operation.
Trade-offs
A read failure now consumes one delivery attempt, so
retry->attemptscovers both deliveryand read failures. It never drops earlier than #12254 does.
A permanently unreadable chunk under
no_limitsstays on disk untilstorage.total_limit_sizeevicts it.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.