feat(operator): add TriageRun SDK - #104
Merged
Merged
Conversation
amwarrier
added
the
release:minor
ChatGPT Codex Connector
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Kubernetes dynamic-client API for TriageRun creation, listing, retrieval, and protected deletion. The API validates inputs, applies defaults, generates metadata, parses status data, sorts history, and exposes typed results with contextual errors. ChangesTriageRun API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TriageRunAPI
participant KubernetesDynamicClient
participant TriageRunResource
Caller->>TriageRunAPI: create TriageRunRequest
TriageRunAPI->>KubernetesDynamicClient: create TriageRun
KubernetesDynamicClient->>TriageRunResource: store resource
TriageRunResource-->>KubernetesDynamicClient: return resource reference
KubernetesDynamicClient-->>TriageRunAPI: return namespace and name
TriageRunAPI-->>Caller: return TriageRunRef
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
amwarrier
changed the title
amwarrier
marked this pull request as ready for review
July 31, 2026 19:40
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/operator/triage.go (1)
176-216: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider server-side filtering as TriageRun history grows.
listTriageRunsfetches every TriageRun in the namespace withmetav1.ListOptions{}(Line 190-193), then discards non-matching applications after parsing each item (Line 204-206). There is no label selector and noLimit/Continuepagination. As run history accumulates across applications in a shared namespace, this list-then-filter approach does more API server and client work than necessary on every call.Add an application-identifying label at creation time (alongside
triageRunManagedByLabelinnewTriageRun, Lines 465-467) and filter with a label selector whenApplicationNameis set. This reduces the payload size and parsing cost for namespaces with many runs across applications.🤖 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 `@pkg/operator/triage.go` around lines 176 - 216, Add a stable application-identifying label in newTriageRun alongside triageRunManagedByLabel, using the application name value. In listTriageRuns, build ListOptions with a label selector for that label when request.ApplicationName is set, while retaining the unfiltered listing behavior otherwise; continue validating and applying the existing application-name filtering as needed.
🤖 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 `@pkg/operator/triage.go`:
- Around line 255-261: Update deleteTriageRun’s phase handling to default an
empty status.phase to "Pending" before the terminal-state comparison and error
message, matching parseTriageRun’s behavior. Preserve the existing Succeeded and
Failed handling and ErrTriageRunNotTerminal response.
- Around line 102-174: Update CreateTriageRun/createTriageRun to wait for
operator readiness via Operator readiness handling before attempting the
mutation, ensuring webhook and deployment dependencies are available. Replace
the direct dynamicClient Resource(...).Create call and its wsm FieldManager with
the established operator-approved v2 application/CR mutation path used for
TriageRun resources, while preserving request validation, default action
handling, and returned reference behavior.
---
Nitpick comments:
In `@pkg/operator/triage.go`:
- Around line 176-216: Add a stable application-identifying label in
newTriageRun alongside triageRunManagedByLabel, using the application name
value. In listTriageRuns, build ListOptions with a label selector for that label
when request.ApplicationName is set, while retaining the unfiltered listing
behavior otherwise; continue validating and applying the existing
application-name filtering as needed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e36e3eb4-1b19-464f-83ab-da0ce933910b
📒 Files selected for processing (2)
pkg/operator/triage.gopkg/operator/triage_test.go
Comment on lines
+102
to
+174
| // CreateTriageRun creates a fresh TriageRun through wsm's configured dynamic | ||
| // Kubernetes client. It always uses generateName: repeated calls represent | ||
| // distinct execution requests rather than updates or retries of an earlier run. | ||
| func CreateTriageRun(ctx context.Context, request TriageRunRequest) (TriageRunRef, error) { | ||
| _, dynamicClient, err := kubectl.GetDynamicClientset() | ||
| if err != nil { | ||
| return TriageRunRef{}, err | ||
| } | ||
| return createTriageRun(ctx, dynamicClient, request) | ||
| } | ||
|
|
||
| // ListTriageRuns returns newest-first run history in one namespace. Supplying | ||
| // ApplicationName filters the history without excluding runs created outside | ||
| // wsm. | ||
| func ListTriageRuns(ctx context.Context, request ListTriageRunsRequest) ([]TriageRun, error) { | ||
| _, dynamicClient, err := kubectl.GetDynamicClientset() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return listTriageRuns(ctx, dynamicClient, request) | ||
| } | ||
|
|
||
| // GetTriageRun returns one run and its latest status. | ||
| func GetTriageRun(ctx context.Context, namespace, name string) (TriageRun, error) { | ||
| _, dynamicClient, err := kubectl.GetDynamicClientset() | ||
| if err != nil { | ||
| return TriageRun{}, err | ||
| } | ||
| return getTriageRun(ctx, dynamicClient, namespace, name) | ||
| } | ||
|
|
||
| // DeleteTriageRun removes a completed run. Pending and Running runs are not | ||
| // deleted because deletion would implicitly cancel their owned Job; cancellation | ||
| // needs a separate explicit contract. | ||
| func DeleteTriageRun(ctx context.Context, namespace, name string) error { | ||
| _, dynamicClient, err := kubectl.GetDynamicClientset() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return deleteTriageRun(ctx, dynamicClient, namespace, name) | ||
| } | ||
|
|
||
| func createTriageRun( | ||
| ctx context.Context, | ||
| dynamicClient dynamic.Interface, | ||
| request TriageRunRequest, | ||
| ) (TriageRunRef, error) { | ||
| if err := validateTriageRunRequest(request); err != nil { | ||
| return TriageRunRef{}, err | ||
| } | ||
| if request.Action == "" { | ||
| request.Action = DefaultTriageAction | ||
| } | ||
|
|
||
| created, err := dynamicClient.Resource(triageRunsV2GVR).Namespace(request.Namespace).Create( | ||
| ctx, | ||
| newTriageRun(request), | ||
| metav1.CreateOptions{FieldManager: "wsm"}, | ||
| ) | ||
| if err != nil { | ||
| return TriageRunRef{}, fmt.Errorf( | ||
| "failed to create TriageRun for Application %s/%s: %w", | ||
| request.Namespace, | ||
| request.ApplicationName, | ||
| err, | ||
| ) | ||
| } | ||
|
|
||
| return TriageRunRef{ | ||
| Namespace: created.GetNamespace(), | ||
| Name: created.GetName(), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WaitForOperator usages =="
rg -n "WaitForOperator" -g '*.go' -C3
echo "== FieldManager wsm usages =="
rg -n 'FieldManager:\s*"wsm"' -g '*.go' -C3
echo "== dynamic client Apply usages in pkg/operator =="
rg -n '\.Apply\(' -g '*.go' pkg/operator
echo "== GetDynamicClientset definition =="
rg -n "func GetDynamicClientset" -g '*.go' -A 30Repository: wandb/wsm
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
git ls-files '*.go' | sed -n '1,200p'
echo "== wait operator exact =="
rg -n "WaitForOperator|Webhook|CA bundle|webhook" -g '*.go' . -C 2 || true
echo "== list operator triage =="
git ls-files 'pkg/operator/*.go' 'pkg/helm/*.go' | sed -n '1,200p'
echo "== triage.go outline =="
ast-grep outline pkg/operator/triage.go --view expanded || true
echo "== triage.go relevant slices =="
wc -l pkg/operator/triage.go
sed -n '1,230p' pkg/operator/triage.go
echo "== dynamic clientset usages =="
rg -n "GetDynamicClientset|dynamicClient" -g '*.go' . -C 3 || true
echo "== v2 tria runs references =="
rg -n "triageRunsV2GVR|TriageRun|Server-side|patch\(|apply\(|Create\(|GenerateName|fieldManager" -g '*.go' . -C 2 || trueRepository: wandb/wsm
Length of output: 50365
🏁 Script executed:
#!/bin/bash
set -u
echo "== GetDynamicClientset =="
sed -n '120,155p' pkg/kubectl/kubectl.go
echo "== operator readiness wait implementation =="
sed -n '954,1028p' pkg/operator/operator.go
echo "== static call/condition analysis around CreateTriageRun path =="
python3 - <<'PY'
from pathlib import Path
triage = Path("pkg/operator/triage.go").read_text()
op = Path("pkg/operator/operator.go").read_text()
kubectl = Path("pkg/kubectl/kubectl.go").read_text()
checks = {
"CreateTriageRun_directly_returns_createTriageRun": "return createTriageRun(ctx, dynamicClient, request)" in triage,
"createTriageRun_unconditionally_calls_dynamic_client_Create": triage.count("dynamicClient.Resource(triageRunsV2GVR).Namespace(request.Namespace).Create(") >= 1,
"createTriageRun_no_before_Create_WaitForOperator_call": triage[:triage.index("dynamicClient.Resource(triageRunsV2GVR).Namespace(request.Namespace).Create(")].count("WaitForOperator") == 0,
"createTriageRun_uses_CreateOptions_FieldManager_wsm": "metav1.CreateOptions{FieldManager: \"wsm\"}" in triage,
"createTriageRun_no_Patch_Apply_call": triage[:triage.index("dynamicClient.Resource(triageRunsV2GVR).Namespace(request.Namespace).Create(")].count(".Apply(") == 0 and triage[:triage.index("dynamicClient.Resource(triageRunsV2GVR).Namespace(request.Namespace).Create(")].count(".Patch(") == 0,
"WaitForOperator_no_argument": "func WaitForOperator(ctx context.Context, namespace string, timeout time.Duration) error" in op,
"GetDynamicClientset_no_timeout_context_argument": "func GetDynamicClientset() (*rest.Config, *dynamic.DynamicClient, error)" in kubectl,
}
for k, v in checks.items():
print(f"{k}={v}")
PYRepository: wandb/wsm
Length of output: 3629
Wait for operator readiness and use an operator-approved v2 CR mutation path.
CreateTriageRun calls kubectl.GetDynamicClientset(), which only initializes the Kubernetes REST config/client, then creates TriageRun with .Create(). This path does not call operator.WaitForOperator, so it can race the webhook CA bundle and operator deployment. Also, TriageRun is a v2 CR, so create it through an established v2 application/CR mutation path instead of mixing FieldManager: "wsm" into a Kubernetes CreateOptions on an operator-managed resource.
🤖 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 `@pkg/operator/triage.go` around lines 102 - 174, Update
CreateTriageRun/createTriageRun to wait for operator readiness via Operator
readiness handling before attempting the mutation, ensuring webhook and
deployment dependencies are available. Replace the direct dynamicClient
Resource(...).Create call and its wsm FieldManager with the established
operator-approved v2 application/CR mutation path used for TriageRun resources,
while preserving request validation, default action handling, and returned
reference behavior.
Source: Coding guidelines
collinol
approved these changes
Aug 3, 2026
collinol
left a comment
collinol
left a comment
Contributor
There was a problem hiding this comment.
Approved but make this change #104 (comment)
5 tasks
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
2 participants
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.
Summary of Changes
Jira: N/A
Adds the shared WSM SDK operations Watchtower and other consumers use to manage application diagnostic runs:
CreateTriageRuncreates a fresh namespacedapps.wandb.com/v2TriageRunListTriageRunsreturns newest-first history, optionally filtered by applicationGetTriageRunreturns the latest phase, summary, structured results, evidence, and remediationDeleteTriageRunremoves terminal history onlyEach creation uses
metadata.generateName, so requests at T0 and T1 create distinct immutable run objects rather than updating or replaying an earlier run. Requests default to thedefaultaction and validate namespace/application identifiers before touching the cluster.Deletion deliberately rejects
PendingandRunningruns. Deleting an active TriageRun would implicitly cancel its owned Job, and cancellation should be a separate explicit contract. Terminal deletion uses a Kubernetes UID precondition to avoid deleting a replacement object with the same name.The implementation uses
unstructured.Unstructuredrather than coupling WSM to an unreleased operator API package. It reuses WSM's existing cached dynamic Kubernetes client.Dependency-ordered rollout
TriageRunTest Plan
GOWORK=off GOCACHE=/private/tmp/wsm-triage-go-cache go test ./...GOWORK=off GOCACHE=/private/tmp/wsm-triage-go-cache go vet ./...git diff --checkRequirements
Summary by CodeRabbit
New Features
Tests