Prevent deferred queue from growing #4017
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
github-actions
bot
requested review from
christoph-zededa,
eriknordmark,
OhmSpectator and
uncleDecart
rouming
added
bug
|
Why doesn't the build workflow run for this PR? |
…new deferred" This reverts commit 62b00e2. Actually original commit introduces more potential problems, for example `RemoveDeferred()` call was not covered at all. In order to fix deferred removal as well more ugly tweaks have to be made. But there is a better way! A proper fix will follow. Stay tuned. Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
…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>
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>
milan-zededa
approved these changes
Jul 1, 2024
|
LGTM |
This was referenced Jul 1, 2024
OhmSpectator
reviewed
Jul 1, 2024
milan-zededa
approved these changes
Jul 1, 2024
|
Unfortunately, we run out of docker pulls for the day so tests are mostly failing now. |
|
@milan-zededa do you want to release Eden with docker login? It should reduce this timeouts occurrence ? |
Yes, let's do it. Are you able to set docker login/password in the eve repository? |
|
It's already there, we have it for publishing. The only problem would be that most likely testing time will increase because we will be running on specified Pinned runners |
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.
This PR addresses an issue related to deferred queue grow.
The first patch reverts a commit (62b00e2) that attempted to use the URL argument as a key for deferred requests. The revert is necessary because the original commit introduced more problems than it solved, particularly with the
RemoveDeferred()function, which was not covered at all. A better solution for handling deferred requests will follow.The second patch introduces separate deferred contexts for LOC and controller requests. This prevents conflicts where deferred items for different URLs could override each other, ensuring that requests to both destinations are reached.
The third patch fixes the issue of a growing deferred queue by ensuring proper merging of deferred requests. Previously, new requests added during queue processing were not properly merged, leading to queue growth (reported by @milan-zededa). The patch ensures that new requests are merged by key, maintaining the queue size.
Ps. PRs to stable branches will follow in a couple of days.
cc: @milan-zededa