fix(controller): prevent false WorkloadRun failure on Workflow cache miss by kaynetu · Pull Request #370 · NVIDIA/cluster-readiness-engine

kaynetu · 2026-09-23T22:05:51Z

Summary

A WorkloadRun can observe its persisted workflowRef before the new Workflow reaches the manager cache. A cached NotFound previously persisted terminal Failed / WorkflowDeleted and produced a misleading Warning Event.

Confirm a cached Workflow miss with the uncached API reader. Wait without changing status when the API server still has the Workflow, fail only on confirmed absence, and retry other read errors.
Default the API reader in SetupWithManager so a manager-backed reconciler cannot silently recheck the same stale cache.
Restore the full WorkloadRun creation-to-success and MPI lifecycle fixtures, including Normal / WorkflowCreated, and add deterministic cache-lag, confirmed-deletion, and live-read-error coverage.

Related Issue

Closes #352. Follow-up to #349.

Type of Change

Component(s) Affected

Testing

Tests pass locally
Manual testing completed
No breaking changes (or documented)

make lint, make build, and the full make test suite passed before the final rebase onto current main. After that rebase, go test ./pkg/controller -count=1, the three affected WorkloadRun integration fixtures with -race, and git diff --check passed. The affected fixtures also passed three fresh-process race runs before the rebase. The final branch is one signed, DCO-signed commit ahead of main; no live-cluster UAT was run.

Risk Assessment

The extra API read occurs only after a cached Workflow miss. A live-read error returns for retry without changing WorkloadRun status; a confirmed API NotFound retains the existing WorkflowDeleted failure and Warning. No CRD schema, RBAC, chart, or dependency changes.

Checklist

Self-review completed
Commits are signed off for the DCO (git commit -s)
make manifests generate run as part of make build; no *_types.go changed
Golden files updated for the restored lifecycle Events and confirmed deletion
Documentation updated
Ready for review

A WorkloadRun can observe workflowRef before its Workflow reaches the manager cache. Confirm a cache miss through the uncached API reader; wait while the Workflow exists, fail only on confirmed absence, and retry other read errors. Default the API reader in manager setup, restore full lifecycle Event fixtures, and cover cache lag, confirmed deletion, and live-read failure. Closes NVIDIA#352 Signed-off-by: Kayne Tu <kaynet@nvidia.com>

coderabbitai · 2026-09-23T22:11:36Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: NVIDIA/cluster-readiness-engine/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1e5ddccf-26cd-4d99-a10e-32a39dffd381

📥 Commits

Reviewing files that changed from the base of the PR and between 3d04a74 and 52a1aab.

⛔ Files ignored due to path filters (16)

cmd/integration/testdata/reconcile/workloadrun-event-success/expected.json is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-event-success/input_client_objects.yaml is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-event-success/input_config.yaml is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-mpi/expected.json is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-mpi/input_config.yaml is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-workflow-deleted/expected.json is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-workflow-deleted/input_client_objects.yaml is excluded by !**/testdata/**
cmd/integration/testdata/reconcile/workloadrun-workflow-deleted/input_config.yaml is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/api-read-error/expected.json is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/api-read-error/input.yaml is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/cache-hit/expected.json is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/cache-hit/input.yaml is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/cache-lag/expected.json is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/cache-lag/input.yaml is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/confirmed-deletion/expected.json is excluded by !**/testdata/**
pkg/controller/testdata/workloadrun-workflow-lookup/confirmed-deletion/input.yaml is excluded by !**/testdata/**

📒 Files selected for processing (5)

cmd/integration/integration_test.go
cmd/manager/main.go
docs/designs/080-phase-transition-events.md
pkg/controller/workloadrun_controller.go
pkg/controller/workloadrun_workflow_lookup_test.go

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

📝 Walkthrough

Walkthrough

The WorkloadRun reconciler now uses an uncached API reader to verify a Workflow after a cached lookup returns NotFound. It requeues when the live read finds the Workflow, returns other live-read errors, and marks the WorkloadRun failed when the live read confirms deletion. Manager and integration test setup pass the API reader. Integration tests no longer directly reconcile WorkloadRuns before manager startup.

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

Merge Risk: ⚪ Minimal · up to 52a1a

The change avoids false deletion failures during cache lag while preserving confirmed-deletion handling and lifecycle assertions. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main behavior change: preventing false WorkloadRun failure when a Workflow is missing from the cache.
Description check	✅ Passed	The description directly explains the cache-miss fix, API-reader behavior, tests, documentation, and related issue.
Linked Issues check	✅ Passed	Issue `#352` requires the reconciler to distinguish cache lag from confirmed Workflow deletion. The change uses the uncached API reader after a cached `NotFound`. A live Workflow causes requeue without …
Out of Scope Changes check	✅ Passed	The reviewed changes stay connected to issue `#352`. Manager wiring enables the required uncached read. Integration harness changes run lifecycle checks through the manager. Documentation and lifecycle …
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 4 files. (1 skipped: 1 u…

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

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

github-actions · 2026-09-23T22:30:29Z

🌿 Preview your docs: https://nvidia-preview-pull-request-370.docs.buildwithfern.com/cluster-readiness-engine

asivanadi0

Tip review 52a1aab (closes #352)

Checked against #352 acceptance criteria and the follow-up comment on restoring Normal / WorkflowCreated:

Cache lag vs confirmed absence: mirrorWorkflowStatus only fails closed after an uncached APIReader NotFound; a live hit requeues without status/Event writes; other live-read errors return for retry with no terminal write. Matches the issue.
Wiring: SetupWithManager defaults APIReader (covered by TestWorkloadRunSetupDefaultsAPIReader); main.go and the integration manager also pass mgr.GetAPIReader(), so a manager-backed reconciler cannot confirm against the same cache.
Tests: workloadrun-workflow-lookup goldens cover cache-hit (0 API reads), cache-lag (requeue, no Event), confirmed deletion (Failed/WorkflowDeleted + Warning), and live-read error (error returned, status unchanged). Integration restores full create→success / MPI lifecycle with WorkflowCreated, and adds workloadrun-workflow-deleted for confirmed absence.
Docs: ADR-080 marks #352 resolved and documents the restored fixtures consistently.
CI on this tip: Lint/Test/Build/UAT/DCO and related checks are green (not thin / not awaiting /ok).

Non-blocking nit: on the cache-lag path the live Get already populated workflow, but the reconciler requeues instead of mirroring from that object. Fine given the Owns-watch safety net and the rare race window; optional follow-up if you want to shave the worst-case 15s wait.

Approve.

kaynetu requested review from lalitadithya and ndipebot as code owners September 23, 2026 22:05

asivanadi0 approved these changes Sep 24, 2026

View reviewed changes

Navigation Menu

fix(controller): prevent false WorkloadRun failure on Workflow cache miss - #370

fix(controller): prevent false WorkloadRun failure on Workflow cache miss#370
kaynetu wants to merge 1 commit into
NVIDIA:mainfrom
kaynetu:kaynet/issue-352-workloadrun-workflow-cache-miss

kaynetu commented Sep 23, 2026

Uh oh!

coderabbitai Bot commented Sep 23, 2026

Walkthrough

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

asivanadi0 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kaynetu commented Sep 23, 2026

Summary

Related Issue

Type of Change

Component(s) Affected

Testing

Risk Assessment

Checklist

Uh oh!

coderabbitai Bot commented Sep 23, 2026

Walkthrough

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

asivanadi0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants