Repository navigation
resmgr: retain pending updates on nri failure - #809
Merged
klihub merged 1 commit intoSep 23, 2026
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation consistently preserves undelivered updates and includes focused coverage for all described outcomes.
Review effort: Balanced
Findings: None
What changed in this PR
Retains pending container updates until the NRI runtime confirms delivery, enabling retries after failures or rejections.
Changes:
- Separates peeking and clearing pending updates.
- Preserves failed/rejected updates while clearing successful ones.
- Adds tests for delivery outcomes.
| File | Description |
|---|---|
pkg/resmgr/nri.go |
Implements acknowledgement-aware update clearing. |
pkg/resmgr/nri_test.go |
Tests successful, failed, and rejected updates. |
pkg/resmgr/cache/container.go |
Adds separate peek and clear operations. |
pkg/resmgr/cache/container_test.go |
Adopts the new pending-update API. |
pkg/resmgr/cache/cache.go |
Updates the container interface. |
cmd/plugins/topology-aware/policy/mocks_test.go |
Updates the test mock interface. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bart0sh
marked this pull request as ready for review
September 22, 2026 14:50
klihub
approved these changes
Sep 22, 2026
klihub
reviewed
Sep 22, 2026
klihub
reviewed
Sep 22, 2026
klihub
reviewed
Sep 22, 2026
An update pushed to the runtime outside an NRI response is the only record of what its container should look like: if we drop it and the push does not arrive, the container stays as it is with nothing left to retry it with. So the updates are now taken from their containers only once the runtime has them, and a failed request leaves them all pending. The runtime can also answer a request it accepted with the updates it could not apply. Those containers were not updated either, so their updates stay pending just the same, which needs peeking at a pending update without consuming it and clearing the delivered ones by hand. The updates carried by an NRI response are still taken as they are collected. Nothing acknowledges those, so there is no later point to clear them at, and keeping them would repeat them in every response from here on. Collecting them is now peeking at them and clearing them right away, which is all it ever did, so the destructive read they used to use is gone from the container interface. Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
bart0sh
force-pushed
the
PR002-retain-pending-updates-on-nri-failure
branch
from
September 23, 2026 07:57
35d51fc to
13b5d18
Compare
Collaborator
Author
|
@klihub Thank you for the review. All review comments have been resolved. |
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.
Pending container updates that go out through
UpdateContainers()are currently taken from their containers before the push:updateContainers()starts withgetPendingUpdates(), which clears each update as it returns it. If the push then fails, the update is gone - and it was the only record of what the container should have looked like, so the container keeps its old resources and there is nothing left to retry it with.The updates are now taken from their containers only once the runtime has them.
updateContainers()peeks at the pending updates, pushes them, and clears the ones that were delivered; a failed request leaves all of them pending.The runtime can also accept the request and report back the updates it could not apply. That answer used to be discarded. Those containers were not updated either, so their updates now stay pending just the same, and a warning names them.
The updates carried by an NRI response are unchanged: they are still taken as they are collected, because nothing acknowledges them - of the NRI messages only
UpdateContainersResponsehas afailedfield — so there is no later point to clear them at, and keeping them would repeat them in every response from then on.cache.Containeraccordingly trades its destructiveGetPendingUpdate()forPeekPendingUpdate()and ClearPendingUpdate()`: reading a pending update and discarding it are now separate steps, which is what the retry above needs.This path is how any out-of-band allocation change reaches the runtime, so the existing user is policy reconfiguration; DRA claim allocation (#806) is what made the lost updates easy to hit.
Tests:
pkg/resmgr/nri_test.gocovers the three outcomes — retained when the request fails, cleared when the runtime has them, retained for the containers the runtime rejected. The first and third fail against the previous behaviour.