Skip to content

fix(cache): emit warning event on tieredStore drift and document update limitations - #6219

Open
frenemy17 wants to merge 1 commit into
fluid-cloudnative:masterfrom
frenemy17:fix/cacheruntime-tieredstore-drift-warning
Open

frenemy17 wants to merge 1 commit into
fluid-cloudnative:masterfrom
frenemy17:fix/cacheruntime-tieredstore-drift-warning

Conversation

@frenemy17

@frenemy17 frenemy17 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Ⅰ. Describe what this PR does

When spec.worker.tieredStore is updated on an existing CacheRuntime, 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 by SyncComponentSpec without pod recreation.

This caused an undetected split between the ConfigMap and the worker pod volumes:

  • The ConfigMap carries the updated quotas and medium settings.
  • The worker pod volumes retain their initial creation-time sizes and media.
  • When worker pods restart for any reason (eviction, rolling update, node reboot), the cache process reads the updated ConfigMap quota and attempts to allocate more space than the container tmpfs or emptyDir volume permits, resulting in crashes or truncated writes with no warning or event emitted.

This PR addresses the issue by:

  1. Adding worker tieredStore drift detection (checkWorkerTieredStoreDrift) in syncRuntimeSpec (pkg/ddc/cache/engine/sync.go):
    • Compares level-by-level quotas in runtime.Spec.Worker.TieredStore.Levels against the actual sizeLimit of worker pod volumes (Quantity.Cmp).
    • Compares emptyDir storage media (Medium) to detect disk $\leftrightarrow$ Memory flips.
    • Checks both processMemory and emptyDir volumes by their deterministic names (getMemoryTieredStoreVolumeName, getTieredStoreVolumeName).
    • Verifies declared hostPath paths match the pod's volume configurations.
    • Detects removed or orphaned tieredStore volumes on the worker pod.
    • Modularized into low-complexity helpers (checkProcessMemoryDrift, checkEmptyDirDrift, checkHostPathDrift).
  2. Emits a RuntimeTieredStoreConflict Warning event on the CacheRuntime object when drift is detected.
    • Remediation guidance directs users: "Delete and recreate the CacheRuntime to apply changes."
    • Throttles the Warning event to a 10-minute interval (e.getTieredStoreWarningInterval()) to prevent event spam while ensuring the event remains visible within the default 1-hour Kubernetes event TTL.
    • Automatically resets the cooldown when the split is resolved.
  3. Ensures worker PodSpec is read during sync even when neither CacheRuntime nor CacheRuntimeClass declares resources (closing the gap where podSpec was only read inside if workerResources != nil).
  4. Adds end-to-end controller loop verification in test/gha-e2e/curvine/test.sh (verify_tieredstore_drift_warning) asserting that a real controller detects tieredStore drift on patch, emits RuntimeTieredStoreConflict, and resets upon reversal.
  5. Documents that spec.worker.tieredStore cannot be updated in-place and details the drift warning behavior in both English and Chinese documentation (cacheruntime_spec_update.md and cache_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: verifies RuntimeTieredStoreConflict event and error description for processMemory quota drift.
  • should emit a Warning event when emptyDir quota in spec drifts from worker volume sizeLimit: verifies Warning event for emptyDir quota 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 for hostPath path 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 when workerResources == 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 when GetPodSpec fails.
  • Integration test in test/gha-e2e/curvine/test.sh (verify_tieredstore_drift_warning): patches cacheruntime curvine-demo emptyDir quota, asserts event emission, and reverts.

100% statement coverage achieved across all new functions and branches.

Ⅳ. Describe how to verify it

  1. Run unit tests with inlining disabled for gomonkey:
    go test -gcflags="all=-N -l" -v ./pkg/ddc/cache/engine -run TestCacheEngine -ginkgo.focus="tiered store drift"
  2. Verify code coverage:
    go test -gcflags="all=-N -l" -coverprofile=coverage.out ./pkg/ddc/cache/engine -run TestCacheEngine -ginkgo.focus="syncRuntimeSpec"
    go tool cover -func=coverage.out | grep -E "checkWorkerTieredStoreDrift|checkEmptyDirDrift|checkProcessMemoryDrift|checkHostPathDrift|findVolume"

Ⅴ. 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.

@fluid-e2e-bot

fluid-e2e-bot Bot commented Oct 22, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cheyang for approval by writing /assign @cheyang in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fluid-e2e-bot

fluid-e2e-bot Bot commented Oct 22, 2026

Copy link
Copy Markdown

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.80%. Comparing base (86e3adf) to head (b56db67).
⚠️ Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 RuntimeTieredStoreConflict warnings 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

Comment thread pkg/ddc/cache/engine/sync.go Outdated

@cheyang cheyang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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".

  2. 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 of checkWorkerTieredStoreDrift compares only SizeLimit, never Medium. Flipping an emptyDir level's medium between disk ("") and "Memory" is not cosmetic: the creation path charges the quota to container memory only when Medium == Memory (handleEmptyDir), and the ConfigMap reports mediumType MEM 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 as spec.{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. But checkWorkerTieredStoreDrift only ever receives the worker pod spec: editing spec.client.tieredStore produces the same split (the ConfigMap's client levels are rebuilt from spec every sync while the client DaemonSet is never re-synced after creation — ShouldSetupClient only runs at Status.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 to spec.worker.tieredStore and 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). tieredStoreWarningInterval is a package-level mutable var while the sibling tunable syncRetryDuration is 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 backdate engine.timeOfLastTieredStoreWarning (sync_test.go:1431) without ever mutating the interval. Either use the seam in the throttle test (clearer) or drop it for a plain const; 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@frenemy17

Copy link
Copy Markdown
Contributor Author

Thank you for the detailed review! All items from the review have been addressed in the latest commit:

  1. [MUST] Reword remediation guidance to "delete and recreate the CacheRuntime":

    • Updated event message in pkg/ddc/cache/engine/sync.go to "Worker tieredStore cannot be updated in-place after creation: %s. Delete and recreate the CacheRuntime to apply changes."
    • Updated all documentation references across docs/en/samples/cacheruntime/cacheruntime_spec_update.md, docs/zh/samples/cacheruntime/cacheruntime_spec_update.md, docs/en/userguide/cache_runtime_tieredstore.md, and docs/zh/userguide/cache_runtime_tieredstore.md.
  2. [MUST] Controller loop verification in e2e:

    • Added verify_tieredstore_drift_warning() to test/gha-e2e/curvine/test.sh. The test patches cacheruntime curvine-demo emptyDir quota, waits for and asserts emission of the RuntimeTieredStoreConflict Warning event from the controller loop, and then reverts the quota.
  3. [SHOULD] Compare emptyDir Medium in drift check:

    • Added vol.EmptyDir.Medium != level.EmptyDir.Medium check in checkEmptyDirDrift to detect disk $\leftrightarrow$ Memory flips.
    • Added regression unit tests for both disk $\to$ Memory and Memory $\to$ disk transitions.
  4. [SHOULD] Scope documentation to spec.worker.tieredStore & follow-up tracking:

  5. [SHOULD] Replace package-level tieredStoreWarningInterval var:

    • Converted defaultTieredStoreWarningInterval = 10 * time.Minute to a const.
    • Replaced package-level var with a per-engine field tieredStoreWarningInterval time.Duration on CacheEngine, initialized in Build() with a getTieredStoreWarningInterval() accessor.
  6. SonarCloud Complexity & Codecov Patch Coverage:

    • Refactored checkWorkerTieredStoreDrift into low-complexity helpers (checkProcessMemoryDrift, checkEmptyDirDrift, checkHostPathDrift), reducing Cognitive Complexity from 42 to $\le 5$ (0 SonarCloud issues).
    • Added unit test covering GetPodSpec failure path, achieving 100.0% patch statement coverage.

…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>
@frenemy17
frenemy17 force-pushed the fix/cacheruntime-tieredstore-drift-warning branch from 58e765d to b56db67 Compare October 24, 2026 19:26
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved gaps affect drift detection, persistent warning visibility, and the reliability of the end-to-end assertion.

3 open issues
1 resolved since last review

Review effort: Balanced

Comment on lines +273 to +284
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...)
Comment on lines +300 to +301
interval := e.getTieredStoreWarningInterval()
if e.timeOfLastTieredStoreWarning.IsZero() || time.Since(e.timeOfLastTieredStoreWarning) >= interval {
Comment on lines +390 to +394
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)
cheyang pushed a commit to cheyang/fluid that referenced this pull request Oct 24, 2026
…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.
cheyang pushed a commit to cheyang/fluid that referenced this pull request Oct 24, 2026
…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.
cheyang pushed a commit to cheyang/fluid that referenced this pull request Oct 24, 2026
…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 cheyang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.

  2. Real e2e coverage — verify_tieredstore_drift_warning patches 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]CacheRuntime: editing tieredStore updates the ConfigMap but not the worker Pod, and nothing reports the split

3 participants