feat(api): publish restore outcome classifier by julienmancuso · Pull Request #130 · ai-dynamo/snapshot

julienmancuso · 2026-08-28T01:45:37Z

Summary

publish stable Unknown, Pending, Succeeded, Failed, and PartiallySucceeded restore outcomes
classify the public nvidia.com/Restored Pod condition without defaulting unrecognized statuses or reasons to Pending
make the node agent use the public terminal reason constants and recognize only explicit terminal outcomes

Validation

GOCACHE=/private/tmp/snapshot-outcomes-go-cache go test ./... in api/
make linux-test (all agent packages)
Linux cross-compilation of agent/internal/controller

make lint could 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
- Added standardized restore status reporting for pending, successful, failed, and partially successful restores.
- Improved handling of restore outcomes when status conditions are missing, unknown, or incomplete.
Bug Fixes
- Restore completion detection now consistently reflects the reported restore outcome.
- Improved reliability when determining whether a restore has reached a terminal state.

Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>

coderabbitai · 2026-08-28T01:46:36Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 6d36e7ba-7076-4f78-8320-a37a4158875b

📥 Commits

Reviewing files that changed from the base of the PR and between 6c19957 and 32cb9ce.

📒 Files selected for processing (4)

agent/internal/controller/controller.go
agent/internal/controller/controller_test.go
api/v1alpha1/restore_status.go
api/v1alpha1/restore_status_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

The API now defines shared restore outcomes and classifies Pod conditions. The restore controller uses these outcomes and shared reason constants. Controller tests validate terminal detection and exported constants.

Changes

Restore outcome handling

Layer / File(s)	Summary
Define and test restore outcome classification `api/v1alpha1/restore_status.go`, `api/v1alpha1/restore_status_test.go`	Adds restore outcome and reason constants. `ClassifyRestoreOutcome` maps conditions to pending, succeeded, failed, partially succeeded, or unknown outcomes.
Use shared restore outcomes in the controller `agent/internal/controller/controller.go`	Removes local reason constants and uses shared constants and `ClassifyRestoreOutcome` for restore status handling.
Align controller tests with the shared contract `agent/internal/controller/controller_test.go`	Adds terminal-detection coverage and updates restore fixtures and assertions to use exported API reason constants.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 32cb9

This PR publishes explicit restore outcome classifications and updates the node agent to recognize terminal outcomes without defaulting unknown values to Pending; 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)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (6 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Breaking Api Changes	✅ Passed	PASS. The PR API diff adds only `api/v1alpha1/restore_status.go` and its test. The new source defines `RestoreOutcome`, constants, and `ClassifyRestoreOutcome`; it adds no exported fields or JSON tags…
Rbac Least Privilege	✅ Passed	PASS: The pull request changes only restore API and controller code. It does not add or modify kubebuilder RBAC markers or Helm RBAC manifests. Repository inspection found no wildcard `verbs: ["*"]`, …
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title uses the required conventional commit prefix `feat`, is 45 characters long, and accurately describes the public restore outcome classifier added by the pull request.

Full details: Breaking Api Changes

Explanation

PASS. The PR API diff adds only api/v1alpha1/restore_status.go and its test. The new source defines RestoreOutcome, constants, and ClassifyRestoreOutcome; it adds no exported fields or JSON tags. No existing API type files, CRDs, generated deepcopy code, or XValidation markers changed. Therefore the check finds no breaking API change or missing optional/default marker.

Full details: Rbac Least Privilege

Explanation

PASS: The pull request changes only restore API and controller code. It does not add or modify kubebuilder RBAC markers or Helm RBAC manifests. Repository inspection found no wildcard verbs: ["*"], resources: ["*"], or wildcard marker grants. Existing RBAC rules enumerate specific verbs and resources.

Fix all pre-merge checks with AI

_{Comment @coderabbitai help to get the list of available commands.}

galletas1712

One thing to watch for when later PRs land — details on the classifier below.

galletas1712 · 2026-08-28T23:32:30Z

+			return RestoreOutcomeFailed
+		case RestoreReasonPartiallySucceeded:
+			return RestoreOutcomePartiallySucceeded
+		default:


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.

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.

I have issue with defaulting to Pending, as it might forgot by other ppl. better to have unkown rather a wrong status. if you really need it you can add it to dynamo and not here

Agreed and fixed in 32cb9ce. The public classifier now returns a distinct RestoreOutcomeUnknown for any unrecognized condition status or reason, so it no longer asserts Pending when this API version cannot know. Missing conditions and explicit RestoreInProgress remain Pending. The agent's terminal check now matches only Succeeded, Failed, and PartiallySucceeded; Unknown is not implicitly treated as terminal. Tests cover both an unrecognized dependency reason and the future-terminal RestoreIncompatible example.

Ronkahn21

I have issue to default on restore pending as this is might not true, I raher say I dont know to say it pending

Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>

feat(api): publish restore outcome classifier

6c19957

Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>

julienmancuso requested review from Ronkahn21, danbar2, dfeigin-nv, galletas1712, hhzhang16, hutm, oleg-kushniriov, shayasoolin and shmuel-runai as code owners August 28, 2026 01:45

galletas1712 reviewed Aug 28, 2026

View reviewed changes

galletas1712 approved these changes Aug 29, 2026

View reviewed changes

Ronkahn21 reviewed Aug 30, 2026

View reviewed changes

fix(api): classify unknown restore outcomes

32cb9ce

Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>

Navigation Menu

Uh oh!

feat(api): publish restore outcome classifier - #130

feat(api): publish restore outcome classifier#130
julienmancuso wants to merge 2 commits into
mainfrom
jsm/restore-outcomes

julienmancuso commented Aug 28, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

galletas1712 left a comment

Uh oh!

galletas1712 Aug 28, 2026

Uh oh!

julienmancuso Aug 29, 2026

Uh oh!

Ronkahn21 Aug 30, 2026

Uh oh!

julienmancuso Aug 30, 2026

Uh oh!

Ronkahn21 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

julienmancuso commented Aug 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

galletas1712 left a comment

Choose a reason for hiding this comment

Uh oh!

galletas1712 Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

julienmancuso Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Ronkahn21 Aug 30, 2026

Choose a reason for hiding this comment

Uh oh!

julienmancuso Aug 30, 2026

Choose a reason for hiding this comment

Uh oh!

Ronkahn21 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

julienmancuso commented Aug 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading