Conversation
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>
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.
| } | ||
|
|
||
| // updateContainers pushes the pending container updates to the runtime. Unlike | ||
| // the updates carried by the response to an NRI request, these are only taken |
There was a problem hiding this comment.
nit: @bart0sh Wouldn't it be more humanly understandable to say 'these are only cleared from the containers' instead of 'taken from the containers' ?
| return updates | ||
| } | ||
|
|
||
| // clearPendingUpdates takes the delivered updates from their containers. The |
There was a problem hiding this comment.
nit @bart0sh ditto here. Wouldn't it be more humanly understandable to say 'clears the delivered updates from their containers' ?
| } | ||
|
|
||
| // getPendingUpdates collects the updates to carry in the response to an NRI | ||
| // request, and takes them from their containers as it does so. Nothing tells us |
There was a problem hiding this comment.
nit: @bart0sh ditto here. Wouldn't it be more humanly understandable to say 'and clears them from their containers' ?
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.