Repository navigation
Conversation
|
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. |
|
/assign @yangyuliufeng |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6200 +/- ##
==========================================
+ Coverage 65.47% 65.52% +0.04%
==========================================
Files 486 486
Lines 34307 34313 +6
==========================================
+ Hits 22463 22482 +19
+ Misses 10085 10076 -9
+ Partials 1759 1755 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
49c54fc to
106b165
Compare
|
[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 |
There was a problem hiding this comment.
🟡 Changes recommended
Missing cache invalidation can leave application scheduling constraints stale after worker scheduling changes, including across controller restarts.
1 open finding
What changed in this PR
Reduces redundant worker workload reads during CacheRuntime status reconciliation, addressing #5879.
Changes:
- Caches worker node affinity on the engine and reuses persisted status.
- Adds tests for cache reuse, selector merging, and fetch failures.
| File | Description |
|---|---|
| pkg/ddc/cache/engine/status.go | Reuses cached affinity during status updates. |
| pkg/ddc/cache/engine/status_test.go | Tests affinity caching and API read counts. |
| pkg/ddc/cache/engine/engine.go | Adds the worker affinity cache field. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| if e.cacheAffinity != nil { | ||
| status.CacheAffinity = e.cacheAffinity.DeepCopy() | ||
| } else if status.CacheAffinity != nil { | ||
| e.cacheAffinity = status.CacheAffinity.DeepCopy() |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
The premise checks out mechanically — on base, every status-update cycle fetches the worker AdvancedStatefulSet twice (once in ConstructComponentStatus, once for node affinity), and this PR eliminates the second read via an engine-level cache plus a persisted-status fallback; the new happy-path tests are solid. One correction on framing: this controller's client is informer-cached, so the removed fetch never actually reached the API server (0 GETs/cycle measured against envtest on both base and head) — the saving is one informer lookup plus a deepcopy per cycle, not API-server load relief. The main concern is the tradeoff that saving buys: the cached affinity is never invalidated, so an out-of-band change to the worker StatefulSet's nodeSelector — the only way worker placement moves today, since fluid never updates the STS template in place — leaves status.CacheAffinity stale forever, even across controller restarts, and that field feeds the nodeaffinitywithcache webhook, which injects preferred and required node terms into app pods; on base the next cycle self-healed it. Both sides of this debate reproduced that regression independently, and agree on the cheapest fix: derive the affinity from the Get the status path already performs every cycle, which keeps the performance win with zero staleness. Smaller items: deep-copy the status write on the cache-miss branch, and add a test pinning the staleness semantic one way or the other. Copilot (advisory) flags the same issue with the same repro and remedy.
| if err != nil { | ||
| return false, err | ||
| // Worker Affinity: read from cache or runtime status to avoid calling GetNodeAffinity on every status update cycle | ||
| if e.cacheAffinity != nil { |
There was a problem hiding this comment.
Once e.cacheAffinity (or the persisted status.CacheAffinity) is set, the worker AdvancedStatefulSet's nodeSelector/node-affinity is never read again — not on later cycles, not after a controller restart (the persisted-status branch re-seeds the engine cache with no fetch), not ever; the only escape is deleting the CacheRuntime CR. The worker STS pod template can change while the runtime lives, and fluid's own control plane neither propagates nor notices it (reconcileStatefulSet is create-only; SyncComponentSpec patches only replicas/image/resources), so an out-of-band STS edit or delete+recreate — the standard way to move cache workers today — is exactly the change this cache freezes out. This is not a cosmetic status field: status.CacheAffinity is consumed by the nodeaffinitywithcache webhook (pkg/webhook/plugins/nodeaffinitywithcache/node_affinity_with_cache.go:159,194), which injects it as preferred AND required node-selector terms on app pods, and pkg/ddc/thin/referencedataset/sync.go:235 propagates it into ThinRuntime status. A stale value pins newly scheduled app pods to the old node set and can make them unschedulable on the required tier. On base, the every-cycle refetch self-healed this within one sync cycle; this PR silently removes that self-healing. Confirmed by two independent verification harnesses (reviewer branches verify/cache-affinity-claude and verify/cache-worker-affinity-cache-codex on the cheyang/fluid fork): after the cache warms and the worker STS's nodeSelector changes out-of-band, status.CacheAffinity stays at the old value on this head and refreshes on base — red on head, green on base, and green on head under a temporary always-fetch control patch; the staleness also survives a simulated controller restart. Suggested fix: cheapest and fully correct — derive the affinity from the STS object ConstructComponentStatus already fetches every cycle (zero extra reads, never stale); or invalidate on template/spec change or TTL; or, if out-of-band STS changes are declared unsupported, document create-time-only semantics as an explicit decision rather than a side effect of a perf PR. Rated major rather than blocker because the trigger requires an out-of-band STS template change, but it is a verified regression of self-healing in a scheduling-visible field.
| // always use getRuntimeInfo() method instead of use this directly. | ||
| runtimeInfo base.RuntimeInfoInterface | ||
|
|
||
| // cacheAffinity caches the worker node affinity to avoid calling kubeclient.GetStatefulSet on every status update cycle |
There was a problem hiding this comment.
The PR (and issue #5879) frames this as removing unnecessary API-server load, but the cache controller's client is informer-cached for everything except Secrets (cmd/cache/app/cache.go wires NewFluidControllerClient → NewCacheClientBypassSecrets; controller-runtime routes reads through the manager's informer cache, and the AdvancedStatefulSet informer is warm regardless). Measured against a real API server (envtest, counting transport): 0 API-server GETs of the worker ASTS per status cycle on both base and this PR with the cached client; 2 per cycle on base and 1 after the first cycle on this PR with a direct (uncached) client. The real saving is one informer lookup + ASTS deepcopy per status cycle — worth having, but CPU-level. This recalibrates the F1 tradeoff (a CPU micro-optimization bought with permanent staleness of a scheduling-relevant field), so the PR/commit message should describe the benefit accurately.
| if err != nil { | ||
| return false, err | ||
| // Worker Affinity: read from cache or runtime status to avoid calling GetNodeAffinity on every status update cycle | ||
| if e.cacheAffinity != nil { |
There was a problem hiding this comment.
Is an out-of-band edit or delete+recreate of the worker AdvancedStatefulSet (to change nodeSelector/affinity) a supported way to move cache workers? Fluid's reconciler never updates the STS pod template after creation (create-only reconcileStatefulSet; SyncComponentSpec touches only replicas/image/resources), so it is de facto the only way today — yet neither this PR nor issue #5879 states a position. The answer decides the F1 disposition: if supported, the cache needs invalidation (or the piggyback design); if declared unsupported, the staleness needs only explicit documentation (status.CacheAffinity is create-time-only, delete the CacheRuntime to refresh). Since the field feeds the nodeaffinitywithcache scheduling webhook, the call should be the maintainer's, made explicitly rather than implied by a perf PR.
| }) | ||
| }) | ||
|
|
||
| Describe("Worker node affinity caching", func() { |
There was a problem hiding this comment.
The four new specs cover the happy caching paths well (first-cycle fetch + Get counts, init from persisted status, NodeSelector merge across cycles, GetNodeAffinity error propagation). Missing: (a) a spec where the worker STS pod template changes after the cache is warm — the exact scenario where the F1 staleness lives, and the regression guard if F1 is fixed — asserting either that the value is intentionally kept (locking in the tradeoff) or that it refreshes (if invalidation is added); (b) a spec asserting CacheAffinity is still persisted after a conflict retry (the existing conflict test, which predates this PR, covers worker phase only; a supplementary harness spec shows the current behavior is correct, so this is a coverage gap, not a bug). Drop-in starting points already exist: TestVerifyStaleCacheAffinityAfterWorkloadChange on branch verify/cache-worker-affinity-cache-codex and the VA-F1/VA-F1b/VA-C1 specs on branch verify/cache-affinity-claude.
| return false, err | ||
| } | ||
| e.cacheAffinity = affinity | ||
| status.CacheAffinity = affinity |
There was a problem hiding this comment.
On the cache-miss branch, e.cacheAffinity = affinity; status.CacheAffinity = affinity leaves the engine cache and the status object handed to the API server as the same object, while the other two branches DeepCopy(). No misbehavior today (nothing mutates either reference in place after the write), but any future in-place mutation of the written status would silently corrupt the engine cache. Verified by two independent pointer-identity canaries. Fix: status.CacheAffinity = affinity.DeepCopy() for copy-discipline consistency across all three branches.
| status.CacheAffinity = e.cacheAffinity.DeepCopy() | ||
| } else if status.CacheAffinity != nil { | ||
| e.cacheAffinity = status.CacheAffinity.DeepCopy() | ||
| } else { |
There was a problem hiding this comment.
ConstructComponentStatus and GetNodeAffinity each Get the same worker AdvancedStatefulSet in the same status cycle (call-count evidence: base performs 2 worker-STS Gets per status cycle, one of which is the status-construction Get that already carries the object the affinity is derived from). Returning the merged node affinity out of that one read — or extending the ComponentManager interface with a combined status+affinity method, which already groups both operations — would meet issue #5879's goal with zero staleness risk and no engine-level cache to invalidate, letting this cache block be dropped entirely. This is also the cheapest correct fix for the major finding above, and makes it moot at the cost of a small interface change.
…o avoid duplicate read (fluid-cloudnative#5879) Signed-off-by: Siddhanth Sadashiv Raikar <raikarsiddhanth@gmail.com>
106b165 to
1d66cff
Compare
frenemy17
changed the title
|
|
Makes total sense about the informer cache and the staleness risk with out-of-band updates, good call. Refactored it to drop the engine-level cache entirely. Instead, we now just pull affinity directly from the workload object during status construction via Quick rundown of what’s in this push:
Let me know what you think.. |
Ⅰ. Describe what this PR does
Previously, each
CacheRuntimestatus update cycle read the workerAdvancedStatefulSettwice:manager.ConstructComponentStatus(...)to build worker replica status.manager.GetNodeAffinity(...)to compute worker node affinity.This PR extends
ComponentManagerwithConstructComponentStatusAndAffinity(...)so that worker node affinity is derived directly from the single workload read that status construction already performs.This addresses #5879 by cutting the lookup count in half (from 2 down to 1 Get per status cycle) with:
nodeSelectorare immediately reflected instatus.CacheAffinityon the very next reconcile cycle (self-healing, without stale affinity reaching thenodeaffinitywithcachewebhook).Ⅱ. Does this pull request fix one issue?
fixes #5879
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
pkg/ddc/cache/component/component_test.go:ConstructComponentStatusAndAffinityreturns both status and affinity forAdvancedStatefulSetManager.ConstructComponentStatusAndAffinityreturns both status and affinity forDaemonSetManager.pkg/ddc/cache/engine/status_test.go:nodeSelectorimmediately updatestatus.CacheAffinityon the subsequent cycle.Ⅳ. Describe how to verify it
go test -v ./pkg/ddc/cache/...Ⅴ. Special notes for reviews
None