fix(backend): batch cleanup loop in listKubernetesObjects (ACM-35158)#6523
fix(backend): batch cleanup loop in listKubernetesObjects (ACM-35158)#6523fxiang1 wants to merge 6 commits into
Conversation
The cleanup loop that removes stale resources after a re-LIST was not using batchPromiseAll, so it blocked the event loop for the entire duration—preventing liveness probe responses. Additionally, the items.find() inside the loop was O(n²). With 22K+ Group resources this caused probe timeouts and CrashLoopBackOff. - Convert the for...in cleanup loop to use batchPromiseAll, yielding the event loop every 100 entries via setImmediate - Replace O(n²) items.find() with a Set<string> for O(1) UID lookups Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: fxiang1 <fxiang@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fxiang1 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesCache reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
backend/src/routes/events.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/routes/events.ts (1)
447-447: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse optional chaining for
metadata.uid.Although Kubernetes objects consistently contain
metadata, theIResourceinterface typesmetadataas optional. Using optional chaining prevents potential runtime crashes and aligns with strict TypeScript checks.♻️ Proposed refactor
- const itemUids = new Set(items.map((item) => item.metadata.uid)) + const itemUids = new Set(items.map((item) => item.metadata?.uid))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/events.ts` at line 447, Update the itemUids construction in the events route to access the UID through optional chaining on item.metadata, preserving the existing Set and mapping behavior while safely handling resources without metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/routes/events.ts`:
- Around line 451-452: Guard the cache lookup in the resource-processing flow
before accessing existing.compressed: when cache[uid] is undefined after the
asynchronous yield, skip that resource safely. Preserve the existing
inflateResource path for entries that remain present.
---
Nitpick comments:
In `@backend/src/routes/events.ts`:
- Line 447: Update the itemUids construction in the events route to access the
UID through optional chaining on item.metadata, preserving the existing Set and
mapping behavior while safely handling resources without metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef97dbed-b07e-4962-b55c-43dca39ec85a
📒 Files selected for processing (1)
backend/src/routes/events.ts
Cache entries can be deleted by a concurrent watch event during the setImmediate yield between batches. Skip UIDs whose cache entry was removed to prevent a TypeError on undefined.compressed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/routes/events.ts (1)
869-869: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant cache entry check.
This check is redundant and can be safely removed. If
expectedCacheEntryis provided, line 860 ensuresexistingis truthy, so the block at line 863 is guaranteed to execute. Since no asynchronous operations occur between the identical check at line 865 and here,cache[uid]cannot change in the interim.♻️ Proposed refactor
- if (expectedCacheEntry && cache[uid] !== expectedCacheEntry) return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/events.ts` at line 869, Remove the redundant `expectedCacheEntry` versus `cache[uid]` comparison from the surrounding cache-update logic in `events.ts`; retain the earlier validation and the rest of the block unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/routes/events.ts`:
- Line 869: Remove the redundant `expectedCacheEntry` versus `cache[uid]`
comparison from the surrounding cache-update logic in `events.ts`; retain the
earlier validation and the rest of the block unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ef356164-f556-46c0-b77a-26e276bbe04f
📒 Files selected for processing (1)
backend/src/routes/events.ts
Signed-off-by: fxiang1 <fxiang@redhat.com>
Signed-off-by: fxiang1 <fxiang@redhat.com>
|
Signed-off-by: fxiang1 <fxiang@redhat.com>
|
@fxiang1: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
📝 Summary
Ticket Summary (Title):
console-mce pod CrashLoopBackOff due to probe failure with large resource sets
Ticket Link:
https://issues.redhat.com/browse/ACM-35158
Type of Change:
Root Cause Analysis
PR #6339 introduced
batchPromiseAllto yield the event loop during resource caching and deletion, preventing liveness probe timeouts with large resource sets. However, the cleanup loop inlistKubernetesObjects(backend/src/routes/events.ts:448-462) was left unbatched. This loop runs after every re-LIST (including watch reconnections) and has two compounding problems:1. Event loop starvation (unbatched loop)
The
for (const uid in cache)loop sequentially inflates every cached resource viainflateResourcewithout ever yielding the event loop. With 22,913 Group resources, this means ~22K sequential decompressions with nosetImmediatebreak, blocking the event loop for the entire duration. During this window, the backend cannot respond to liveness or readiness probe requests, causing Kubernetes to kill the pod.2. O(n²) UID lookup
Inside the loop,
items.find((resource) => resource.metadata.uid === uid)performs a linear scan of the entireitemsarray for each cached entry. With 22K Groups, this results in ~525 million string comparisons — all on the main thread without yielding.Timeline of the crash loop
batchPromiseAllcaches all items with event loop yields (PR ACM-35158 avoid blocking incoming requests [release-2.16] #6339 fix works here ✅)What Changed
File:
backend/src/routes/events.tsReplaced the
for...incleanup loop withbatchPromiseAll— processes cache entries in batches of 100, callingsetImmediatebetween batches to yield the event loop. This is consistent with the pattern established by PR ACM-35158 avoid blocking incoming requests [release-2.16] #6339 for the caching and deletion steps.Replaced O(n²)
items.find()with aSet<string>— buildsitemUids = new Set(items.map(item => item.metadata.uid))upfront, then usesitemUids.has(uid)for O(1) lookups instead of linear scanning.✅ Checklist
General
ACM-12340 Fix bug with...)If Bugfix
🗒️ Notes for Reviewers
This is a follow-up to PR #6339 which batched the
cacheResourceanddeleteResourcecalls but missed the cleanup loop between them. The cleanup loop is the remaining code path that blocks the event loop with large resource sets.The fix reuses the existing
batchPromiseAllutility (introduced in #6339) — no new dependencies or utilities are added. TheSetreplacement foritems.find()is a straightforward algorithmic improvement from O(n²) to O(n).All existing backend tests pass (336/336). The
batchPromiseAllutility is already covered bybackend/test/lib/batch-promise-all.test.ts.Summary by CodeRabbit