fix: keep DRA devices held by non-pod consumers when their pods delete - #3215
Open
thc1006 wants to merge 2 commits into
Open
fix: keep DRA devices held by non-pod consumers when their pods delete#3215thc1006 wants to merge 2 commits into
thc1006 wants to merge 2 commits into
Conversation
gatherAllocatedDevices freed an allocated device for reallocation as soon as all of its pod consumers were deleting, without checking whether a non-pod consumer still held it. A device kept in use by a non-pod consumer (its metadata Releasable flag is false) could be handed back to the allocator and allocated a second time. Guard the all-consumers-deleting shortcut with Releasable, matching the sibling no-live-consumers shortcut, and propagate each claim's Releasable status onto its ContributionMetadata so a shared device's non-releasable share is not subtracted when its pod deletes. The per-device classification is extracted into deviceReallocation so the decision is unit-tested directly. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thc1006 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
cncf-cla: yes
…the seed state deviceReallocation freed a shared device whenever its effective consumed capacity map came out empty. An empty map does not prove the last share is gone: a surviving share can legitimately consume zero capacity, and subtracting a deleting claim's contribution can empty the map while a live pod or a non-pod consumer still holds the device. Freeing it then drops the device from the allocator seed state, so its fixed shared counters can be handed back to the pool and the allocator can over-admit. Make the two consumer-liveness checks the only whole-device release conditions, and always return a surviving shared device's effective capacity as a non-nil map that may be empty. gatherAllocatedDevices then keeps the device's ConsumedCapacity entry, recording the shared occupancy regardless of the capacity value. Add regressions for a live and a non-pod zero-capacity survivor (asserting the returned map is non-nil but empty), a controller test that a mixed pod/non-pod claim propagates a non-releasable contribution, and derive the test helper's device-level releasability from its contributions. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Author
|
Pushed a follow-up for the zero-capacity case: |
Author
|
/kind bug |
kubernetes-prow
Bot
added
the
kind/bug
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
1 participant
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.
Karpenter's provisioning loop reconstructs the set of already-allocated DRA devices before each scheduling simulation, and filters out the ones whose consumers are all going away so their capacity can be reused. That filter treated "every pod consumer is deleting" as "the device is free", but a ResourceClaim can also be reserved by a non-pod consumer, and those are invisible to the deleting-pod check.
The problem
gatherAllocatedDevicesfreed a device as soon asallConsumersDeletingreturned true for its pod UIDs. The device metadata already carries aReleasableflag that is false when any referencing claim has a non-pod consumer (or an emptyReservedFor), and the sibling "no live consumers" shortcut right above it already respects that flag. The all-consumers-deleting shortcut did not.So a device reserved by both a deleting pod and a non-pod consumer, or a shared device split across a deleting-pod claim and a separate non-pod claim, was handed back to the allocator as free while something still held it. The scheduling simulation would then see it as available and could allocate it a second time.
The fix
Two changes, both mirroring the guard that was already present for the no-live-consumers case:
meta.Releasable, so a device still held by a non-pod consumer is never freed through the deleting-pod path.Releasableonto itsContributionMetadata, and skip subtracting a non-releasable contribution's share ineffectiveConsumedCapacity. This covers the shared-device case where a single claim mixes a deleting pod with a non-pod consumer, so its share stays reserved even though the pod is gone.The per-device classification that
gatherAllocatedDevicesruns is pulled out into a small pure helper,deviceReallocation, so the reallocation decision (free, keep as exclusive, or keep with the remaining shared capacity) can be unit-tested without standing up the whole controller.Testing
Added unit tests for both paths, each confirmed to fail before the change and pass after: a non-releasable contribution keeps its share in
effectiveConsumedCapacity, anddeviceReallocationkeeps a device held by a non-pod consumer even when its pods are all deleting, alongside the freed, exclusive, and shared-partial branches. The fullprovisioninganddeviceallocationsuites pass.Scope
This addresses the non-pod-consumer release path described in #3212. The other half of that issue, that
Sharedis derived from the presence ofConsumedCapacityrather than from the claim'sShareID, is a separate and more structural change that I have left out of this PR.