Conversation
aivong-openhands
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable — the collector is isolated and read-only, but one core comparison conflates numeric caps with LiteLLM budget source.
[CRITICAL ISSUES]
- [charts/openhands/templates/troubleshoot/support-bundle.yaml, Line 893] Correctness:
budget_matchestreatsdesired_max_budget_in_team == Noneas “the applied max budget must also beNone”. That is not the reconciliation contract. In the Enterprise budget sync code, an expected member budget ofNonemeans “this user should be on the shared team budget”; readback only fails if the user is not using the shared budget. With this collector, an org with a team monthly limit but no per-user default limit will report every shared-budget member as mismatched becauseappliedisteam.max_budget, notNone. The inverse can also slip through: a private membership withmax_budget: nullcan match even though it is not sharing the team budget. Please make the match source-aware: whenrequires_private_budgetis false, healthy should mean shared/no private budget; when it is true, healthy should require a private budget and compare the numeric private cap.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is an isolated, read-only support-bundle collector, so it does not affect runtime behavior. The risk is elevated because it handles budget diagnostics and LiteLLM financial state; a false degraded result is likely to send Support down the wrong path for the exact incident class this collector is meant to diagnose.
VERDICT:
❌ Needs rework: the collector should preserve the reconciliation distinction between shared team budgets and private member budgets before this diagnostic is reliable.
KEY INSIGHT:
The diagnostic can only mirror budget reconciliation if “budget source” and “budget value” stay separate in the match logic.
This review comment was created by an AI agent (OpenHands) on behalf of the user.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., “Security concerns about X do not apply here because Y”). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
There was a problem hiding this comment.
Seems like a great add to support bundles. Approved and my review focused mainly on the avoidance of including any sensitive information in the resulting output (already noted in the comments/PR description, but was double checking)
Edit: I see the review above and commentary around it but dont feel equipped to confirm the accuracy of that claim. Any way we can get a sample support bundle? Maybe via the unstable deployment?
|
Addressed the source/value distinction in 58e6b53. Shared-budget policy now matches only when LiteLLM has no private member budget, regardless of the inherited team cap value. Private-budget policy now requires a private membership budget and then compares its numeric cap. Unknown baseline/live-spend states remain non-assertive ( |
aivong-openhands
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste — the collector is isolated, read-only, and follows the existing support-bundle exec pattern without adding chart configuration surface area.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This support-bundle collector reads budget and LiteLLM financial state from customer environments, so the review focus is data exposure and diagnostic accuracy. I verified the collector avoids managed/custom LLM key columns, does not emit the LiteLLM API key, uses a read-only DB session, and the latestmember_budget_matcheslogic preserves the shared-team-budget vs private-member-budget distinction used by Enterprise reconciliation. GitHub checks, including Helm Unit Tests and chart lint/test, are passing.
VERDICT:
✅ Worth merging: the diagnostic is scoped to observability, matches the relevant reconciliation semantics, and has coverage for rendering plus key-column regressions.
KEY INSIGHT:
The change adds useful budget reconciliation visibility without attempting repair or expanding the self-hosted chart interface.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
@dylan-openhands Yes — I ran the rendered collector read-only against the internal beta deployment. Here is the sanitized summary (opaque org/user IDs removed): {
"budget_org_count": 1,
"degraded_org_count": 1,
"team_budget_matches": true,
"member_match_counts": {"true": 3, "unknown": 10},
"applied_budget_sources": {
"private_membership": 3,
"team_shared_roster_only": 10
},
"degraded_reasons": [
"members_without_expected_private_budget",
"known_member_cycle_baseline_missing",
"last_budget_sync_failed"
]
}That is the real state we were trying to make visible: the team cap is correct, the 3 members with stored baselines/private caps match, and 10 legacy known members have neither a recoverable stored baseline nor their expected private cap. No managed/custom key columns or LiteLLM credentials are selected or emitted. I can also run the same collector on a disposable replicated instance when we exercise the repair PR. |
|
Additional packaging validation: this branch built successfully as an isolated Replicated release (sequence 1606) with the released Enterprise The fresh install on replicated-01 stopped before application deployment on the shared test infrastructure's storage preflight: its nominal 200 GB EBS volume exposes only ~192.7 GiB to Kubernetes, below the 200 GiB gate. That is unrelated to this chart diff and is tracked separately in OpenHands/replicated-testing#1. The earlier rendered-collector and read-only beta validations remain green. |
|
The minimal 1Finity maintenance-line backport is now open as #1234. It targets |
Summary
openhands-budget-diagnosticssupport-bundle collector for OHE-3260Why
Today a customer support bundle cannot establish whether an org has the budget upgrade/reconciliation states tracked in OHE-3253 through OHE-3256. This gives Support an initial diagnostic slice that can identify missing baselines, failed maintenance, and stale LiteLLM caps without direct customer database access.
This intentionally does not attempt repair and does not fully diagnose OHE-3252 managed-key ownership; that needs a separately reviewed key-safe diagnostic or targeted operator inspection.
Validation
git diff --checkpassedTracking