fix CRR queue progression after deletion#2485
Conversation
Signed-off-by: Jayant <212013719+Jayant-kernel@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Welcome @Jayant-kernel! It looks like this is your first PR to openkruise/kruise 🎉 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in the kruise-daemon ContainerRecreateRequest (CRR) controller where a subsequent CRR for the same Pod could remain stuck in Pending after the previous active CRR leaves the filtered informer (delete / active-label removal), by ensuring the Pod-scoped queue key is re-enqueued on delete events (including tombstones).
Changes:
- Route CRR delete events through a new
enqueueAfterDeletehelper that clears resource-version expectations and requeues the Pod key. - Add tombstone (
cache.DeletedFinalStateUnknown) handling to cover missed delete notifications. - Add a unit test validating that both normal delete and tombstone delete enqueue the expected Pod-scoped key.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/daemon/containerrecreate/crr_daemon_controller.go | Re-enqueues the Pod workqueue key on CRR delete/tombstone to allow the next pending CRR to progress. |
| pkg/daemon/containerrecreate/crr_daemon_controller_test.go | Adds unit coverage asserting delete and tombstone events enqueue the correct Pod-scoped queue key. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2485 +/- ##
==========================================
+ Coverage 49.93% 49.95% +0.01%
==========================================
Files 325 325
Lines 28435 28449 +14
==========================================
+ Hits 14200 14212 +12
- Misses 12581 12582 +1
- Partials 1654 1655 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Jayant <212013719+Jayant-kernel@users.noreply.github.com>
|
@furykerry review my pr |
|
/assign @FillZpp |
Ⅰ. Describe what this PR does
This PR prevents a
ContainerRecreateRequest(CRR) from remainingPendingwhen it is created immediately after a completed CRR for the same Pod is deleted.The kruise-daemon intentionally serializes CRRs by using
<namespace>/<podName>as its workqueue key. A race was possible when a second CRR was created while the daemon informer still contained the first CRR:Pending.activelabel after completion, the filtered informer's delete handler only cleared its resource-version expectation.Pendingindefinitely.The delete handler now:
DeletedFinalStateUnknowntombstones when the original delete notification was missed.Unit coverage verifies that both normal delete events and tombstone delete events enqueue the expected Pod key. The existing one-at-a-time execution semantics for CRRs targeting the same Pod are preserved.
Ⅱ. Does this pull request fix one issue?
Fixes #2474
Ⅲ. Describe how to verify it
The following checks pass locally with Go 1.23.8:
To verify the original scenario in a cluster:
PendingtoRecreatingand thenCompleted.Ⅳ. Special notes for reviews
This change deliberately keeps the Pod-scoped queue key instead of allowing CRRs for one Pod to execute concurrently. It only restores the missing wake-up when the previous active CRR leaves the filtered informer.
go test ./pkg/daemon/...was also attempted. The modifiedcontainerrecreatepackage and the other daemon packages passed, except the unrelated existingpkg/daemon/criruntime/imageruntimetest, which requires a credential-provider plugin binary that is not present in the local environment. A live cluster e2e run was not available locally.