fix(cache): restore Dataset to Bound after CacheRuntime recovers from an outage - #6162
fix(cache): restore Dataset to Bound after CacheRuntime recovers from an outage#6162pujitha24 wants to merge 4 commits into
Conversation
… an outage Motivation: A CacheRuntime-backed Dataset that flips to Failed during a transient runtime outage (e.g. a worker pod restart) never returns to Bound, even after the runtime becomes fully Ready again. Only deleting and recreating the CacheRuntime restored the correct state. Approach: CacheEngine.Sync only refreshed cache states when the runtime was ready, without touching the Dataset's phase. The Bound phase was otherwise only ever set once, by BindToDataset during initial Setup, which does not run again on later reconciles, so the Failed phase was a one-way trap. Sync now checks the Dataset's current phase whenever the runtime is ready and, if it is Failed, restores it to Bound via the existing UpdateDatasetStatus helper before falling back to the regular cache-states sync. Validation: - go build ./... - go vet ./pkg/ddc/cache/... - gofmt -l pkg/ddc/cache/engine/sync.go pkg/ddc/cache/engine/sync_test.go (no output) - golangci-lint run ./pkg/ddc/cache/... -> 0 issues - go test ./pkg/ddc/cache/engine/... -run TestCacheEngine --ginkgo.focus="left Failed by a previous outage" -v -> PASS - Confirmed the new test is a genuine regression test: reverting only sync.go while keeping the new test makes it fail with Failed != Bound - Full unfocused suite in this package shows pre-existing, order-dependent flaky failures in ufs_test.go/dataset_test.go/fileutils_test.go that reproduce identically on unmodified master, unrelated to this change Report: fluid-cloudnative#6160 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @pujitha24. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6162 +/- ##
==========================================
+ Coverage 65.13% 65.19% +0.06%
==========================================
Files 485 485
Lines 34039 34053 +14
==========================================
+ Hits 22171 22202 +31
+ Misses 10127 10108 -19
- Partials 1741 1743 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
I checked this against a real cluster rather than only reading it, and the fix holds up. Across five induced worker outages, master leaves the Dataset in Failed all five times and never recovers, while with this change it returns to Bound every time.
Two things I verified because they're easy to get wrong. The root cause is what you describe: IsSetupDone keys off the presence of a DatasetReady condition and ignores its status, and the Failed path writes that condition, so Setup and BindToDataset really never run again. Your regression test is a genuine one as well. Reverting only sync.go fails it with Failed against Bound, exactly as you reported.
There's one thing I'd like sorted out in this round before I add the merge labels, inline below. The restore calls UpdateDatasetStatus(Bound), which pod-execs through GetCacheStates, and it does so outside the permitSyncEngineStatus guard that exists to bound exactly those calls. In practice the cost is small, about one extra exec per recovery by my measurement rather than anything dramatic, so I'm not treating it as urgent. It's just small and local enough that fixing it here beats leaving a known invariant violation in place.
The test notes in the second comment are optional and I won't hold anything on them.
| // phase because the phase is otherwise only restored to Bound by the mount flow, | ||
| // which does not run on a normal reconcile. Restore it here. | ||
| e.Log.Info("runtime is ready again, restoring dataset phase from Failed to Bound") | ||
| err = e.UpdateDatasetStatus(datav1alpha1.BoundDatasetPhase, runtime, runtimeClass) |
There was a problem hiding this comment.
UpdateDatasetStatus(BoundDatasetPhase, ...) isn't a cheap phase write. For the Bound case, dataset.go:49-55 calls GetCacheStates, which execs into the master pod via ExecCommandInContainerWithTimeout with a 20s timeout floor (MinExecutionTimeoutSeconds). That call sits outside permitSyncEngineStatus, and only the else if below still carries the guard, so the restore path issues exactly the sort of unthrottled RPC that the comment on line 38 says the limiter exists to bound.
Alluxio is a useful comparison here, since it does the same recovery and is also called outside the limiter (base/syncs.go:63 into alluxio/health_check.go:81). That's fine there for two reasons the cache engine's version lacks: its UpdateDatasetStatus writes only phase, condition, mounts and runtimes with no RPC, and it wraps the transition in if phase != dataset.Status.Phase, so calling it again costs nothing. Cache states get refreshed separately by UpdateCacheOfDataset().
I measured this rather than guessing, and I want to be straight about the size of it. In a unit test, driving three not-ready to ready flaps inside a single 5s window produces 3 execs where at most 1 should happen. On a real cluster it's much milder: across five induced worker outages in 47s, the change added one exec attempt (6 versus 5 without it), because a real recovery cycle takes longer than 5s and so tends to get its own window. So this is not a production hazard, and I'm not claiming it is.
Even so, I'd rather see it handled in this PR than carried forward, because the fix is small and lives in code you're already touching. Making the restore phase-only and moving the idempotence check into the helper does it, and then this call site no longer needs the GetDataset above either:
current, err := utils.GetDataset(e.Client, e.name, e.namespace)
if err != nil {
return err
}
if current.Status.Phase == phase {
return nil
}
// pod exec with a 20s timeout floor, so keep it behind the limiter
if phase == datav1alpha1.BoundDatasetPhase && e.permitSync() {
cacheStates, err = e.GetCacheStates(runtime, runtimeClass)
...
}One approach I'd avoid: simply wrapping the restore in permitSyncEngineStatus. That makes recovery wait on the limiter rather than making it cheap, so it swaps this for a slower fix.
Smaller point about the current shape: on the reconcile that restores the phase, syncDatasetCacheStates gets skipped entirely, because the restore takes the if and the sync sits in the else if.
Harness and captured output, if it's useful: https://github.com/cheyang/fluid/tree/verify/cacheruntime-dataset-phase-restore/docs/verification/cacheruntime-dataset-phase-restore
| Namespace: "default", | ||
| }, updatedDataset) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(updatedDataset.Status.Phase).To(Equal(datav1alpha1.BoundDatasetPhase)) |
There was a problem hiding this comment.
This pins the phase but not the condition. UpdateDatasetStatus also flips DatasetReady back to ConditionTrue, and the condition is what IsSetupDone and the other consumers actually read, so it seems worth asserting too:
idx, cond := utils.GetDatasetCondition(updatedDataset.Status.Conditions, datav1alpha1.DatasetReady)
Expect(idx).NotTo(Equal(-1))
Expect(cond.Status).To(Equal(corev1.ConditionTrue))It would also help to seed the Dataset with a DatasetReady/False condition in the BeforeEach. That's what a real outage leaves behind, and it's the reason Setup never re-runs, so the fixture would then match the state the fix is actually for.
One path this case can't reach: the shared fixture leaves syncRetryDuration at its zero value, which makes permitSync() always return true and hides the closed-limiter branch completely. Setting it to defaultSyncRetryDuration covers the case where the limiter is shut.
There was a problem hiding this comment.
Pull request overview
This PR fixes a reconciliation bug in the CacheRuntime CacheEngine where a Dataset could remain permanently in Failed after a transient runtime outage, by restoring the Dataset phase back to Bound once the runtime becomes Ready again. It also adds a focused Ginkgo regression test to ensure the phase transition is restored on subsequent reconciles.
Changes:
- Update
CacheEngine.Syncto detectDataset.Status.Phase == Failedwhen the runtime is ready and restore it toBound. - Add a unit test covering the “runtime recovered but Dataset still Failed” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/ddc/cache/engine/sync.go | Restores Dataset phase from Failed to Bound when the runtime recovers. |
| pkg/ddc/cache/engine/sync_test.go | Adds a regression test validating the restored phase transition on recovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } else { | ||
| dataset, getErr := utils.GetDataset(e.Client, e.name, e.namespace) | ||
| if getErr != nil { | ||
| return getErr | ||
| } | ||
|
|
||
| if dataset.Status.Phase == datav1alpha1.FailedDatasetPhase { | ||
| // the runtime recovered from a previous outage but the dataset was left in Failed | ||
| // phase because the phase is otherwise only restored to Bound by the mount flow, | ||
| // which does not run on a normal reconcile. Restore it here. | ||
| e.Log.Info("runtime is ready again, restoring dataset phase from Failed to Bound") | ||
| err = e.UpdateDatasetStatus(datav1alpha1.BoundDatasetPhase, runtime, runtimeClass) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } else if permitSyncEngineStatus { | ||
| // sync dataset cache states when runtime is ready and sync permitted | ||
| e.Log.Info("sync dataset cache states") | ||
| err = e.syncDatasetCacheStates(ctx, runtime, runtimeClass) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
UpdateDatasetStatus now short-circuits when the dataset is already in the requested phase, and only execs into the master pod for cache states (GetCacheStates) when phase == Bound and the sync limiter permits it. Previously the Failed->Bound restore path in Sync() called this unconditionally, bypassing the same rate limiter that bounds every other engine RPC. Addresses review feedback from cheyang on PR fluid-cloudnative#6162. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
| // GetCacheStates execs into the master pod with a floor of MinExecutionTimeoutSeconds, | ||
| // so keep it behind the same rate limiter that bounds other engine RPCs, and only | ||
| // attempt it for BoundDatasetPhase. | ||
| if phase == datav1alpha1.BoundDatasetPhase && e.permitSync() { |
There was a problem hiding this comment.
I think the permitSync() method should be only called in sync method, it will make the sync logic clear.
UpdateDatasetStatus called e.permitSync() internally to decide whether to fetch cache states. Replace that with a fetchCacheStates parameter so the rate-limiting decision stays in Sync(), the only place permitSync is now called from. Addresses review feedback from xliuqq on PR fluid-cloudnative#6162. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
…he skip The "sync limiter is closed" test only checked the restored phase, so it stayed green even with cheyang's rate-limiter-bypass bug reintroduced. Patch GetCacheStates directly (NewCacheFileUtil isn't reachable in this Context since no ReportSummary entry is configured) and assert it's never called while the limiter is closed. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Ⅰ. Describe what this PR does
Fixes a bug where a CacheRuntime-backed Dataset gets permanently stuck in
Failedphase after a transient runtime outage (e.g. a worker pod restart), even after the
runtime becomes
Readyagain.Root cause: in
pkg/ddc/cache/engine/sync.go,CacheEngine.Syncsets the Dataset toFailedwhenever the runtime isn't ready, but when the runtime becomes ready again itonly calls
syncDatasetCacheStates, which never touches.Status.Phase. TheBoundphase is otherwise only set once, by
BindToDatasetduring the initialSetup(),which does not run again on subsequent reconciles. So once a Dataset flips to
Failed,nothing ever flips it back to
Bound, even though the underlying runtime has fullyrecovered.
The fix checks, on every reconcile where the runtime is ready, whether the Dataset is
currently
Failed; if so it restores it toBoundvia the existingUpdateDatasetStatushelper (the same helperBindToDatasetuses), instead of onlyrefreshing cache states.
Ⅱ. Does this pull request fix one issue?
fixes #6160
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added a Ginkgo test in
pkg/ddc/cache/engine/sync_test.go:"when runtime is ready but dataset was left Failed by a previous outage" — it seeds a
Dataset with
Status.Phase = FailedDatasetPhase, makes the master/workerStatefulSets report Ready, runs
engine.Sync(ctx), and asserts the Dataset's phase isrestored to
Bound.I confirmed this test is a genuine regression test for the bug: with only the test
added and the
sync.gofix reverted, it fails withFailed != Bound; with the fixapplied, it passes.
Ⅳ. Describe how to verify it
Commands run locally (all passed):
go build ./...go vet ./pkg/ddc/cache/...gofmt -l pkg/ddc/cache/engine/sync.go pkg/ddc/cache/engine/sync_test.go(no output)golangci-lint run ./pkg/ddc/cache/...→ "0 issues"go test ./pkg/ddc/cache/engine/... -run TestCacheEngine --ginkgo.focus="left Failed by a previous outage" -v→ PASSNote: running the full
pkg/ddc/cache/enginesuite unfocused shows some pre-existing,order-dependent flaky failures in
ufs_test.go/dataset_test.go/fileutils_test.go(unrelated files). These reproduce identically on unmodified
masterwith the samerandom spec ordering (verified by stashing this change and rerunning), so they are not
caused by this change.
I did not reproduce this against a live cluster; the fix and its regression test are
scoped to a single, deterministic phase-transition defect in
Sync, which thefocused unit test above demonstrates directly.
Ⅴ. Special notes for reviews
None.