Repository navigation
Conversation
|
[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 @frenemy17. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6219 +/- ##
==========================================
+ Coverage 65.61% 65.80% +0.18%
==========================================
Files 486 486
Lines 34361 34455 +94
==========================================
+ Hits 22546 22672 +126
+ Misses 10061 10027 -34
- Partials 1754 1756 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
Same-quota changes between disk-backed and memory-backed emptyDir storage still bypass drift detection.
1 open issue
What changed in this PR
This PR makes CacheRuntime worker tiered-store drift visible during reconciliation, addressing the silent configuration mismatch described in #6192.
Changes:
- Detects worker quota, hostPath, and missing or orphaned volume mismatches, including when worker resources are unspecified.
- Emits throttled
RuntimeTieredStoreConflictwarnings and adds drift-detection and cooldown tests. - Documents tiered-store update limitations in English and Chinese.
| File | Description |
|---|---|
pkg/ddc/cache/engine/sync.go |
Adds worker volume drift detection and warning emission. |
pkg/ddc/cache/engine/sync_test.go |
Tests drift scenarios, warning throttling, and cooldown reset. |
pkg/ddc/cache/engine/engine.go |
Adds warning interval and per-engine timestamp state. |
pkg/common/constants.go |
Defines the tiered-store conflict event reason. |
docs/zh/userguide/cache_runtime_tieredstore.md |
Adds Chinese update-limit and warning guidance. |
docs/zh/samples/cacheruntime/cacheruntime_spec_update.md |
Documents tiered-store update behavior in Chinese. |
docs/en/userguide/cache_runtime_tieredstore.md |
Adds English update-limit and warning guidance. |
docs/en/samples/cacheruntime/cacheruntime_spec_update.md |
Documents tiered-store update behavior in English. |
Review effort: Balanced
cheyang
left a comment
There was a problem hiding this comment.
Premise independently reproduced by both of us on the base branch — issue #6192's split (edited tieredStore reaches the ConfigMap while worker volumes keep creation-time sizes, with nothing reporting it) — and the fix is verified on this head: the worker drift warning fires through the full Sync path with quota, path, and missing/extra-volume coverage, no false positives after a real API-server round-trip, a 10-minute throttle with cooldown reset, detection still working when no worker resources are declared, and en+zh docs. CI is green on 58afa3a. Two things gate merge:
-
The remediation guidance doesn't work as written. The event says "Recreate the worker to apply changes." and the docs say "deleting and recreating the worker workload" — but nothing ever recreates a deleted worker AdvancedStatefulSet. Verified end-to-end: sync fails NotFound every 20 s, and the failure happens before the drift check, so this PR's own warning goes silent exactly when the user needs it. Only recreating the CacheRuntime converges. The wedge itself is pre-existing, but the text pointing users into it is new — please reword the event message and the ~6 doc sites (en+zh) to "delete and recreate the CacheRuntime".
-
The PR's only user-visible behavior has no coverage above the fake-client unit layer, and no tracked deferral. The 10-minute throttle survives across reconciles only via the controller's per-runtime engine cache, and live event emission is untested — a patch-then-assert-event case fits the existing test/gha-e2e/{mooncake,curvine} pattern exactly, or file a tracking issue with a timeline.
Non-blocking, folded below: emptyDir medium-only flips escape the check (one-line compare — Copilot flagged this too), the docs promise the warning for spec.client.tieredStore while detection is worker-only, and the package-level tieredStoreWarningInterval var is an unused test seam. The one real disagreement in the debate — whether the e2e gap blocks — settled toward blocking per the repo's test policy. Reviewer C (Copilot) agreed with both debaters on its single finding; nothing rests on Copilot alone.
Next round
[MUST] Reword the remediation guidance to "delete and recreate the CacheRuntime" — event message (pkg/ddc/cache/engine/sync.go:310) plus cacheruntime_spec_update.md and cache_runtime_tieredstore.md (en+zh); the current "recreate the worker workload" reading wedges sync and silences the warning.
[MUST] Cover the warning through a real controller loop: add a patch-tieredStore-then-assert-event e2e case in test/gha-e2e/{mooncake,curvine}, or file a tracked deferral issue with a timeline and reference it in the PR.
[SHOULD] Compare emptyDir Medium in checkWorkerTieredStoreDrift (sync.go:283) so disk<->Memory flips are detected.
[SHOULD] Scope the docs' warning claim to spec.worker.tieredStore and open a follow-up issue for client-side drift detection (also answer: does the event path remain the fallback when the #6193 webhook is disabled?).
[SHOULD] Replace the package-level tieredStoreWarningInterval var with a per-engine field or a const.
[Filed] none
Non-blocking observations (4) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/ddc/cache/engine/sync.go:283 [minor] — emptyDir medium-only edits (disk <-> Memory) are invisible to the drift check
Consensus finding, three-way agreement (Claude F2 / TS-MEDIUM-BLINDSPOT, Codex F2, Copilot C4367295361). The emptyDir branch ofcheckWorkerTieredStoreDriftcompares onlySizeLimit, neverMedium. Flipping an emptyDir level's medium between disk ("") and "Memory" is not cosmetic: the creation path charges the quota to container memory only whenMedium == Memory(handleEmptyDir), and the ConfigMap reportsmediumTypeMEM vs HDD from spec (extractTieredStoreLevels). After a medium-only flip the ConfigMap and resource accounting say tmpfs while the worker volume stays disk-backed with the same name and quota — a real silent split, the class of failure this PR exists to surface. Narrow trigger (a simultaneous quota change is still caught; the processMemory branch needs no medium check since the creation path pins StorageMediumMemory). One-line fix:else if vol.EmptyDir.Medium != level.EmptyDir.Medium { ... }in the emptyDir branch, plus regression tests for both medium-only transitions. - docs/en/samples/cacheruntime/cacheruntime_spec_update.md:145 [minor] — Docs promise the drift warning for spec.{worker,client}.tieredStore, but detection is worker-only
Consensus finding (Claude F1 / TS-DOC-CLIENT, Codex F3; provenance adjudicated — see verdict). Section 4.1 lists the field path asspec.{worker,client}.tieredStore, the summary-table row (":163", en+zh) says a tieredStore edit "Emits RuntimeTieredStoreConflict Warning event on drift" with no worker qualifier, and the userguide note 8 (cache_runtime_tieredstore.md:229, en+zh) says "Modifying tieredStore on an existing CacheRuntime will trigger" the event. ButcheckWorkerTieredStoreDriftonly ever receives the worker pod spec: editingspec.client.tieredStoreproduces the same split (the ConfigMap's client levels are rebuilt from spec every sync while the client DaemonSet is never re-synced after creation —ShouldSetupClientonly runs atStatus.Client.Phase == RuntimePhaseNone) and no event fires. The drift-warning bullet at :148 is already worker-scoped, so the edit is concentrated in the field-path header, the summary-table row, and userguide note 8. The underlying client silence is pre-existing (origin of the behavior); the over-promising doc wording is introduced by this PR. Fine to ship worker-only — issue #6192's asks are worker-scoped — but scope the doc lines tospec.worker.tieredStoreand track the client case as a follow-up (see TS-CLIENT-FOLLOWUP). - pkg/ddc/cache/engine/sync.go:234 [minor] — Is client-side drift detection tracked as a follow-up, and does the event path remain the fallback when the #6193 webhook is disabled?
Merged open question (Claude TS-CLIENT-SCOPE, Codex Q1). Issue #6192 explicitly notes the client case ("spec.client.tieredStore should behave the same way, going by the code … I have only reproduced the worker case"), and the PR implements worker-only detection while the docs read as covering {worker,client} (see TS-DOC-CLIENT-SCOPE). Which is intended? If worker-only is the scope decision, the docs need scoping and a follow-up issue for the client case; if not, extending the check to the client DaemonSet pod spec is small (the same helper pattern works, and both harness canaries flip red when it lands). Also, per #6193 (proposed validating webhook for tieredStore immutability), does the event path remain the detection fallback when webhook.enabled=false? - pkg/ddc/cache/engine/engine.go:40 [nit] — Package-level mutable tieredStoreWarningInterval var instead of a per-engine field or const
Consensus nit (Claude F4 / TS-VAR-SCOPE, Codex N1).tieredStoreWarningIntervalis a package-level mutable var while the sibling tunablesyncRetryDurationis resolved per-engine from env (getSyncRetryDuration, engine.go:91-101). It works (engines are per-runtime and same-key reconciles are serialized), but the var is not even used as the test seam it appears to be: the PR's own throttle specs backdateengine.timeOfLastTieredStoreWarning(sync_test.go:1431) without ever mutating the interval. Either use the seam in the throttle test (clearer) or drop it for a plainconst; a per-engine field would also work. Non-blocking.
Verified review delta
- TS-REMEDIATION-GUIDANCE-WEDGE: open (initial-pr)
- TS-MEDIUM-BLINDSPOT: open (initial-pr)
- TS-DOC-CLIENT-SCOPE: open (initial-pr)
- TS-CLIENT-FOLLOWUP: open (initial-pr)
- TS-E2E-GAP: open (initial-pr)
- TS-VAR-SCOPE: open (initial-pr)
Code: needs-work. CI: success; attribution: baseline. Required CI checks still apply.
| ```bash | ||
| kubectl get events -n <namespace> --field-selector type=Warning,reason=RuntimeTieredStoreConflict | ||
| ``` | ||
| - **How to apply changes**: Changes to `tieredStore` require deleting and recreating the worker workload (or the CacheRuntime) to take effect. |
There was a problem hiding this comment.
Consensus finding (Codex F1, confirmed by Claude's independent cross-critique audit). The new event message ("Recreate the worker to apply changes.", pkg/ddc/cache/engine/sync.go:310) and the new docs ("deleting and recreating the worker workload (or the CacheRuntime)") point users at a remedy that wedges the runtime. Verified: SetupWorkerComponent only runs from Setup and is gated on Status.Worker.Phase == RuntimePhaseNone, and both setup gates stay closed after worker deletion — the dataset's DatasetReady condition is never cleared (syncRuntimeSpec errors before status can update), and ConstructComponentStatusAndAffinity errors on NotFound instead of resetting the phase — so a deleted worker AdvancedStatefulSet is never recreated. syncRuntimeSpec then fails at GetPodSpec (NotFound) on every 20s reconcile ("Failed to sync the ddc" events), and because the error precedes checkWorkerTieredStoreDrift, the RuntimeTieredStoreConflict warning this PR adds goes silent exactly when the user needs it. Deleting individual worker pods also doesn't help (the ASTS controller recreates them from the unchanged template). Only recreating the CacheRuntime converges. The underlying non-recreation behavior is pre-existing; the guidance text pointing users into it is introduced by this PR. Fix: reword the event message and the ~6 introduced doc sites (cacheruntime_spec_update.md en:145/152/163 + zh, cache_runtime_tieredstore.md note 8 en+zh) to "delete and recreate the CacheRuntime", or teach the controller to recreate a missing workload.
| }) | ||
| }) | ||
|
|
||
| Context("tiered store drift detection and warning", func() { |
There was a problem hiding this comment.
Consensus finding (Claude F3 rated major; Codex M1 initially minor and withdrawn in cross-critique — "my earlier non-gating label was the deviation, and I withdraw it" — citing the governing policy that controller behavior needs e2e coverage and deferral is acceptable only with a tracked issue and a timeline; adjudicated major, see verdict). The PR's deliverable — a controller-emitted, throttled Warning event — is validated only at the fake-client unit layer: the 12 new specs drive syncRuntimeSpec directly (Claude's contract test adds the full engine.Sync path; Codex's envtest drives the engine against a real API server but not the real Reconcile loop). Two properties are exercised by nothing in the PR or the repo: (1) the 10-minute throttle surviving across reconciles, which rests entirely on CacheRuntimeReconciler.GetOrCreateEngine caching engines per runtime — if that caching ever changes, the warning degrades to an event on every sync, the exact event flood issue #6192 warned would crowd out other warnings; (2) live event emission and visibility on a real CacheRuntime. Both reviewers marked these properties "not verified (reasoning only)". The gap is cheap to close: a patch of tieredStore after Bound plus a kubectl get events --field-selector reason=RuntimeTieredStoreConflict assertion fits the existing pattern exactly (test/gha-e2e/curvine/test.sh:299 already patches a CacheRuntime; test/gha-e2e/mooncake/test.sh:268-269 already asserts an event that way). Fix: add the e2e case, or file a tracking issue with a timeline and reference it in the PR.
58afa3a to
58e765d
Compare
|
Thank you for the detailed review! All items from the review have been addressed in the latest commit:
|
…te limitations When spec.worker.tieredStore is updated on an existing CacheRuntime, the runtime config ConfigMap is recomputed and updated with the new quotas during sync, but the worker pod volumes (emptyDir.sizeLimit, memory tmpfs) cannot be updated in-place without pod recreation. This created an undetected split between the ConfigMap and the worker pod volumes, which could cause worker crashes or out-of-space issues upon subsequent restarts. This commit: 1. Adds checkWorkerTieredStoreDrift to syncRuntimeSpec in pkg/ddc/cache/engine/sync.go to compare worker pod volumes against runtime.Spec.Worker.TieredStore.Levels quotas (Quantity.Cmp) and emptyDir storage media. 2. Emits a RuntimeTieredStoreConflict Warning event on the CacheRuntime object when drift is detected, throttled by a 10-minute interval to prevent spamming while keeping the event visible within the 1-hour Kubernetes event TTL. Remediation clearly directs users to delete and recreate the CacheRuntime. 3. Automatically resets the throttling cooldown once the drift is resolved. 4. Reads the worker PodSpec whenever tieredStore or resources are present, ensuring drift detection runs even when neither the CacheRuntime nor the CacheRuntimeClass declares resources. 5. Adds e2e controller loop verification in test/gha-e2e/curvine/test.sh to assert RuntimeTieredStoreConflict warning emission upon patch. 6. Documents the in-place immutability of spec.worker.tieredStore and the drift warning behavior in both Chinese and English user guides. Signed-off-by: Siddhanth Sadashiv Raikar <raikarsiddhanth@gmail.com>
58e765d to
b56db67
Compare
|
| case level.EmptyDir != nil: | ||
| volName, msg := checkEmptyDirDrift(levelIndex, level, podSpec.Volumes) | ||
| expectedVolumes[volName] = true | ||
| if msg != "" { | ||
| driftMsgs = append(driftMsgs, msg) | ||
| } | ||
| case level.HostPath != nil: | ||
| volNames, msgs := checkHostPathDrift(levelIndex, level, podSpec.Volumes) | ||
| for _, vn := range volNames { | ||
| expectedVolumes[vn] = true | ||
| } | ||
| driftMsgs = append(driftMsgs, msgs...) |
| interval := e.getTieredStoreWarningInterval() | ||
| if e.timeOfLastTieredStoreWarning.IsZero() || time.Since(e.timeOfLastTieredStoreWarning) >= interval { |
| local deadline=120 | ||
| local counter=0 | ||
| local event_found="" | ||
| while true; do | ||
| event_found=$(kubectl get events --field-selector reason=RuntimeTieredStoreConflict -o jsonpath='{.items[0].message}' 2>/dev/null || true) |
…ontract for PR fluid-cloudnative#6219 round 2 TS3 flipped to red on head b56db67 (Medium comparison landed in checkEmptyDirDrift), so per polarity policy it is inverted into the contract test TestVerifyEmptyDirMediumFlipDetected (both disk->Memory and Memory->disk). TS2 stays a canary, now pinned to follow-up issue fluid-cloudnative#6221. TS1 additionally pins the reworded remediation text. TS4 is a new contract test for the throttle-across-Syncs property the controller relies on. Production code untouched.
…b56db67 TS3 canary flipped on head (Medium comparison landed) and was inverted into the contract test; TS4 throttle contract added and bite-checked; TS2 pinned to follow-up issue fluid-cloudnative#6221; TS1 pins the reworded remediation text. Round-2 re-verify output and bite-checks captured; .last-reviewed advanced to b56db67. Production code untouched.
…ead b56db67 - F1/F2/F3 re-polarized canary->contract: fixes landed as wording (event message + en/zh docs now 'delete and recreate the CacheRuntime'), medium comparison, and worker-only doc scoping - new additive contracts: TestVerifyPR6219RemediationGuidanceRecreateCacheRuntime, TestVerifyPR6219MediumDriftNowDetected, TestVerifyPR6219DocsGuidanceAndScope - retired canaries stay in tree as pinned pre-existing behavior records - results: head-b56db670-harness.txt (all contracts green, both fix canaries flipped), head-b56db670-pr-specs.txt (15/15 drift specs pass)
cheyang
left a comment
There was a problem hiding this comment.
Round 2 on b56db670 (the author force-pushed one amended commit addressing every round-1 finding): both blockers from the last review are fixed, and each fix is verified rather than assumed.
-
Remediation guidance — the event message and all six doc sites (en+zh) now say "Delete and recreate the CacheRuntime", the only remedy our round-1 verification showed actually converges. Verified three ways: both harnesses' contract tests on head, a grep showing no leftover worker-recreation wording anywhere, and the CI kind-e2e log where the live event carries the new text. The wording is now pinned by contracts on both sides, so a regression fails the harnesses.
-
Real e2e coverage —
verify_tieredstore_drift_warningpatches the live curvine-demo CacheRuntime and asserts the event with a 120 s deadline and panic-on-timeout; it runs inside the required kind-e2e checks and is green on all five k8s versions (v1.22.17–v1.33.2), with the v1.22.17 log confirming the event was actually found — not a vacuous pass.
Also fixed and verified: emptyDir medium-only flips are now detected (both round-1 canaries flipped red and were inverted into bite-checked contract tests), the docs are worker-scoped everywhere with client-side detection tracked in #6221 (the PR body also answers the webhook-off fallback question), and the package-level interval var became a per-engine field with a tested fallback. Required CI is green on b56db670 (only tide pending, which is approved/lgtm label gating, not a test).
What's left is non-blocking and folded below: the e2e's revert leg asserts nothing while the comment and the PR body claim it verifies the cooldown reset (and the revert patch drops the level's high/low), nothing on a live cluster yet distinguishes a throttled warning from per-reconcile spam, and the events query could take -n default. The only disagreement in the debate was a severity label (minor vs nit on the revert leg), settled at minor because the docs affirmatively claim a verification the code doesn't perform. Reviewer C (Copilot) flagged three advisory items: the requeue-dependent re-emission mechanism is real (it reinforces the throttle assertion below), the event-selector scoping is a hardening point that can't trigger in the current CI layout, and the drift-dispatch precedence mismatch is plausible by code read but harness-unverified — none of them gate.
Next round
[MUST] none — merge-ready from the review side, once maintainers accept the #6221 client-detection deferral.
[SHOULD] In verify_tieredstore_drift_warning, assert the revert leg (no further conflict events, or a fresh drift re-warns immediately) and revert the full original level including high/low; drop the "resets upon reversal" claim from the PR body in the meantime.
[SHOULD] While drift persists, assert the RuntimeTieredStoreConflict event's count stays 1 across a couple of reconcile cycles, pinning the throttle on a real controller loop.
[SHOULD] Add -n default (and optionally a UID + Warning-type filter) to the events query at test/gha-e2e/curvine/test.sh:394.
[Filed] none — the client-side follow-up is already tracked in #6221 (author-filed).
Non-blocking observations (5) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- test/gha-e2e/curvine/test.sh:406 [minor] — e2e revert leg asserts nothing while the comment and PR body claim it verifies the cooldown reset; the revert patch also drops the level's high/low
New this round, consensus (Claude F1 minor + Codex F7 TS-E2E-REVERT-HYGIENE nit, merged; severity adjudicated minor). The revert leg comments "Revert quota back to 1Gi to verify drift resolution and cooldown reset" but the code only patches back and sleeps 5 (test.sh:407-408) — no assertion that drift resolved, that no further event fires, or that the cooldown reset; the PR body's item 4 claims the e2e verifies the warning "resets upon reversal", an affirmatively stated verification the code never performs. Additionally the revert patch (kubectl patch --type=merge) JSON-merge-replaces the levels array, dropping the original level's high "0.8" / low "0.5" (cacheruntime.yaml:36-41), leaving the CR different from how the test found it. Harmless today — the drift check compares only Medium/SizeLimit/count, and delete_dataset_and_runtime runs immediately after (test.sh:448) with each CI run re-creating the CR — and the cooldown-reset property itself is unit-covered (PR spec + harness TS4). Fix: revert the full original level and/or assert no new RuntimeTieredStoreConflict events after the revert (or a fresh drift re-warns immediately), or drop the "resets" claim from the PR description. - test/gha-e2e/curvine/test.sh:394 [minor] — e2e proves event emission but not bounded re-emission (throttle) on a real controller loop
New this round, consensus (Claude F2; matches the residual Codex recorded non-blocking in its F4). The e2e breaks out of the wait loop on the first event and immediately reverts, so nothing on a live cluster distinguishes a correctly throttled warning from per-reconcile event spam — the exact flood issue #6192 warned would crowd out other warnings. The proposed assertion is mechanically sound: client-go's event correlator aggregates repeats into one event object with count++, so asserting .items[0].count stays 1 across a couple of ~20 s reconcile cycles pins the throttle end-to-end, and a broken engine cache (fresh engine per reconcile → zeroed timeOfLastTieredStoreWarning) would surface as count growth. Engine-level throttle is pinned by harness contract TS4 (3 consecutive Syncs → exactly 1 event, re-emission after interval, reset on resolution, bite-checked); what remains untested is the pre-existing GetOrCreateEngine cache handoff shared with timeOfLastSync. Copilot C4368713456 adds a real adjacent mechanism (see TS-REQUEUE-REEMISSION) that strengthens the case for landing this. - test/gha-e2e/curvine/test.sh:394 [nit] — events query without -n default in the new e2e; selector unscoped by involved object
New this round, consensus nit (Claude F3; overlaps Copilot C4368713479 from a different angle). 'kubectl get events --field-selector reason=...' is the only events query in the script without a namespace; the diagnostics a few lines up use -n default (test.sh:252). Functionally fine — every kubectl create in the script likewise relies on the context namespace — but the flag keeps the assertion correct if the runner's default context ever changes. Copilot additionally suggests filtering by the runtime's UID and Warning type; both cross-critiques confirmed the false-pass vector cannot trigger in this suite today (exactly one CacheRuntime, curvine-demo, per fresh kind cluster), so this is robustness hardening of the same line, not a live defect. - pkg/ddc/cache/engine/sync.go:284 [minor] — Drift-check dispatch order (EmptyDir before HostPath) differs from the ConfigMap extraction precedence (HostPath before EmptyDir) — flagged by Reviewer C, unconfirmed by the debaters
Copilot-only finding C4368713436, recorded per advisory policy — never a consensus, never gating. Claim: the drift switch dispatches ProcessMemory → EmptyDir → HostPath (sync.go:266-282) while extractTieredStoreLevels checks HostPath before EmptyDir (cm.go:225 vs :239), and the CRD has no one-of validation (api/v1alpha1/cacheruntime_types.go:275-299, all three mediums merely +optional), so a level edited to carry both emptyDir and hostPath would be reported as hostPath in the ConfigMap while the drift check validates the stale emptyDir volume — a spurious-warning failure mode, not silence. Both debaters independently code-read both precedence orders and call the mechanism plausible, but it is harness-unverified, requires a malformed-but-accepted CR, and the failure mode is a wrong warning rather than a missed one. Candidate for a follow-up hardening: align the dispatch with the ConfigMap precedence (or add one-of validation) with a regression test for the both-fields-set edit. - pkg/ddc/cache/engine/sync.go:301 [minor] — With FLUID_RUNTIME_RECONCILE_DURATION=-1 the throttled warning can expire under persistent drift — Reviewer C flag, mechanism confirmed by both debaters, consequence unverified
Copilot finding C4368713456, partially confirmed by the debaters: the interval check does not itself schedule another reconciliation, and after a successful sync the controller returns via GenerateRandomRequeueDurationFromEnv (runtime_controller.go:305) — with FLUID_RUNTIME_RECONCILE_DURATION=-1 that is NoRequeue, so once watched objects stop changing, no reconcile fires to re-emit the warning and the 1 h event TTL can expire despite persistent drift. Claude's cross-critique confirmed the mechanism (default is a 90 s requeue, crtl_utils.go:38) and Codex called it 'real mechanism, supports the F4 residual' — but the PR does not touch requeue behavior (the mechanism is pre-existing infra), the non-default env must be set explicitly, and no harness run demonstrates the expiry. Advisory only; if it matters, the remedy is to schedule a follow-up reconcile while drift persists or record a persistent status condition. Reinforces TS-E2E-THROTTLE-ASSERT as the worth-landing assertion.
Verified review delta
- TS-REMEDIATION-GUIDANCE-WEDGE: fixed (initial-pr)
- TS-MEDIUM-BLINDSPOT: fixed (initial-pr)
- TS-DOC-CLIENT-SCOPE: fixed (initial-pr)
- TS-E2E-REVERT-ASSERT: open (initial-pr)
- TS-E2E-THROTTLE-ASSERT: open (initial-pr)
- TS-E2E-NS-FLAG: open (initial-pr)
- TS-CLIENT-FOLLOWUP: fixed (initial-pr)
- TS-DISPATCH-PRECEDENCE: open (initial-pr)
- TS-REQUEUE-REEMISSION: open (initial-pr)
- TS-E2E-GAP: fixed (initial-pr)
- TS-VAR-SCOPE: fixed (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.




Ⅰ. Describe what this PR does
When
spec.worker.tieredStoreis updated on an existingCacheRuntime, the runtime configuration ConfigMap (fluid-runtime-config-<name>) is recomputed from spec on every sync (cm.go:102), but the worker pod volumes (emptyDir.sizeLimit, emptyDir storage medium, memory tmpfs/dev/shm) cannot be updated in-place bySyncComponentSpecwithout pod recreation.This caused an undetected split between the ConfigMap and the worker pod volumes:
This PR addresses the issue by:
tieredStoredrift detection (checkWorkerTieredStoreDrift) insyncRuntimeSpec(pkg/ddc/cache/engine/sync.go):runtime.Spec.Worker.TieredStore.Levelsagainst the actualsizeLimitof worker pod volumes (Quantity.Cmp).emptyDirstorage media (Medium) to detect diskprocessMemoryandemptyDirvolumes by their deterministic names (getMemoryTieredStoreVolumeName,getTieredStoreVolumeName).hostPathpaths match the pod's volume configurations.checkProcessMemoryDrift,checkEmptyDirDrift,checkHostPathDrift).RuntimeTieredStoreConflictWarning event on theCacheRuntimeobject when drift is detected.e.getTieredStoreWarningInterval()) to prevent event spam while ensuring the event remains visible within the default 1-hour Kubernetes event TTL.PodSpecis read during sync even when neitherCacheRuntimenorCacheRuntimeClassdeclares resources (closing the gap wherepodSpecwas only read insideif workerResources != nil).test/gha-e2e/curvine/test.sh(verify_tieredstore_drift_warning) asserting that a real controller detectstieredStoredrift on patch, emitsRuntimeTieredStoreConflict, and resets upon reversal.spec.worker.tieredStorecannot be updated in-place and details the drift warning behavior in both English and Chinese documentation (cacheruntime_spec_update.mdandcache_runtime_tieredstore.md).Ⅱ. Does this pull request fix one issue?
fixes #6192
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added unit test suite in
pkg/ddc/cache/engine/sync_test.go(Context("tiered store drift detection and warning")):should not emit a warning event when tiered store matches worker pod volumes: verifies 0 warning events when volumes match spec (both processMemory and emptyDir).should emit a Warning event when processMemory quota in spec drifts from worker volume sizeLimit: verifiesRuntimeTieredStoreConflictevent and error description forprocessMemoryquota drift.should emit a Warning event when emptyDir quota in spec drifts from worker volume sizeLimit: verifies Warning event foremptyDirquota drift.should emit a Warning event when emptyDir medium is edited from disk to Memory: verifies Warning event for emptyDir medium drift.should emit a Warning event when emptyDir medium is edited from Memory to disk: verifies Warning event for emptyDir medium drift.should emit a Warning event when hostPath paths are edited: verifies Warning event forhostPathpath changes.should emit a Warning event when a new level is added to spec that worker pod lacks: verifies Warning event for newly added levels.should emit a Warning event when a tieredStore volume exists on pod but was removed from spec: verifies Warning event for orphaned volumes.should throttle warning events and re-emit only after interval expires: verifies rate-limiting within 10 minutes and re-emission after expiration.should reset throttling cooldown when drift is resolved: verifies cooldown reset upon resolution and immediate re-triggering on subsequent drift.should detect drift even when neither CacheRuntime nor template declares worker resources: verifies drift detection runs whenworkerResources == nil.should return gracefully when podSpec is nil: verifies nil safety.should detect missing volume for processMemory level: verifies missing volume detection.should detect missing volume for hostPath level: verifies missing volume detection for host paths.should return error when reading worker pod spec fails: verifies error propagation and logging whenGetPodSpecfails.test/gha-e2e/curvine/test.sh(verify_tieredstore_drift_warning): patchescacheruntime curvine-demoemptyDir quota, asserts event emission, and reverts.100% statement coverage achieved across all new functions and branches.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews
Client-side tieredStore drift detection (
spec.client.tieredStore) is tracked in #6221 as a follow-up, as the client DaemonSet currently uses an OnDelete/immutable model. When the validating webhook (#6193) is disabled, controller event-based detection will provide the detection fallback.