[10.4 stable] Prevent deferred queue from growing #4030
Merged
Conversation
This file contains 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
rouming
requested review from
rene,
milan-zededa,
deitch and
eriknordmark
as code owners
eriknordmark
requested changes
Jul 1, 2024
There was a problem hiding this comment.
Good to have the PR reference in the descriptions.
But we should also have the original master commit in the commit messages.
(And if you do that with git cherry-pick -x I suspect that would have carried the DCO signature across as well.)
In any case, the DCO needs to be fixed.
Will add once master is merged, otherwise SHA may change. |
Something already unfixable: |
FWIW that is bogus from more than one perspective:
I don't know what's broken in GH. Let's see what happens when you update the commit descriptions if DCO still has issues. |
…C reqs LOC has different URL, so controller and LOC deferred requests can't be placed into the same queue due to the same requests keys. Having two destinations (controller + LOC), the second sender overrides a deferred item, so first one destination will be never reached. First attempt to fix this was made here: 62b00e2 ("zedcloud/deferred: respect URL argument as a key on starting new deferred") But this introduces more problems then fixes them (for example `RemoveDeferred()` was not covered at all). This is the proper way to deal with different destinations. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com> (cherry picked from commit 713af84)
This fixes the growing deferred queue issue caused by missing request merges. Commit 9f2c9ad ("zedcloud/deferred: don't take lock while processing the deferred queue") relaxes locks and makes it possible to add new requests while queue processing waits in send. New requests should land in the same `deferredItems` list, which should be processed later at the end of the `handleDeferred()` operation. The bug lies in the actual merging of two queues: the one that has not been completed due to a possible error and the other queue, which was populated with new requests during the send. Proper queue merging is not just concatenation of queues (which would cause the resulting queue to always grow), but involves item replacement by key, so the queue size stays the same, with the item being replaced. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com> Fixes: 9f2c9ad ("zedcloud/deferred: don't take lock while processing the deferred queue") Reported-by: Milan Lenco <milan@zededa.com> (cherry picked from commit 4582efb)
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.
Backport of the #4017 PR.