Repository navigation
DRA: refresh node resources after claim changes - #829
Conversation
bart0sh
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Asynchronous NRT refreshes can execute out of order and restore stale pre-release data.
1 open finding
What changed in this PR
Refreshes exported node resources after DRA claim allocation and release.
Changes:
- Centralizes node-resource refreshes.
- Refreshes NRT and extended resources after claim changes.
- Adds unit and end-to-end coverage.
| File | Description |
|---|---|
pkg/resmgr/resource-manager.go |
Adds the shared refresh helper. |
pkg/resmgr/nri.go |
Uses the helper for NRI events. |
pkg/resmgr/dra.go |
Refreshes resources after claim changes. |
pkg/resmgr/dra_test.go |
Tests claim-triggered refreshes. |
pkg/resmgr/nri_test.go |
Adds the required test agent. |
test/e2e/.../code.var.sh |
Verifies NRT claim updates. |
test/e2e/.../py_consts.var.py |
Supports empty CPU sets. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
caf8313 to
e139127
Compare
ea5356f to
538ce5d
Compare
538ce5d to
d28110d
Compare
d28110d to
030a2ab
Compare
dbe04d6 to
d28110d
Compare
A claim changes what the policy has free, which the NRT CR and the node's extended resources report. Refresh both after a claim is allocated or released, as the NRI path does after container changes. Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com>
d28110d to
95791b8
Compare
| # | ||
|
|
||
| cleanup() { | ||
| vm-command "kubectl delete pods --all --now --wait" || : |
There was a problem hiding this comment.
nit: we have now common test helpers in lib/test.bash, including a delete-pods which we could use here. I think most (maybe all) of the other test cases have already been converted to use it. There are a bunch of others and this DRA test probably slipped through, because it was not in the merged main tree when we converted the others. Not even nearly a show-stopper to get this merged, but maybe you could push another PR on the stack to convert it to be closer to the rest.
There was a problem hiding this comment.
@bart0sh Actually, let's forget about doing this change here. You also have pending #834 touching the same tests, and we did a sweep and there is quite a bunch of the other existing test cases which also could use a 2nd unification round. So my suggestion would be to get in this and #834, and then we'll merge a larger test case unification and cleanup PR, to take care of a larger bunch.
klihub
left a comment
klihub
left a comment
There was a problem hiding this comment.
This looks to me good to go in now.
The potential NRT ordering problem is true and real but it has nothing to do with the changes here. It's been there forever, because we never really fixed the problem in the original implementation (synchronous update slowing down policy decisions), instead we just half-arsed hid it by tossing in an async goroutine and calling it a day. We can and should fix that separately from this.
Refreshing node resources after claim changes
Eighth PR of the DRA series, on top of #823. Two commits: resmgr refreshes
the node resources after a claim change, and the
test42-dra-devicese2e testchecks the NodeResourceTopology CR after a claim and after its release.
The NRI path refreshes the NodeResourceTopology CR and the node's extended
resources after every change to the policy's allocations. The claim path did
not, so after a claim was allocated or released both kept what the last NRI
event left them. With topology-aware, the CR then still showed the claimed CPUs
as free.
Now
ClaimAllocatedandClaimReleasedrefresh both, after saving the cacheand updating the containers. A failed refresh is logged, as on the NRI path,
and does not fail the claim: it does not change what the claim was given. A
failed allocation is rolled back through
ClaimReleased, which refreshes then.A small helper,
updateNodeResources(), now does the refresh. It replaces thepaired
updateTopologyZones()/updateNodeExtendedResources()calls on the NRIpath and after a reconfiguration, so there is one list of what gets refreshed.
No policy or interface change is needed: the policy still answers
GetTopologyZonesandGetExtendedResourceswhen resmgr asks.The e2e test
test42-dra-devicesreads theshared cpusetattribute of a NUMA node's zonein the CR. After a claim takes CPUs of a node, the zone must stop listing them.
After the claim is released, it must list them again. The NRI path also
refreshes the CR when the claiming container is created, so the first check is
only a sanity check. No container event follows the release, so the second
check passes only if the claim path refreshes the CR. The test's
cpuset()helper now also accepts an empty string, which is what a zone with no shared
CPUs left reports.
What is not here
These are known limitations, each lifted by a later PR in the series:
but the policy resets its saved state on start, so after the plugin
restarts their CPUs are free again and can be granted twice.
DeviceClassships yet, so a claim needs one written by the user.Verification
go test -race ./...passes, and golangci-lint finds no issues inpkg/resmgr.TestClaimCommitRefreshesNodeResourceschecks that allocating and releasinga claim each refresh the node resources once, and that a failing NRT CR
update does not fail either call. Removing either of the two new calls fails
the test.
test42-dra-devicespasses on n4c16 with Kubernetes 1.37, containerdv2.4.1, and the
DRAConsumableCapacityandDRANodeAllocatableResourcesgates. A claim for 2 CPUs of node 0 got 0-1, the CR dropped 0-1 from the
zone's shared CPUs, and listed them again after the claim was released.