Repository navigation
Conversation
…aseline A CacheRuntime that names only some resource keys used to replace the container's whole ResourceRequirements, dropping every requirement it did not restate. Raising just limits.memory also cleared limits.cpu, requests.cpu, requests.memory and claims, leaving the container with no CPU request and no CPU limit at all, and letting Kubernetes default the memory request up to the new limit. Both the creation and the update path treated the CacheRuntime resources and the CacheRuntimeClass template as alternatives. They are not: the template carries the runtime's own requirements and the CacheRuntime expresses the deltas an owner wants for their instance. Overlay them key by key instead, through one helper both paths call, so the two cannot drift apart and leave the workload rolling on every reconcile. The corollary is that a key set by the template can now be overridden but no longer removed by omitting it from the CacheRuntime. Removing a requirement is a change to the template, which is where the runtime's requirements are described. This is the same semantic question raised in the review of Fixes fluid-cloudnative#6173 Signed-off-by: btxu-db <btxu-db@outlook.com>
…eSpec The unit specs pin mergeResourceRequirements and desiredComponentResources directly. These drive the whole sync against a fake client instead, so the reported scenario is covered end to end: a worker whose CacheRuntimeClass template declares a full set of requirements and whose CacheRuntime raises only limits.memory keeps its CPU request, CPU limit and memory request. Removing the merge fails it with the value the issue reports. A second spec pins the nil handling. When neither side declares requests the merged value must leave them nil rather than an empty map: Semantic.DeepEqual separates the two, so an empty map would never compare equal to the workload and the sync would patch on every reconcile. Dropping the empty-overlay guard in mergeResourceList fails it. Signed-off-by: btxu-db <btxu-db@outlook.com>
The spec update guide described resources as the CacheRuntime value or, when unset, the CacheRuntimeClass template value. They are a baseline and an overlay now, so record that, and restate the limitation it carries: a key the template declares can be overridden but no longer removed by omitting it. Signed-off-by: btxu-db <btxu-db@outlook.com>
…out returning mergeResourceList and mergeResourceClaims modified base in place and also returned it, so a caller could not tell from the signature whether base was left untouched, and had to use the returned value to pick up a list allocated from nil or a slice grown by append. Both now take a pointer and modify it in place without returning. The nil case is handled inside: mergeResourceList allocates *base only when the overlay declares something, keeping an untouched nil list nil, and mergeResourceClaims relies on append growing a nil *base. mergeResourceRequirements keeps its signature: it still deep-copies the template baseline and returns the merged value, so its callers are unchanged. Its comment is condensed to the overlay rule it implements. Signed-off-by: btxu-db <btxu-db@outlook.com>
The resources row pinned the replace-the-whole-struct behaviour as a known bug of fluid-cloudnative#6173. With the resources now overlaid on the CacheRuntimeClass template key by key, it expects what that row said it should become: requests {cpu 1, memory 2Gi} and limits {cpu 2, memory 8Gi} on both the create and the update path. Signed-off-by: btxu-db <btxu-db@outlook.com>
Drop the doubled blank line left where the rebase onto fluid-cloudnative#6188 joined the existing specs and the resources specs, which failed the gofmt check. Signed-off-by: btxu-db <btxu-db@outlook.com>
The comment on mergeResourceList, and the one on the "stop patching once converged" spec, said an empty list would never compare equal to a workload rendered with nil, so the sync would patch on every reconcile. That is not so: updateResources compares with equality.Semantic, which treats nil and empty maps as equal. What the nil actually protects is the fluid-cloudnative#6165 contract. When neither side declares resources, desiredComponentResources returns nil and the sync leaves the workload alone. Allocating empty lists there turns that into a non-nil empty requirement, and the sync clears the resources of a workload edited out of band. Dropping the guard fails the specs that pin this, not the convergence spec. Say so in both comments. Signed-off-by: btxu-db <btxu-db@outlook.com>
transformComponentPodTemplate and desiredComponentResources both resolve a component's resources through mergeResourceRequirements, but each path was only tested on its own. Nothing asserted that they agree, which is what keeps the first reconcile after creation from patching the workload it has just rendered. Replay a table of template and CacheRuntime resources through both paths. Each row checks that the two resolvers return the same value, compared with equality.Semantic as updateResources does, and that a workload rendered by the creation path is left untouched by syncRuntimeSpec. The rows cover the nil/empty boundary on both sides and claims. Reverting either path alone to the wholesale replacement fails the partial-overlay, added-key, empty-list and claims rows. Signed-off-by: btxu-db <btxu-db@outlook.com>
The curvine suite covered create, bind, IO and scaling, but none of the in-place resources update line (fluid-cloudnative#6165, fluid-cloudnative#6167, fluid-cloudnative#6173) ran against a real cluster. Give the worker template a full set of requirements, then patch only limits.memory on the CacheRuntime. The AdvancedStatefulSet must take the new value and keep the three keys the CacheRuntime did not name. Wait for the update to roll out, in place or by recreation depending on the cluster, and check that the worker pod carries the merged values before the scaling case runs. Signed-off-by: btxu-db <btxu-db@outlook.com>
Three consequences of the resources overlay that operators need to know: - Editing resources in a CacheRuntimeClass template now reaches every CacheRuntime using that class, on its next reconcile, for the keys it does not override. Removing all of the template's resources while the CacheRuntime sets none leaves the workload's values in place. - After upgrading, a CacheRuntime that set only part of resources gets the template's other requirements back on the first reconcile, which updates the affected pods once. - claims can only reference pod-level resourceClaims the template declares. Any other name now makes pod creation fail validation; earlier releases ignored a claims-only CacheRuntime. List claims among the supported sub-fields, as the resolution order already mentions them. Signed-off-by: btxu-db <btxu-db@outlook.com>
The specs for a CacheRuntime that names only part of the template's resources drove the worker alone. The master resolves its resources through the same desiredComponentResources, so add the same case for it: a full template, limits.memory raised on the CacheRuntime, and the other three keys expected to survive the sync. Generalise the worker-only workload helper to take the workload name so both components share it. Signed-off-by: btxu-db <btxu-db@outlook.com>
…e declares desiredComponentResources returned nil when neither side declared any resources, which told the sync to leave the workload untouched. Since the merge onto the template baseline already keeps the template's values, that nil only served to block removals: a value set on the CacheRuntime over an empty template could not be dropped by removing it, and removing the last key from the template did not reach the workloads either. Always return the merged result, so a key neither side declares is cleared. This implements the rules agreed in the fluid-cloudnative#6165 review: a template key can be overridden but not removed, and a value the template does not declare can be set and later removed. An empty list such as limits: {} names no key and reads the same as leaving it out. Signed-off-by: btxu-db <btxu-db@outlook.com>
Signed-off-by: btxu-db <btxu-db@outlook.com>
Merging the CacheRuntime onto the CacheRuntimeClass template per key can turn two valid inputs into an invalid pair, such as a template requests.cpu: 1 with a CacheRuntime limits.cpu: 500m. The ASTS pod template is not validated, so the controller patched it silently and the API server only rejected the Pods it created. Check the merged first container on both paths. On creation, transform returns an error naming the conflicting keys, so setup fails. On sync, the component's resources are left as they are and a RuntimeResourcesConflict Warning event is emitted on the CacheRuntime, while image and replicas still sync. Signed-off-by: btxu-db <btxu-db@outlook.com>
Signed-off-by: btxu-db <btxu-db@outlook.com>
… resources Kubernetes requires huge pages and extended resources to set a request equal to their limit, and a limit whenever a request is set. A merged pair such as a template pinning nvidia.com/gpu to 1 plus a CacheRuntime raising only the limit to 2 passed validateResourceRequirements and was only rejected once a Pod was created, while sync patched it onto the workload. Validate those resources too, using a local mirror of IsOvercommitAllowed since that helper package is not vendored. Signed-off-by: btxu-db <btxu-db@outlook.com>
CacheRuntime has not shipped in a release yet, so there is no earlier behavior to upgrade from. Remove the upgrade note and the remark about earlier releases ignoring claims, and keep the one fact the note added: a template edit updates the master and worker Pods, recreating them where the cluster cannot resize them in place. Signed-off-by: btxu-db <btxu-db@outlook.com>
…icting resources The sync path already names the conflicting keys in a RuntimeResourcesConflict Warning event, but the creation path only returned an error that surfaced under the generic setup failure reason. Emit the same reason when the master, worker or client is rejected on creation, and cover all three components, the client included. Signed-off-by: btxu-db <btxu-db@outlook.com>
getRuntimeClass already rejects such a class, so no production caller reaches the dereference; the guard keeps a direct caller from panicking and, like the transform path, syncs no component. Signed-off-by: btxu-db <btxu-db@outlook.com>
…ce table The creation path adds the tiered store memory quota to the merged resources and the sync path charges it back from the workload. Add a Memory-medium emptyDir row and a process memory row, and have the resolve check charge the quota the way syncRuntimeSpec does. Signed-off-by: btxu-db <btxu-db@outlook.com>
Assert the Warning type alongside the reason, and that each rejected creation emits the event once. Signed-off-by: btxu-db <btxu-db@outlook.com>
…able charges The rows compared the two paths against each other, so a quota that both paths dropped would still agree. Pin the quota and the resulting memory request and limit the creation path must render for each row. Signed-off-by: btxu-db <btxu-db@outlook.com>
The sync patches container resources only. A CacheRuntimeClass edit that adds a pod-level resourceClaims entry and the matching container claim would carry the container claim into existing workloads without the pod-level entry, and with DynamicResourceAllocation enabled the API server would then reject every replacement Pod, stalling the rollout with nothing on the CacheRuntime. Leave out of the synced resources any claim the workload's pod spec does not declare and its container does not already carry, emit a RuntimeResourcesConflict Warning event naming it, and still sync the rest. Skipping the whole resources instead would hold back a requests or limits edit on clusters without DRA, where the API server ignores claims anyway; keeping claims the container already carries avoids rolling a workload whose Pods would not change. Document the limitation in both languages. Signed-off-by: btxu-db <btxu-db@outlook.com>
Signed-off-by: btxu-db <btxu-db@outlook.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6242 +/- ##
==========================================
+ Coverage 65.75% 66.01% +0.25%
==========================================
Files 486 488 +2
Lines 34459 34727 +268
==========================================
+ Hits 22660 22925 +265
+ Misses 10044 10043 -1
- Partials 1755 1759 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Resource claims are keyed by name, so a reordered list from the class template no longer patches the AdvancedStatefulSet. Requests and limits are compared semantically as before. Also correct the SyncComponentSpec comment: the bundled AdvancedStatefulSet controller updates only the image in place and recreates the Pods for any other template change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: btxu-db <btxu-db@outlook.com>
The first time a CacheRuntime is set up, the CacheRuntimeClass it names is copied into an immutable ConfigMap fluid-runtime-class-snapshot-<runtime> controlled by the runtime. From then on the runtime reads its class only from that snapshot, so editing or deleting a CacheRuntimeClass affects only the CacheRuntimes created afterwards. - Setup and Sync read the bound class once and pass it to every step, so a snapshot created moments ago but not yet in the informer cache is never read back. On AlreadyExists the existing snapshot is awaited. - A snapshot controlled by an earlier runtime of the same name is deleted with a UID precondition, so a stale cache cannot remove the runtime's own snapshot. - An invalid snapshot is never overwritten (RuntimeClassSnapshotInvalid event); changing runtimeClassName keeps the snapshot (RuntimeClassChanged event). - Data operations only read: they never bind, delete or fall back to the live class, and retry until the runtime owns its snapshot. Fixes fluid-cloudnative#6213 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: btxu-db <btxu-db@outlook.com>
Add a CacheRuntimeClass Snapshot section to the en/zh spec update guide, state that an image change restarts the containers while a resources change recreates the Pods, and explain how to release a new version of a RuntimeClass in the integration guide. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: btxu-db <btxu-db@outlook.com>
Edit the class worker limits.cpu, which the CacheRuntime does not override, together with the worker replicas as a positive control. Once the new replicas reach the AdvancedStatefulSet, the worker resources must be unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: btxu-db <btxu-db@outlook.com>
btxu-db
force-pushed
the
cacheruntime-pin-baseline
branch
from
November 13, 2026 05:48
1ad2fa4 to
5ac1064
Compare
btxu-db
changed the title
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Snapshot storage can exceed Kubernetes’ 1 MiB ConfigMap limit and fail Setup, while the e2e script can falsely pass after a failed class patch without testing snapshot isolation.
2 open issues
What changed in this PR
This PR binds each CacheRuntime to an immutable snapshot of its CacheRuntimeClass at first Setup, keeping existing runtimes independent of later class edits or deletion. It builds on #6177’s per-key resource overrides.
Changes:
- Uses the bound snapshot for setup, reconciliation, configuration, and DataLoad, with ownership checks and cache-lag handling.
- Avoids workload updates caused only by reordered resource claims.
- Adds snapshot regression coverage and updates English and Chinese guidance on class binding and Pod updates.
| File | Description |
|---|---|
test/gha-e2e/curvine/test.sh |
Checks resource overrides and isolation from class edits. |
test/gha-e2e/curvine/cacheruntimeclass.yaml |
Supplies baseline worker resources for end-to-end checks. |
pkg/ddc/cache/engine/ufs.go |
Accepts the bound class for mount updates. |
pkg/ddc/cache/engine/ufs_test.go |
Adapts mount-update tests to the class parameter. |
pkg/ddc/cache/engine/transform_worker.go |
Validates merged worker resources. |
pkg/ddc/cache/engine/transform_worker_test.go |
Covers worker resource conflicts. |
pkg/ddc/cache/engine/transform_master.go |
Validates merged master resources. |
pkg/ddc/cache/engine/transform_master_test.go |
Covers master resource conflicts. |
pkg/ddc/cache/engine/transform_common.go |
Adds shared resource merging and validation. |
pkg/ddc/cache/engine/transform_common_test.go |
Tests resource resolution, validation, and claims. |
pkg/ddc/cache/engine/transform_client.go |
Validates merged client resources. |
pkg/ddc/cache/engine/transform_client_test.go |
Covers client resource conflicts. |
pkg/ddc/cache/engine/sync.go |
Reuses the snapshot throughout reconciliation. |
pkg/ddc/cache/engine/sync_test.go |
Extends resource-sync regression coverage. |
pkg/ddc/cache/engine/setup.go |
Binds the class before creating runtime resources. |
pkg/ddc/cache/engine/runtime_class_snapshot.go |
Implements snapshot creation, reading, and ownership handling. |
pkg/ddc/cache/engine/runtime_class_snapshot_test.go |
Tests binding, invalid snapshots, cache lag, and isolation. |
pkg/ddc/cache/engine/resources_equivalence_test.go |
Checks creation and sync resource consistency. |
pkg/ddc/cache/engine/dataload.go |
Reads snapshots without creating them or using live classes. |
pkg/ddc/cache/engine/composition_matrix_test.go |
Updates expectations for per-key resource overrides. |
pkg/ddc/cache/engine/cm.go |
Generates runtime configuration from the supplied bound class. |
pkg/ddc/cache/engine/cm_test.go |
Adapts configuration tests to explicit class input. |
pkg/ddc/cache/component/sync_component_spec_test.go |
Tests order-independent claim comparisons. |
pkg/ddc/cache/component/resources.go |
Adds semantic resource and unordered claim comparison. |
pkg/ddc/cache/component/advanced_statefulset_manager.go |
Uses the comparison helper and clarifies update behavior. |
pkg/common/label.go |
Defines the snapshot’s class-name annotation. |
pkg/common/constants.go |
Adds resource-conflict and snapshot warning reasons. |
pkg/common/cacheruntime.go |
Defines snapshot ConfigMap naming. |
docs/zh/samples/cacheruntime/cacheruntime_spec_update.md |
Documents binding and update behavior in Chinese. |
docs/zh/dev/generic_cache_runtime_integration.md |
Adds Chinese class-version release guidance. |
docs/en/samples/cacheruntime/cacheruntime_spec_update.md |
Documents binding and update behavior in English. |
docs/en/dev/generic_cache_runtime_integration.md |
Adds English class-version release guidance. |
Review effort: Balanced
| BlockOwnerDeletion: ptr.To(true), | ||
| }}, | ||
| }, | ||
| Data: map[string]string{runtimeClassSnapshotKey: string(data)}, |
Comment on lines
+451
to
+452
| kubectl patch cacheruntimeclass "$class_name" --type json \ | ||
| -p "[{\"op\":\"replace\",\"path\":\"${class_cpu_path}\",\"value\":\"4\"}]" |
cheyang
pushed a commit
to cheyang/fluid
that referenced
this pull request
Nov 13, 2026
cheyang
reviewed
Nov 13, 2026
cheyang
left a comment
cheyang
left a comment
Collaborator
There was a problem hiding this comment.
Both reviewers independently reproduced issue #6213 on the base — a CacheRuntimeClass edit resizes and rolls every CacheRuntime using it on the next 90s reconcile — and confirmed this head closes it, each with a re-runnable contract harness (verify/cacheruntime-class-snapshot-claude on base ff99142, verify/cacheruntime-class-snapshot-codex on 956d78a; both isolate the same four snapshot commits). The whole-class snapshot design is the right call: every class consumer now reads the bound immutable ConfigMap, the dataset controller only reads, stale-owner replacement is UID-preconditioned (via an interceptor client, since the stock fake client ignores UID preconditions), and the informer-lag, malformed-snapshot, and class-rename edges are all handled and tested. The PR's 15 snapshot specs, the full cache/common suites, and kind e2e across k8s v1.22–v1.33 are green.
No blocking findings. The two reviewers fully converged — every finding was confirmed in cross-critique, and the only corrections were factual refinements to how the 1 MiB ConfigMap limit bites (it is the apiserver's ConfigMap-specific validation vs the CR's ~1.5 MiB etcd bound, not JSON escaping — which actually makes it more reachable — and the error text does surface today, just under a generic reason).
One decision before merge: issue #6213's contract item 4 asks that a resource-merge conflict be reported "with a condition and a RuntimeResourcesConflict Warning event" — this PR, like #6177, ships only the event, and events expire in about an hour. Since the PR says "Fixes #6213", merging auto-closes the issue with that element unimplemented. Please either confirm events-only reporting is acceptable (and amend the issue), or commit to a ResourcesSynced condition follow-up before v1.1.0 — CacheRuntimeStatus.Conditions already exists, so adding it later is cheap.
Next round
[MUST] Maintainer decision on #6213 item 4 (persistent conflict condition vs events-only) — merge is gated on this sign-off, not on code changes
[SHOULD] Record the class resourceVersion/generation in the snapshot annotation, or do the one-time bind read through a non-cached client (F1)
[SHOULD] Pre-Create guard plus a dedicated event for classes whose snapshot exceeds the 1 MiB ConfigMap cap — one guard can also cover the 224-char name edge (F2, F5; agrees with Copilot's size flag)
[SHOULD] Emit RuntimeClassChanged once per spec change instead of every reconcile (F3)
[SHOULD] Check the class patch's exit status in test/gha-e2e/curvine/test.sh so the isolation check can't pass vacuously (F6, from Copilot)
[Filed] none
Reviewer C (Copilot) advisory: its 1 MiB snapshot-size flag is confirmed by the debate (with the mechanism corrected); its curvine e2e errexit flag was validated by both reviewers and is folded in above as a non-blocking item. Reviewer disagreement was limited to factual corrections within F2 — none changed a severity or verdict. Evidence branches: verify/cacheruntime-class-snapshot-claude and verify/cacheruntime-class-snapshot-codex on the reviewer fork.
Non-blocking observations (6) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/ddc/cache/engine/runtime_class_snapshot.go:184 [minor] — one-time bind reads the class through the informer cache; a stale view is pinned forever
F1: bindRuntimeClass resolves the class with e.getRuntimeClass -> e.Get, i.e. through the controller's informer cache, and the snapshot it writes is immutable (Immutable=true, never rewritten, never re-read while it exists). A class edit that lands inside the cache-propagation window when a runtime is first set up — realistically, the documented remediation itself (class edit +kubectl delete cmto force a rebind, back to back) or GitOps applying class+runtime together — silently pins the pre-edit class forever. The snapshot annotation records only the class name, so the captured version is invisible anywhere. Confirmed by both reviewers (A raised, B confirmed with the full caller chain: neither controller watches the class, so lag is never observed by a reconcile trigger; a first-ever cached Get does a fresh LIST, so the race needs a warm-but-stale informer — consistent with minor). Suggested: do the one-time bind read through a direct (non-cached) client, or record the class's resourceVersion/generation in the snapshot annotation so the mismatch is observable. - pkg/ddc/cache/engine/runtime_class_snapshot.go:217 [minor] — no size guard before writing the marshaled class into a ConfigMap (1 MiB cap)
F2: the whole CacheRuntimeClass is json.Marshal-ed into the singleruntimeclass.jsonConfigMap data key and Created with no size check. The apiserver's ConfigMap-specific validation rejects total data over 1 MiB, while CRD objects bypass it entirely (bounded only by etcd's ~1.5 MiB max-request-bytes) — so a legally-creatable class in the ~1–1.5 MiB band (long scripts in dataOperationSpecs/executionEntries/extraResources) has its snapshot Create rejected with 'Too long: may not be more than 1048576 bytes', wedging Setup in a 20s requeue loop until the class shrinks. Consensus finding (A raised, B confirmed and strengthened the reachability), agreeing in the core with Copilot C4569021046. Two corrections settled in cross-critique: (1) the mechanism is the stricter ConfigMap validation vs the CR's etcd bound — not JSON-escaping overhead (the snapshot strips ObjectMeta, incl. last-applied-configuration, so it is usually smaller than the stored class); (2) the error does surface today via a generic ErrorProcessRuntimeReason event and a Dataset condition carrying the apiserver's 'Too long' text — what is missing is a dedicated, actionable event and a pre-Create guard. Suggested: check len(data) before Create and emit a RuntimeClassSnapshotInvalid-style event naming the 1 MiB limit and the offending class; the same guard can also close F5's name-length edge. - pkg/ddc/cache/engine/sync.go:361 [minor] — #6213 contract item 4 asks for a persistent conflict condition; only the event is implemented
F4: Issue #6213's user-facing contract item 4 says a resource-merge conflict is reported 'with a condition and a RuntimeResourcesConflict Warning event' (the sketch suggests ResourcesSynced=False 'so the state is still visible after the events expire'). This PR, like #6177 before it, emits only the event (sync.go:361/374, transform_common.go:323); nothing writes a condition on the CacheRuntime — CacheRuntimeStatus.Conditions exists and the engine already calls utils.UpdateRuntimeCondition, so absence is a choice, not an API limitation. Events expire in about an hour, so a conflict that stopped the resources sync hours ago leaves no persistent trace — exactly the gap the condition was meant to close. The behavioral half of item 4 (nothing is synced on an invalid merge) is implemented and tested (sync_test.go:1384-1485). Consensus finding (B raised, A confirmed and independently re-verified the issue text and the grep). Because the PR says 'Fixes #6213', merging auto-closes the issue with that element unimplemented. Maintainer decision needed — this is the only failed DoD item (AC9): accept events-only (and amend the issue), or track the condition as a follow-up before v1.1.0, the first CacheRuntime release. The event-only pattern itself pre-exists in the #6177 stack, but the auto-close scope burden is this PR's claim. - pkg/ddc/cache/engine/runtime_class_snapshot.go:159 [nit] — RuntimeClassChanged warning is emitted on every reconcile
F3: while spec.runtimeClassName disagrees with the bound class, loadRuntimeClassSnapshot records a RuntimeClassChanged Warning on every reconcile — the runtime controller's 90s requeue cadence (defaultRuntimeReconcileDuration), reached before any rate limiting via getBoundRuntimeClass at sync.go:53, and additionally from the dataset controller's read-only path (getBoundRuntimeClassReadOnly, e.g. dataload.go:58), so a runtime with active data operations gets it from two controllers. The recorder aggregates identical events by count so it is benign noise. Consensus (A raised, B confirmed and broadened). Emitting once per spec change (e.g. gated on a status condition) would keep the event stream cleaner. (An earlier suggestion referenced a RuntimeResourcesConflict condition as a model — that condition does not exist either; see F4.) - pkg/common/cacheruntime.go:138 [nit] — snapshot ConfigMap prefix narrows the longest usable CacheRuntime name by 8 chars
F5: GetCacheRuntimeClassSnapshotConfigMapName prefixes 'fluid-runtime-class-snapshot-' (29 chars), tightening the longest usable CacheRuntime name to 224 chars where the pre-existing runtime-config CM prefix ('fluid-runtime-config-', 21 chars) allowed 232 — and the CRD imposes no name-length limit beyond the API server's generic 253, so a 225–232-char name was admissible and functional before. Such a runtime now fails to bind forever: bindRuntimeClass handles only IsAlreadyExists specially, so the invalid-name Create error returns up and Setup/Sync retry unbound. Extreme edge (B raised, A confirmed the arithmetic and the failure mode). A hash suffix or a documented name-length limit would close it; a pre-Create guard shared with F2's size check closes both. - test/gha-e2e/curvine/test.sh:452 [nit] — curvine e2e: class kubectl patch runs unchecked and the script has no errexit
F6 (flagged by Reviewer C / Copilot C4569021108; no debater raised it as a finding, but both independently validated it during cross-critique): the classkubectl patchat test/gha-e2e/curvine/test.sh:451-453 runs unchecked and the script has no errexit, so if the patch fails, the unchanged-resources assertion passes vacuously and the replica-bump positive control does not prove the class edit landed. Valid test-rigor point that does not change either reviewer's verdict (both hold independent unit-level differential proof of the class-edit contract, and the e2e's replica-wait guard ensures a sync actually ran). Check the patch's exit status and panic on failure.
Verified review delta
- F1: open (initial-pr)
- F2: open (initial-pr)
- F4: open (initial-pr)
- F3: open (initial-pr)
- F5: open (initial-pr)
- F6: open (initial-pr)
Code: pending. CI: success; attribution: unknown. Required CI checks still apply.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ⅰ. Describe what this PR does
Editing a CacheRuntimeClass can currently change existing CacheRuntimes on their next reconcile. This PR binds each runtime to an immutable snapshot at its first Setup, so later class edits or deletion leave existing runtimes unchanged.
The snapshot covers the whole class, including component templates, options and data operation definitions, to keep images, configuration and scripts on the same version. It is stored in
fluid-runtime-class-snapshot-<runtime>, a ConfigMap owned by the CacheRuntime.runtimeClassNamekeeps the existing binding and emits a warning. Recreate the CacheRuntime to use a different class version.Also avoids workload updates caused only by reordered resource claims, and updates the English and Chinese guides for snapshot behavior and Pod updates.
Ⅱ. Does this pull request fix one issue?
Fixes #6213.
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Unit tests cover snapshot creation and ownership, class edits and deletion, invalid snapshots, cache lag, safe cleanup, read-only data operations, and resource claim ordering.
The curvine e2e adds a check that class resource edits do not reach an existing worker, while a runtime replica change still takes effect.
Ⅳ. Describe how to verify it
go test -gcflags=all=-l ./pkg/ddc/cache/... ./pkg/common/...On a cluster, create a CacheRuntime, edit its class resources, and trigger a reconcile. The worker resources should stay unchanged. Updating the CacheRuntime's own resource overrides should still apply.
Ⅴ. Special notes for reviews
CacheRuntime has not been released; migration from earlier development builds is not included. Unlike the original #6213 sketch, this snapshots the whole class, following #6239.
The curvine e2e was syntax-checked locally, but was not run on kind.