fix(workloadmanager): deduplicate GC candidates before deletion#335
Conversation
Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the garbage collection logic in pkg/workloadmanager/garbage_collection.go to deduplicate sandboxes from inactiveSandboxes and expiredSandboxes based on their SessionID. Feedback suggests simplifying the implementation by iterating over a slice of slices to reduce code duplication and adding nil checks for sandbox pointers to prevent potential panics.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 47.57% 49.30% +1.73%
==========================================
Files 30 30
Lines 2819 2868 +49
==========================================
+ Hits 1341 1414 +73
+ Misses 1338 1301 -37
- Partials 140 153 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…uce cyclomatic complexity Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a deduplicateSandboxes utility function to merge and deduplicate sandbox lists by SessionID, preventing redundant processing when a sandbox meets multiple expiration criteria. The review feedback suggests adding a nil check for sandbox pointers within the deduplication loop to improve the function's robustness.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The garbage collector's
once()method merges two candidate lists - inactive sandboxes (idle timeout elapsed) and expired sandboxes (past TTL) - before iterating over them for deletion. A sandbox that qualifies for both conditions appears in both lists, causing the GC to attempt deleting it twice in the same cycle. The second attempt fails onDeleteSandboxBySessionID(store entry already removed) and logs a spurious error.This PR deduplicates the merged candidate slice by
SessionIDbefore the deletion loop, ensuring each sandbox is processed exactly once.Special notes for your reviewer:
The deduplication uses a
map[string]struct{}keyed bySessionID. Inactive sandboxes are inserted first, then expired ones, preserving the original iteration order while skipping duplicates.Does this PR introduce a user-facing change?: