docs: list the fields CacheRuntime can update in place, and stop enumerating the rest - #6183
Conversation
|
Hi @btxu-db. 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 #6183 +/- ##
=======================================
Coverage 65.24% 65.24%
=======================================
Files 486 486
Lines 34194 34194
=======================================
Hits 22309 22309
Misses 10135 10135
Partials 1750 1750 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0a27cfc to
2fc529e
Compare
btxu-db
changed the title
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Read this against the code and it holds up. syncRuntimeSpec passes &runtime.Spec.{Master,Worker}.Replicas into ComponentSpec on every reconcile (always non-nil, unlike resources), and SyncComponentSpec patches the AdvancedStatefulSet through updateReplicas when the value differs. So moving replicas into section 3 is correct and the old section-4 entry really was stale.
I ran the actual sync path against a fake client to confirm both Master and Worker, both scale directions, and that an unchanged count is a no-op rather than a periodic overwrite. I also checked the two things you called out for review: this path records no RuntimeCondition and emits no Event (unlike pkg/ctrl/replicas.go), and there is no drain or migration on scale-in, so both limitations in 3.3 read accurately. The deploy/cacheruntime-controller + grep "replicas changed" command matches the chart and the Info-level log line, and collapsing section 4 into a pointer at section 3 is safe because SyncComponentSpec touches exactly those three fields.
One non-blocking thought: 3.3 documents Master replicas as in-place-updatable and the code does sync it, but you mentioned only exercising Worker live. Fine to leave as-is since it is confirmed in the code path.
LGTM.
Superseded: found a doc inaccuracy in §3.3 (observability) after approving; switching to request changes with the correction.
|
[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 |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Switching my earlier approval to a change request — I dug into the observability claim in §3.3 after approving and one line is inaccurate. Everything else holds up: I ran the real sync path against a fake client and confirmed replicas is synced in place for both Master and Worker, in both scale directions, value-driven rather than a periodic overwrite, and the section-4 whitelist rewrite is sound. The fix below is minor, but it's a factual error in a doc whose whole job is to be accurate, and the project already documents the status-based observability path elsewhere, so worth correcting before merge. Details inline.
|
|
||
| **Limitations**: | ||
| - ⚠️ **Scaling in discards cached data**: the Worker Pods removed are deleted outright, and the data they had cached is not migrated to the remaining Workers — it has to be loaded from the underlying storage again. Scaling out leaves the existing cache untouched. | ||
| - ⚠️ Scaling writes no RuntimeCondition and emits no Kubernetes Event; it can only be observed from the controller logs: |
There was a problem hiding this comment.
The "no RuntimeCondition / no Event" half is right, but "can only be observed from the controller logs" isn't. The reconcile writes replica counts to the CacheRuntime status on every loop: ConstructComponentStatus derives desiredReplicas from the ASTS spec.replicas, setWorkerComponentStatus/setMasterComponentStatus assign it to status.{worker,master}, and CheckAndUpdateRuntimeStatus persists it. Your own curvine_cache_runtime.md (line 573) already reads scaling this way: kubectl get cacheruntime ... -o jsonpath='{.status.worker.readyReplicas}/{.status.worker.desiredReplicas}'. Suggest: "Scaling writes no RuntimeCondition and emits no Kubernetes Event. Progress can be observed through status.{master,worker}.{readyReplicas,desiredReplicas} or the controller logs:"
|
|
||
| **限制**： | ||
| - ⚠️ **缩容会丢失缓存数据**：被缩掉的 Worker Pod 直接删除，其上已缓存的数据不会迁移到其余 Worker，需要重新从底层存储加载。扩容不影响已有缓存。 | ||
| - ⚠️ 扩缩容不会写入 RuntimeCondition，也不会产生 Kubernetes Event，只能从 controller 日志观察： |
There was a problem hiding this comment.
Same fix as the English side: drop "只能从 controller 日志观察" and mention the status fields, e.g. "扩缩容不会写入 RuntimeCondition，也不会产生 Kubernetes Event。可以通过 status.{master,worker}.{readyReplicas,desiredReplicas} 或 controller 日志观察进度："
…erating the rest
The spec update doc listed `replicas` under "Unsupported Update Fields" and told
readers a redeploy was required. It is in fact synced on every reconcile:
syncRuntimeSpec passes it into ComponentSpec for Master and Worker, and
SyncComponentSpec applies it through updateReplicas before image and resources.
Give `replicas` its own subsection in section 3, alongside runtimeVersion and
resources, and record the two things worth knowing before scaling: cache on a
removed Worker is discarded rather than migrated, and scaling writes no
RuntimeCondition and emits no Event, though progress is visible in
status.{master,worker}.{readyReplicas,desiredReplicas} and the controller log.
Section 4 used to enumerate the unsupported fields. Keeping that list correct
meant remembering, every time a field became syncable, to delete a line from a
section unrelated to the change, with nothing in the code or CI to prompt for
it — which is how the `replicas` entry survived. It was also labelled
"including but not limited to", so it could not be authoritative anyway.
Section 4 now refers to section 3 instead of repeating it, leaving one place
where a supported field is recorded.
Also drop the TODO above the syncRuntimeSpec call asking for the replica
scaling that the call already does.
Signed-off-by: btxu-db <btxu-db@outlook.com>
2fc529e to
6bcf188
Compare
|
2 participants
What this PR does
Fixes #6182.
docs/{zh,en}/samples/cacheruntime/cacheruntime_spec_update.mdstate that onlyruntimeVersionandresourcescan be updated in place, and listreplicasamong the fields that need a redeploy.replicasis actually synced on every reconcile:pkg/ddc/cache/engine/sync.go:199,236pass&runtime.Spec.{Master,Worker}.ReplicasintoComponentSpec— always non-nil, unlikeresourcesSyncComponentSpecapplies it first, throughupdateReplicas, and patches the AdvancedStatefulSet when the value differssyncRuntimeSpecis called from the reconcile loop atsync.go:76Verified on a kind cluster. Patching
spec.worker.replicasfrom 1 to 2, with no redeploy:Both lines share one
reconcileID, so the AdvancedStatefulSet change comes from this path and not from a coincidental re-apply. The new replica appeared within ~15s; patching back to 1 produced the mirror-image log and removed the pod. Reconciles that ran while the spec was unchanged loggedno spec changes detected, skip update, so the sync is value-driven rather than a periodic overwrite.Changes
replicasits own subsection 3.3, matching the layout of 3.1 and 3.2runtimeVersionandresourcesexplicitly in key takeaway 2, which said "these two fields" and became ambiguous once a third was added// TODO: implement other logic like inplace update and replica scalingsitting directly above thesyncRuntimeSpeccall that implements itWhy section 4 changed
Section 4 listed the unsupported fields one by one. Keeping that list correct meant remembering, every time a field became syncable, to go and delete a line from a section unrelated to the change — nothing in the code or CI prompts for it. The stale
replicasentry is what that looks like in practice, and the list was already labelled "including but not limited to", so it could never be authoritative anyway.It now points at section 3 rather than restating it: section 3 is the only place a supported field is recorded, and adding one no longer requires an edit elsewhere to stay accurate.
Notes for reviewers
Section 3.3 records two limitations that are not in the code as comments, and I would appreciate a check on both:
grep -rn "ScaleDown|destroyWorker|drain" pkg/ddc/cache/finds nothing — so a removed Worker's cached data has to be reloaded from the underlying storage. This is inferred from the absence of handling rather than measured; I only exercised scale-out and the rollback.pkg/ctrl/replicas.go:SyncReplicas, which records aRuntimeWorkerScaledIn/ScaledOutcondition and emits an Event, this path does neither. Documenting it makes current behaviour discoverable, but if the intent is to add conditions and events later, that line should come out again.The
replicasrow in the summary table is the only placeresourcesandreplicassit side by side, so it is worth confirming the wording does not implyreplicascarries the K8s >= 1.27 requirement — it does not.