feat(api): publish restore outcome classifier - #130
Conversation
Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe API now defines shared restore outcomes and classifies Pod conditions. The restore controller uses these outcomes and shared reason constants. Controller tests now validate the exported constants. ChangesRestore outcome handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change publishes restore outcome classification and updates the node agent to use the public constants and classifier; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RestoreController
participant ClassifyRestoreOutcome
participant RestoredPodConditions
RestoreController->>ClassifyRestoreOutcome: classify conditions
ClassifyRestoreOutcome->>RestoredPodConditions: scan Restored condition
RestoredPodConditions-->>ClassifyRestoreOutcome: status and reason
ClassifyRestoreOutcome-->>RestoreController: RestoreOutcome
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Full details: Breaking Api ChangesExplanation No breaking API change matches the custom check. The exact pull-request diff adds only Full details: Rbac Least PrivilegeExplanation PASS — The pull request changes only controller and API status-classification code and tests. It adds no kubebuilder RBAC markers or Helm RBAC manifests. Repository RBAC markers and Helm Role/ClusterRole rules use explicit verbs and resources; no wildcard
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
galletas1712
left a comment
galletas1712
left a comment
There was a problem hiding this comment.
One thing to watch for when later PRs land — details on the classifier below.
| return RestoreOutcomeFailed | ||
| case RestoreReasonPartiallySucceeded: | ||
| return RestoreOutcomePartiallySucceeded | ||
| default: |
There was a problem hiding this comment.
Right now, any reason this function doesn't recognize is treated as "restore is still in progress." That's fine for "we're waiting for the snapshot."
There's a draft PR (#97) that will add a new final reason: RestoreIncompatible. That means the node looked at the checkpoint and said no. This code would call that "still in progress," and Dynamo would wait forever instead of starting a new Pod.
You don't have to change this PR for that. When #97 lands, add a case for RestoreIncompatible. A test that goes through every RestoreReason* and checks the finished ones aren't reported as Pending would keep this from happening again.
There was a problem hiding this comment.
Agreed. #130 intentionally classifies only the public reasons that exist today. I am leaving it unchanged as suggested; when #97 lands, RestoreIncompatible should be added to the terminal classifier together with an exhaustive test over every exported RestoreReason* constant, so a new terminal outcome cannot silently fall back to Pending.
2 participants
Summary
Pending,Succeeded,Failed, andPartiallySucceededrestore outcomesnvidia.com/RestoredPod condition without requiring consumers to parse private dependency reasonsPart of ai-dynamo/dynamo#13919.
Validation
GOCACHE=/private/tmp/snapshot-outcomes-go-cache go test ./...inapi/make linux-test(all agent packages)agent/internal/controllermake lintcould not run locally because the installed golangci-lint binary was built with Go 1.24, below this repository's Go 1.26.6 module version.Summary by CodeRabbit
New Features
Bug Fixes