Repository navigation
Say when rightsizing could not read a workload's pods - #1746
Merged
Merged
Conversation
Rightsizing read the workload's pods for two things — the pod names and whether those pods have been OOM-killed — and discarded the error if the read failed. An empty answer then looked exactly like a workload whose pods are healthy, so a recommendation could silently omit "these pods have been OOM-killed" and present itself as complete. Two cases reach that error and they are not alike. A warming informer clears on its own within seconds, so reporting it would attach a caveat that the next refresh removes; the read now waits for it, bounded, and gets the real pods. A denial never clears, and is reported: rows carry liveInventoryDenied, and the UI qualifies the suggestion rather than presenting a partial one as whole. The cluster scan had the same hole by a different route — it lists pods directly and returned early on a denied lister, and never checked informer sync at all, so a warming cache read as a cluster with no OOM-killed pods anywhere. It now waits once for the whole scan and marks every workload when the pods stay unreadable. The recommendation itself is unchanged in either case: its safety gate is OOMEvidenceAvailable, computed from the Prometheus termination window, which never depended on the live pod read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
PR Summary by QodoReport unavailable live pod inventory in rightsizing results
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+342
to
+344
| row.liveInventoryDenied && row.resource === 'memory' | ||
| ? "Radar could not read this workload's pods, so it could not check them for out-of-memory restarts." | ||
| : undefined |
There was a problem hiding this comment.
4. Cloud users miss inventory caveat 🔗 Cross-repo conflict ≡ Correctness
liveInventoryDenied is rendered only by the newly changed Radar UI, while Radar Hub Web builds its embedded RadarApp from the locked 1.13.5 package rather than this branch. Until a new package is published and Hub's lockfile and deployment are updated, cloud users receiving the new backend field still see recommendations without the caveat.
Agent Prompt
## Issue description
Radar Hub Web embeds a lockfile-pinned release of `@skyhook-io/radar-app`, so merging and deploying the backend field alone does not make the new inventory caveat visible to cloud users.
## Fix Focus Areas
- web/src/components/resource/RightsizingStrip.tsx[339-347]
- .github/workflows/publish-radar-app.yml[12-36]
- /cross_repos/radar-hub-web/package-lock.json[1223-1226]
## Recommended Fix
Publish a new `@skyhook-io/radar-app` version containing this change, update Radar Hub Web's dependency and lockfile to that version, and coordinate the Hub deployment alongside or before agents that emit `liveInventoryDenied`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The scan marked a denied inventory only when the pod informer was unusable, and still discarded the errors from the list calls themselves. It also treated a missing ReplicaSet lister as ordinary: a pod names the ReplicaSet that owns it, never the Deployment, so without that map every Deployment quietly collected no OOM evidence while reporting none had any. Both list paths now report failure, and a lost ownership path marks Deployments alone — a StatefulSet owns its pods directly and loses nothing. The scan's evidence note also composes now. The inventory caveat sat in the early-return chain ahead of the limit-conflict branch, so a row that could not read its pods hid its own limit conflict; it renders alongside whichever reason applies instead. Its claim that the suggestion "rests on the metrics window alone" is gone, since the metrics query can have failed too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 40a2600. Configure here.
Contributor
Author
|
On the remaining cross-repo item ("Cloud users miss inventory caveat") — accurate, but not actionable here, so it is deliberately not addressed in this PR. Radar Hub builds its embedded The backend field is additive and optional ( Also worth noting for anyone reading the checks: Cursor Bugbot reported |
…eplicaSets Two paths still lost the live-inventory caveat. The strip returned a staged reduction's copy before it reached the caveat, so the one suggestion that asks the reader to reduce in steps was also the one that never said its pods had gone unchecked. The caveat now qualifies every message the strip returns. The scan waited for the pod informer but took the ReplicaSet lister at face value. ReplicaSets load on the deferred path, so a warming lister is ordinary right after startup and answers an empty list with no error — resolving every Deployment's pods to nothing while reporting the inventory as read. It is now waited on exactly as pods are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
An informer that is synced and non-nil still answers only for the namespaces it watches, which is how Radar runs whenever cluster-wide list is denied. The scan took a successful empty list as proof, so a workload in an unwatched namespace reported no OOM-killed pods with full confidence. Availability is now decided per workload, against the namespaces its pods and — for a Deployment — its ReplicaSets actually cover. A scan holding no Deployments no longer waits on the deferred ReplicaSet informer it never reads, and a cancelled request stops instead of recording an unreadable inventory and continuing into Prometheus on a dead context. When there is no retained ownership and the pods cannot be listed, the response now says so rather than reporting an absence of samples: the selection falls back to the current pods and matches nothing by construction, which is a fact about the inventory, not about the metrics. The field is liveInventoryUnavailable rather than Denied, because an exhausted warming wait lands here too and is not a permission decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW
nadaverell
changed the title
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
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.
Closes #1726.
Rightsizing read a workload's pods for two things — the pod names, and whether those pods have been OOM-killed — then discarded the error when the read failed. An empty answer was indistinguishable from a workload whose pods are healthy, so a recommendation could silently drop "these pods have been OOM-killed" and still present itself as complete.
Availability is decided per workload
The question is never "can Radar list pods" but "can Radar list this workload's pods". Three different failures answer no, and they are not alike:
WorkloadPods; the scan waits once for the whole scan rather than per workload, and only for the informers it will actually read — a scan holding no Deployments never waits on the deferred ReplicaSet informer.Rows carry
liveInventoryUnavailable— unavailable rather than denied, because an exhausted warming wait lands there too and is not a permission decision.What the reader sees
The caveat qualifies whichever explanation applies rather than standing in for it: a row can both be unable to check its pods and have a limit conflict, and hiding either behind the other loses something. It survives the staged-reduction path too — the one suggestion that asks you to reduce in steps is exactly where an unchecked pod matters most.
When there is no retained ownership and the pods cannot be listed, the response says so directly instead of reporting an absence of samples. The selection falls back to the current pods and then matches nothing by construction, so "no samples available" would send the reader hunting for missing metrics rather than an unreadable inventory. That reason is set server-side, so every consumer gets it rather than one UI.
What does not change
The recommendation itself. Its safety gate is
OOMEvidenceAvailable, computed from the Prometheus termination window, which never depended on the live pod read.liveInventoryUnavailablemeanscurrentPodOOMis unknown rather than false — a qualification, not a suppression.A cancelled request stops rather than recording an unreadable inventory and continuing into Prometheus on a dead context.
Testing
Unit. Go tests drive the scan against a real
ResourceCacheover a fake clientset: pods readable, pod lister denied, ReplicaSet ownership unreadable (Deployments flagged, StatefulSets not), and a namespace-scopedResourceScopescache where one workload sits inside the watched namespace and one outside. A further test pins that a DaemonSet-only scan does not spend the warming budget on ReplicaSets. Frontend tests cover the caveat composing with a suppression reason and surviving a staged reduction.go build ./...,go vet,go test ./internal/prometheus/(including-race),tsc --noEmit, and the rightsizing suites pass.Live, against a GKE cluster with real Prometheus and kube-state-metrics. Binaries built from this branch and from
mainwere run against the same cluster and the same Prometheus, and their responses compared:Deployment staging/billing): identical —ownerCoverage: ksm_history, 2 rows,fit=oversized, matching OOM-evidence flags.liveInventoryUnavailableis absent, which is correct for a readable inventory.fit,recommendationReason,hpaManaged,hpaEvidenceAvailable,oomEvidenceAvailable,currentPodOOM,windowOomEvidence,limitConflict,confidence,reductionLimited,recommendedRequest. The only payload differences were measured values (observed, peak, throttle ratio, sample counts) drifting between two runs seconds apart.That exercises the per-workload coverage check, the ReplicaSet ownership path and the warming waits against real informers.
Live, under a genuinely denied pod inventory. A throwaway ServiceAccount was granted everything rightsizing needs except
pods—kubectl auth can-i list podsreturnedno,deploymentsandreplicasetsreturnedyes— and both builds were run under it against the same cluster:maincurrentPodOOMfalsefalseliveInventoryUnavailabletrueOn
mainthe response is indistinguishable from a workload whose pods have simply never been OOM-killed. The recommendation is unchanged between the two builds —fit=oversized, sameownerCoverage: ksm_history, same row and workload counts — which is the point: the pod read was never what the recommendation rested on, only what the OOM check needed. The ServiceAccount, ClusterRole, ClusterRoleBinding and namespace were deleted afterwards and verified gone.Visual-test skipped: the UI delta is copy inside existing components, no new surface.
Follow-up
The same class of bug exists on the HPA paths, where it is worse:
HPAEvidenceAvailablegates whether a reduction is suppressed at all, so an uncovered namespace can let one through on a workload whose autoscaler was never seen. It is pre-existing and outside these lines, tracked in #1749.Both rightsizing components predate the current Prettier config and are not formatted to it on
main; this change matches each file's existing style rather than reformatting ~1,000 unrelated lines.🤖 Generated with Claude Code
https://claude.ai/code/session_0125QDX24TNuod7FKLwTDhvW