Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @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. |
766461f to
c6277f4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6177 +/- ##
==========================================
+ Coverage 65.45% 65.72% +0.26%
==========================================
Files 486 486
Lines 34307 34480 +173
==========================================
+ Hits 22455 22661 +206
+ Misses 10091 10064 -27
+ Partials 1761 1755 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // returns it. Callers own base: mergeResourceRequirements hands over a deep copy. A nil | ||
| // base is kept nil when the overlay declares nothing, so that an untouched component | ||
| // still compares equal to the workload it was rendered into. | ||
| func mergeResourceList(base, overlay corev1.ResourceList) corev1.ResourceList { |
There was a problem hiding this comment.
modifying it in place or returns it. modified in place and return it is not a design.
|
|
||
| // mergeResourceClaims applies the overlay claims onto base by name, modifying it in | ||
| // place, and returns it. | ||
| func mergeResourceClaims(base, overlay []corev1.ResourceClaim) []corev1.ResourceClaim { |
…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>
c6277f4 to
0601908
Compare
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>
There was a problem hiding this comment.
🟡 Changes recommended
Empty resource maps can no longer clear runtime-only values, and removing the final template requirement leaves stale workload resources.
2 open findings
What changed in this PR
Fixes partial CacheRuntime resource overrides by merging them with the CacheRuntimeClass baseline.
Changes:
- Adds key-level merging for requests, limits, and claims.
- Aligns creation and reconciliation paths with regression coverage.
- Documents the new overlay semantics.
| File | Description |
|---|---|
pkg/ddc/cache/engine/transform_common.go |
Adds resource merging helpers. |
pkg/ddc/cache/engine/transform_common_test.go |
Tests merge and resolution behavior. |
pkg/ddc/cache/engine/sync.go |
Uses merged resources during reconciliation. |
pkg/ddc/cache/engine/sync_test.go |
Tests partial overrides and convergence. |
pkg/ddc/cache/engine/composition_matrix_test.go |
Updates cross-path expectations. |
docs/en/samples/cacheruntime/cacheruntime_spec_update.md |
Documents English merge semantics. |
docs/zh/samples/cacheruntime/cacheruntime_spec_update.md |
Documents Chinese merge semantics. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| func mergeResourceList(base *corev1.ResourceList, overlay corev1.ResourceList) { | ||
| if len(overlay) == 0 { | ||
| return | ||
| } | ||
| if *base == nil { | ||
| *base = corev1.ResourceList{} | ||
| } | ||
| for name, quantity := range overlay { | ||
| (*base)[name] = quantity.DeepCopy() | ||
| } | ||
| } |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Two independent reviews with separate verification harnesses both reproduced #6173 on the merge base (a partially specified CacheRuntime resources wholesale-replaces the template's requirements, dropping requests.cpu, requests.memory, limits.cpu and claims at both the creation render and the sync resolver) and both confirm the fix at head, including convergence — the two reviewers found no blocking issues and agree on every point. What's left before merge: (1) an explicit maintainer ack of the settled semantics — template keys can now be overridden but not removed by omission, with one unstated edge where removing the final key from the template leaves the old value on the workload — which the author themselves asked to have confirmed; (2) a docs sentence for the one-time repair roll after upgrading and the newly template-live propagation of CacheRuntimeClass edits; (3) an e2e spec-update case in the curvine suite, plus a small creation/sync equivalence test to pin the shared-helper invariant. Copilot (advisory) flags the same nil/empty-boundary family and its removal-edge flag is confirmed by both reviewers.
| @@ -248,28 +248,26 @@ func (e *CacheEngine) syncRuntimeSpec(ctx cruntime.ReconcileRequestContext, runt | |||
| } | |||
|
|
|||
| // desiredComponentResources resolves the resources that should be synced to a | |||
There was a problem hiding this comment.
Consensus (both reviewers, confirmed in cross-critique): two operator-visible consequences of the new baseline model are absent from the shipped docs. (1) A workload created by an earlier release from a partially specified CacheRuntime is missing the template's other requirements; the first reconcile after upgrade restores them and rolls/resizes those pods once — the PR body says "the roll is the repair", but the docs give no warning. (2) A CacheRuntimeClass resource edit now re-propagates on every reconcile to runtimes that partially override resources (previously a runtime declaring anything never saw template changes; resources are now uniquely template-live, more so than image, which sync reads from RuntimeVersion). One sentence in cacheruntime_spec_update.md or a release note would cover both.
| func mergeResourceRequirements(base, overlay corev1.ResourceRequirements) corev1.ResourceRequirements { | ||
| merged := *base.DeepCopy() | ||
| mergeResourceList(&merged.Limits, overlay.Limits) | ||
| mergeResourceList(&merged.Requests, overlay.Requests) |
There was a problem hiding this comment.
Consensus (Codex F1, confirmed and sharpened by Claude): the base guards keyed on Limits/Requests being non-nil, so a CacheRuntime declaring only claims was silently dropped; mergeResourceRequirements now merges claims unconditionally. Since the cache engine never renders pod-level resourceClaims, every runtime-declared claim is unmatched, and upstream pod validation (validateResourceClaimNames) rejects it — a claims-only CacheRuntime now surfaces as a loud, recurring validation failure at pod creation or workload patch time. Arguably better than the old silent drop and within the issue's scope (claims were dropped too), but this failure mode is not called out in the docs or PR body.
| return runtimeResources.DeepCopy() | ||
| var templateResources corev1.ResourceRequirements | ||
| if componentDefinition != nil && len(componentDefinition.Template.Spec.Containers) > 0 { | ||
| templateResources = componentDefinition.Template.Spec.Containers[0].Resources |
There was a problem hiding this comment.
Consensus (Codex F2, confirmed by Claude with the live consequence dissolved): the guard tests Requests/Limits/Claims == nil rather than len() == 0, so a template storing an explicitly empty map resolves to a non-nil desired containing an empty map. This is a pre-existing pattern, not introduced here, and it converges anyway: updateResources compares via equality.Semantic.DeepEqual, which equates nil and empty maps, so an empty-map desired never produces a patch. A len()-based check would have been more robust; noting it for a future cleanup only.
| 2. the `resources` set on the CacheRuntime are overlaid on that baseline key by key — | ||
| `requests` and `limits` merge by resource name, `claims` by claim name; | ||
| 3. when neither declares anything, nothing is synced and the workload keeps its current resources. | ||
|
|
There was a problem hiding this comment.
Consensus (Claude F1 = Codex F3, confirmed both ways; Copilot agrees and sharpens): once merged, a key the CacheRuntimeClass template declares can be overridden but no longer removed by omitting it from the CacheRuntime — dropping a requirement means editing the template. The template-as-baseline reading looks right (the null-sentinel alternative is an API change, correctly out of scope), the docs state it in en and zh, and the PR body itself asks for confirmation — but this field-level semantic change deserves an explicit maintainer ack before it ships. Note the documented removal recipe has one unstated boundary (verified by both reviewers, flagged by Copilot): removing the final key from the template resolves to an all-nil desired, desiredComponentResources returns nil, and SyncComponentSpec skips resources entirely (advanced_statefulset_manager.go newSpec.Resources != nil guard), so the deleted value stays on the workload; removing a non-final key works. The ack should be made aware of this edge.
| // Issue #6173: a CacheRuntime that names one key used to replace the container's | ||
| // whole ResourceRequirements, so raising the memory limit alone left the container | ||
| // with no CPU request and no CPU limit at all. | ||
| Context("when the CacheRuntime only names part of the template's resources", func() { |
There was a problem hiding this comment.
Consensus (Claude F3 = Codex F4, reached independently): the PR's coverage is unit + fake-client integration (sync_test.go drives the whole syncRuntimeSpec including a convergence spec — good, and it matches the convention #6165 set for this same code path), but the gha-e2e curvine suite covers create/bind/IO only. The whole in-place-update feature line (#6165, #6167, this PR) is untested against a real cluster, and this PR is the cheapest place to add the case (patch a CacheRuntime's resources, assert the workload keeps the template's other keys) since it defines the observable contract the docs now advertise.
| return existing | ||
| } | ||
|
|
||
| // mergeResourceRequirements overlays the CacheRuntime component resources onto the |
There was a problem hiding this comment.
Consensus (Claude F4, confirmed by Codex): the PR's central design argument — both paths call one helper, so they cannot drift apart — is not asserted by any test; each path is tested separately. A small table test comparing transformComponentPodTemplate's rendered resources against desiredComponentResources for the same inputs would pin the invariant (a working candidate, TestP1CreationAndSyncAgree, exists on branch verify/cache-resources-merge-claude: passes at head, fails at base on the claims case). The new sync specs cover worker only; master shares desiredComponentResources, so one master spec would make coverage symmetric.
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>
| mergeResourceList(&merged.Limits, overlay.Limits) | ||
| mergeResourceList(&merged.Requests, overlay.Requests) |
… (delta c1c303e..e259d73) Re-verified B1-B5 on head e259d73 (all FIXED) and the full engine suite (292 vs 275 passed, identical pre-existing failure set). New probes: D1 nil-removal semantics (contract), D2 requests-above-limits synthesis (canary, open finding), D3 quota read-back boundary (contract). .last-reviewed advanced to e259d73. Production code untouched. Co-Authored-By: Claude Code <noreply@anthropic.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>
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 4 (497bbea -> 3adc547): the one ask from the last review landed, and it landed right. The single delta commit teaches validateResourceRequirements the API server's non-overcommittable rules — for hugepages-* and extended resources such as nvidia.com/gpu, a request must equal its limit, and a request needs a limit at all — via a local isOvercommitAllowed mirror (the upstream helper's package is genuinely not vendored, so the copy is justified). Both harnesses prove the flip independently: new contract tests are red on the previous head and green here — the sync refuses a merged gpu 1/2 pair, leaves the workload untouched, still moves replicas, emits a RuntimeResourcesConflict Warning event naming the key, and a second sync patches nothing — and the mirror matches k8s v1.29.15 line for line, including the upstream quirk that kubernetes.io/-prefixed qualified names stay overcommittable. The one apiserver rule deliberately left out (hugepages needing cpu or memory) is provably safe to leave out: the merge preserves the union of keys, so the cpu/memory accompanying hugepages in either input survives. The PR's own specs cover the validator table, the master creation path and the sync event, and the docs are updated in both languages. Both round-3 harnesses re-ran fully green on the new head, the engine suite shows no regression (312 passed vs 298, the 12 failures byte-identical and pre-existing), a test-merge with current master is clean, and every CI lane is green at the exact head — DCO included.
One new item, filed as minor and non-blocking (F11 — and not a Copilot-only flag: all three reviews converged on it, each side reproducing it independently): a CacheRuntimeClass edit that adds a pod-level resourceClaims entry and its matching container claim propagates the container claim into existing workloads through the merged desired resources, while nothing ever writes the workload's pod-level spec.resourceClaims — replacement Pods are then rejected by the API server and the rollout stalls with no event on the CacheRuntime. The same wedge exists on the merge base for the natural template shape, so the mechanism is pre-existing and out of this PR's strict scope; the PR only widens exposure for partial-override runtimes and claim-only templates. If it is worth closing, the cheap remedy is the guard pattern this PR already uses twice: check the merged claims against the workload's pod-level declarations, skip and emit a Warning event — or document the class-edit limitation.
There was no disagreement between the two reviewers this round; the one dispute on record is with Reviewer C's other carried thread (an explicit limits: {} "failing to clear" the workload) — it does not reproduce and is refuted by a contract passing on base, previous head and head, while its merged-validation thread is exactly what rounds 3-4 implemented.
With that, everything this pipeline asked for across four rounds on the code itself is done. Still outstanding before merge, unchanged: the maintainer ack of the broadened clear semantics (F1) — no human maintainer comment on the PR or #6165 since round 2; the cheyang review entries on the PR are this pipeline's own published summaries, not a personal decision.
Next round
[MUST] none — no blocker or major remains open.
[SHOULD] Maintainer ack of the broadened clear semantics (F1) before merge — the one non-nit item left, and it is the merge owner's call, not a code change.
[SHOULD] F11 at author's/maintainer's discretion: guard claims in validDesiredResources against the workload's pod-level declarations (skip + Warning event, the F6/F9 pattern), or document the class-edit limitation in both languages; invert E1 / flip the canary when taken.
[SHOULD] Nits, author's discretion: emit the RuntimeResourcesConflict reason on the creation-path rejection and add a client creation-rejection spec (F10, half-closed this round by the new master spec); a nil-Topology guard in syncRuntimeSpec (F7, pre-existing); a tiered-store row in the equivalence table (F8).
[Filed] none.
Non-blocking observations (2) — 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:267 — A CacheRuntimeClass edit adding a pod-level resourceClaims entry half-propagates: the container claim reach... [minor] — pre-existing, not introduced by this PR
- pkg/ddc/cache/engine/transform_worker.go:47 — Creation-path conflict rejects with a generic event reason; client still lacks a creation-rejection spec (m... [nit]
Verified review delta
- F9: fixed (initial-pr)
- F11: open (preexisting)
- F6: fixed (initial-pr)
- F10: open (initial-pr)
- F12: refuted (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
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>
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 5 (3adc547 -> ce4f0c4): the last three carried nits landed, and each is closed by proof rather than by reading the diff. The creation path now emits the specific RuntimeResourcesConflict Warning when it rejects a merged spec, for all three components — worker, master and client, the client gaining its first creation-rejection spec (F10): both harnesses flip red -> green, rejection-without-event on the previous head and on the merge base, error + exactly one event naming the conflict here. syncRuntimeSpec now guards a topology-less class instead of dereferencing it (F7): a recovered nil-pointer panic on base and the previous head, a clean no-op here with workloads untouched. And the creation/sync equivalence table renders tiered-store workloads with the charged quota pinned per row (F8), so a row can no longer pass with the quota silently dropped — honestly a coverage fix: the quota machinery predates this PR on the merge base, and the harness pin passes on both heads by design.
The docs drop the upgrade notes, and the premise holds under two independent verifications: the CacheRuntime API is in no release tag, absent from the v1.0.8 tree, and no release branch newer than release-1.0 exists — nothing released ships the feature, so there is no upgrade path to warn about. One trivial residual, new this round: the PR body still carries that upgrade note (F13, nit, at your discretion).
Everything earlier stays green: both majors (F6 request>limit refusal, F9 non-overcommittable must-equal) re-verified fixed at this head with the validator untouched by the delta; the full re-verify sweep is green except the one expected red; the engine suite shows no revision regression (321 passed vs 312, the 12 failures byte-identical and pre-existing); and every CI lane is green at the exact head — DCO included, e2e on all five Kubernetes versions.
There was no disagreement between the reviewers this round — every flip was independently reproduced on both harnesses; the only divergence was bookkeeping (the stale PR-body paragraph, folded into F2 by one reviewer and split out as F13 by the other; the split is adopted). Reviewer C (Copilot) reviewed this head and raised no inline findings; of its historical threads, one is refuted (the explicit limits: {} claim), one is exactly what rounds 3-4 implemented (merged-request validation), and one is the still-open F11.
F11 itself is unchanged: the pre-existing class-claims half-propagation wedge (canary still passing at head), still pre-existing on the merge base for its dominant shape, still minor and non-blocking — guard or document at your discretion.
Still outstanding before merge, unchanged after five rounds: the maintainer ack of the broadened clear semantics (F1) — the latest cheyang review entry is this pipeline's own round-4 summary, not a personal decision, and #6165's proposal still has no human reply. That is the merge owner's call; there is nothing further for a code round to ask of the author.
Next round
[MUST] none — no blocker or major remains open, and no new finding beyond a nit this round.
[SHOULD] Maintainer ack of the broadened clear semantics (F1) before merge — the one non-nit item left, and it is the merge owner's call, not a code change.
[SHOULD] F11 at author's/maintainer's discretion: guard claims in validDesiredResources against the workload's pod-level declarations (skip + Warning event, the F6/F9 pattern), or document the class-edit limitation in both languages; the E1/canary contract flips when taken.
[SHOULD] Cosmetic: drop the stale upgrade-note paragraph from the PR description (F13).
[Filed] none.
Non-blocking observations (1) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- docs/en/samples/cacheruntime/cacheruntime_spec_update.md:1 — nit: the PR body still carries the upgrade-note paragraph the docs deliberately dropped this round [nit]
Verified review delta
- F7: fixed (preexisting)
- F8: fixed (initial-pr)
- F10: fixed (initial-pr)
- F13: open (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
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>
|
|
Maintainer decision on the open semantics question (F1). This one is from me, not the review pipeline. Thanks @btxu-db for carrying this through five rounds; the code is in good shape. Per-key overlay: agreed. The CacheRuntime expresses deltas on top of the CacheRuntimeClass template: "A key neither side declares is cleared": agreed. The three behaviour changes this brings (the CacheRuntime drops its last key, the template drops its last key, a value on the AdvancedStatefulSet that neither side declares) all come from the
The "kept" rows are a side effect of that No upgrade concern. CacheRuntime hasn't shipped: it isn't in v1.0.8 or on One change before merge: please don't document live template propagation. Remove or neutralize:
A neutral sentence is enough, e.g. "How CacheRuntimeClass edits affect existing CacheRuntimes is tracked in #6213; don't rely on them reaching existing instances." Here's why. CacheRuntimeClass is cluster-scoped, the controller doesn't watch it, and every CacheRuntime is requeued every 90s by default. A single class edit would therefore resize or recreate the master and worker pods of every CacheRuntime using that class, across all namespaces, within about one requeue interval. Where in-place resize isn't available the pods are recreated, which means a cold cache for memory and emptyDir tiers. Also, only So the plan is to pin the template when the components are created and keep this PR's per-key overlay on top of that pinned baseline. The proposal is in #6213. It is a follow-up to this PR and should land before v1.1.0, the first release with CacheRuntime. No code change is needed here: reading the live class on master is fine until then, because nothing has shipped. Once the baseline is pinned, the class-edit case of F11 goes away, because container claims and pod-level With the doc change and F13, this is good to merge from my side. |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 6 (ce4f0c4f → a773dfa6): one commit, and it closes the last open risk with proof. The F11 class-claims wedge — a CacheRuntimeClass edit adding a pod-level resourceClaims entry plus the matching container claim, half-propagating to existing workloads and stalling their rollouts with no signal — is fixed by the exact guard shape round 5 proposed: splitDeclaredClaims splits the merged container claims by what the workload's pod spec declares (pod-level, or already carried by the container), and validDesiredResources drops the undeclared ones, emits one RuntimeResourcesConflict Warning naming them, and still syncs requests and limits, so a claim-only edit cannot hold back a limits edit on non-DRA clusters. Both harnesses prove it independently: one contract flips fail → pass (still-broken at the previous head, green here), and a second three-ref differential is red on the merge base (wholesale apply), red on the previous head (claim propagated, no warning), green at head (drop + named Warning + convergence), with pass-through pins confirming no over-drop of declared or already-carried claims. Full credit to Codex for self-disclosing that the round-4 canary could structurally never report a fix: it is retired and replaced by polarity-correct contracts, and no earlier conclusion rested on it. The full re-verify sweep is green on both harnesses with no exceptions; the engine suite shows no revision regression (329 passed vs 321, +8 new claim specs, the 12 failures byte-identical and pre-existing); and every CI lane is green at the exact head — DCO included, e2e on all five Kubernetes versions.
The process side moved too. The maintainer decision this PR has waited six rounds for has landed (F1 closed): the 2026-10-20 comment — a personal decision, not a pipeline artifact — agrees to the per-key overlay and to "a key neither side declares is cleared", and confirms there is no upgrade concern. The same comment records one change before merge, which is the round's one new finding and the only disagreement between the reviewers — a severity label, not a fact, since both confirm the finding, the evidence, and the gate. The en/zh docs and the PR description still advertise live CacheRuntimeClass template-edit propagation to existing instances (the "Edits to the template's resources reach existing instances" bullet, the "change the CacheRuntimeClass template" advice, and the matching Behaviour-changes line). The maintainer asks these be neutralized and pointed at #6213, which will pin the class template at creation — and the mechanism checks out under both reviewers' independent verification: nothing watches the cluster-scoped class, every CacheRuntime requeues on a 90-second default, only resources follows the class, and pod recreation means a cold cache. The documented behavior is true of the code today, so I am treating this as a documentation-policy gate rather than a code defect — but it is the merge owner's recorded pre-merge requirement, so it stays change-requesting until the edit lands. With it and the body cleanup below, the maintainer states the PR is good to merge from their side.
Reviewer C (Copilot) reviewed this head, marked its historical claims thread resolved, and raised no new inline findings; of its other threads, the limits: {} claim stays refuted and the merged-request-validation one is what rounds 3-4 already implemented.
Next round
[MUST] F14: neutralize the live-template-propagation passages — the "Edits to the template's resources reach existing instances" bullet (en:101, zh:97), the "change the CacheRuntimeClass template" advice (en:96-99, zh:94-96), and the matching PR-body Behaviour-changes line — pointing at #6213 instead (the maintainer's recorded 2026-10-20 pre-merge requirement).
[MUST] F13: drop the stale "Upgrade note." paragraph from the PR description and refresh the adjacent claims bullet, which now describes only the creation path (both named in the same maintainer comment).
[SHOULD] none further — all code findings are fixed and verified at head a773dfa6.
[Filed] #6213 (maintainer-filed): pin the CacheRuntimeClass template at creation; subsumes the class-edit case of F11 before the first CacheRuntime release.
Verified review delta
- F14: open (initial-pr)
- F11: fixed (preexisting)
- F1: fixed (initial-pr)
Code: needs-work. CI: success; attribution: baseline. Required CI checks still apply.
| unconstrained. To relax a limit, set the value you want explicitly; to drop a requirement | ||
| altogether, change the CacheRuntimeClass template, which is where the runtime's own requirements | ||
| are described. | ||
| - ⚠️ **Edits to the template's `resources` reach existing instances**: the CacheRuntimeClass template |
There was a problem hiding this comment.
F14 (new this round, raised independently by both reviewers; severity adjudicated by the synthesis — Codex major, Claude minor, both confirmed existence, content, attribution and the merge gate, and Claude explicitly consented to this finding occupying the blocking slot, so the dispute is the label, not the finding): at head a773dfa both language docs still carry the 'resources reach existing instances' bullet (en:101, zh:97), the 'overridden but not removed' bullet still advises 'change the CacheRuntimeClass template' to drop a requirement (en:96-99, zh:94-96), and the PR description still lists 'edits to the template's resources reach existing instances' under Behaviour changes. The maintainer's recorded decision (cheyang, issue comment 2026-10-20T12:38:43Z, self-identified personal decision, not a pipeline artifact) gates merge on neutralizing exactly these passages — a neutral pointer to #6213 suffices ('How CacheRuntimeClass edits affect existing CacheRuntimes is tracked in #6213; don't rely on them reaching existing instances') — and states 'With the doc change and F13, this is good to merge from my side.' The rationale was independently re-verified by both reviewers: the controller registers no watch on CacheRuntimeClass, every CacheRuntime requeues on defaultRuntimeReconcileDuration = 90s (pkg/utils/crtl_utils.go:38), so one cluster-scoped class edit resizes or recreates the master/worker pods of every CacheRuntime using that class within about one interval (recreation = cold cache for memory/emptyDir tiers), while only resources follows the class — image, args, env, volumes and pod-level claims keep creation-time values (F11 was exactly that mismatch). #6213 (OPEN, maintainer-filed 2026-10-20) pins the template at creation, so the documented propagation semantic stops being true once it lands; advertising it in merged docs entrenches a contract the project intends to remove before the first release carrying CacheRuntime. The content is PR-introduced (absent at merge base e0fc4c1, grep count 0) and the round-6 commit predates the decision (committed 2026-10-19, decision 2026-10-20), so this is pending work, not author pushback and not a revision regression. The documented behavior is accurate against the code at head — a documentation-policy correction, not a code defect; the change-requesting weight rests on the merge owner's recorded pre-merge requirement, which both debaters verify and relay, and under which the required action is identical regardless of the severity label.
Ⅰ. Describe what this PR does
A CacheRuntime that names only some resource keys loses every requirement it does not
restate. The practical case is an owner who wants to raise one number: patching just
limits.memoryon a worker whose CacheRuntimeClass template declares a full set leaves thecontainer with no CPU request and no CPU limit at all.
There is no error and no event. The scheduler stops reserving CPU for the worker and nothing
caps its CPU on the node;
requests.memorythen comes back as8Gibecause Kubernetesdefaults requests to limits, silently tripling the memory reservation the template asked for.
Why it happens. The CacheRuntimeClass template and the CacheRuntime are treated as
alternatives — one or the other, never both. Two places make that assumption, and they have
to agree, because one renders the workload and the other rebuilds the desired state on every
reconcile:
ResourceRequirementscarriesLimits,RequestsandClaims, so replacing it wholesalediscards all three.
updateResources(advanced_statefulset_manager.go:289) then writes thatvalue onto the container, which is correct at its own level — it is handed a complete desired
state and has no way to know part of it went missing upstream.
Approach. The two are not alternatives. The template carries the runtime's own
requirements; the CacheRuntime expresses the deltas an owner wants for their instance. This PR
overlays them key by key through one helper,
mergeResourceRequirements, that both paths call,so the creation path and the sync path cannot drift apart and leave the workload rolling on
every reconcile.
LimitsandRequestsmerge by resource name;Claimsby claim name, withthe template's order preserved and unnamed claims appended.
updateResourcesis left alone deliberately. It receives a fully resolved desired state, andteaching the workload layer about the CacheRuntimeClass would put the template in two places.
The resolved value depends only on the current template and the current CacheRuntime: per
key, the CacheRuntime's value if it names one, otherwise the template's, and a key neither
declares is cleared.
desiredComponentResourcestherefore no longer returnsnil("leave theworkload untouched", introduced in #6165). That
nilexisted to keep the template's values whenthe CacheRuntime sets none; the merge now does that on its own, and the
nilhad only oneeffect left — a value could not be removed once neither side declared anything. An empty list
such as
limits: {}names no key, so it reads the same as leavinglimitsout. (Convergence isunaffected:
Semantic.DeepEqualtreats nil and empty maps as equal, so a freshly createdworkload is not patched on its first sync.)
Master, worker and client all go through
transformComponentPodTemplateon creation. Onlymaster and worker are synced, since client runs as a DaemonSet and is deliberately not synced.
Ⅱ. Does this pull request fix one issue?
fixes #6173
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
transform_common_test.gois new and covers the helper and the resolver directly:mergeResourceRequirements— an overlay naming one key moves it and keeps the rest of thebaseline; an overlay naming a key the baseline does not declare adds it; neither side
declaring anything leaves the lists nil; the baseline it is handed is not mutated; claims
are kept, replaced by name, and appended.
desiredComponentResources— resolves to the template when the CacheRuntime sets nothing;keeps the template's other requirements when the CacheRuntime only raises the memory limit
(the reported case); resolves to empty resources when neither declares anything; resolves to the CacheRuntime
when only it declares; does not mutate the CacheRuntimeClass template.
Two specs in
sync_test.godrive the whole sync against a fake client, under"when the CacheRuntime only names part of the template's resources":
form of the report — full template,
limits.memoryraised on the CacheRuntime, CPU request,CPU limit and memory request expected to survive. Copying this spec into a worktree at the
base commit — test present, fix absent — fails on the CPU limit with
Expected <string>: 0 to equal <string>: 2, i.e. the container has no CPU limit at all,which is the reported symptom.
nothing to carry over for stay unset, and a second sync leaves the workload's
resourceVersionalone.Further
sync_test.gospecs: the same partial override on the master; "should clear theworkload's resources" (empty template, workload carrying
8Gi, CacheRuntime declaring nothing);and "should restore the template value when the CacheRuntime specifies none", which is the
"remove an override, fall back to the template" case. Restoring the
nilreturn fails theclear spec and the empty case in
transform_common_test.go.resources_equivalence_test.gopins the invariant the shared helper is meant to give: for atable of template/CacheRuntime combinations (neither side, either side, a partial override, a
key the template lacks, empty lists on either side, claims on both), the creation path
(
transformComponentPodTemplate) and the sync path (desiredComponentResources) resolve tothe same resources, and a workload rendered by the former is not patched by
syncRuntimeSpec.Reverting either path to the old wholesale replace fails it.
The curvine e2e suite (
test/gha-e2e/curvine) now declares a full set of resources on theworker template, patches only
limits.memoryon the CacheRuntime, and checks that theAdvancedStatefulSet rolls out with the new limit while keeping the template's CPU limit and
requests.
The existing
syncRuntimeSpecspecs from #6165 pass unchanged, which is the evidence that the"template value when the CacheRuntime sets nothing" behaviour is preserved.
Ⅳ. Describe how to verify it
On a cluster, with the manifests from #6173 — a template declaring
requests {cpu 1, memory 2Gi}andlimits {cpu 2, memory 4Gi}, and a CacheRuntime thatpatches
limits.memoryto8Gi:kubectl get advancedstatefulset restest-worker \ -o jsonpath='{.spec.template.spec.containers[0].resources}'resourcessilently drops the rest of the container's resource requirements #6173):{"limits":{"memory":"8Gi"}}{"limits":{"cpu":"2","memory":"8Gi"},"requests":{"cpu":"1","memory":"2Gi"}}A CacheRuntime that sets no resources at all stays on the template values across reconciles,
unchanged from #6165. With a template that declares no resources, setting
limits.memoryon theCacheRuntime and then removing it again clears the limit from the workload.
Ⅴ. Special notes for reviews
Semantics. This implements the two rules proposed in the
#6165 review:
value from the CacheRuntime or the CacheRuntimeClass is taken, key by key;
Behaviour changes compared with
master, all documented indocs/{en,zh}/samples/cacheruntime/cacheruntime_spec_update.md:resourceskeeps the template's other keys ([BUG]CacheRuntime: a partially specifiedresourcessilently drops the rest of the container's resource requirements #6173);does not declare it (on
master, with a template that declares no resources, the workloadkept the old value);
are reverted on the next reconcile (on
masterthey were kept when neither side declared any);resourcesreach existing instances on their next reconcile, for thekeys the CacheRuntime does not override;
claimsis no longer ignored; a claim must reference a pod-levelresourceClaimsentry the template declares, otherwise the API server rejects the Pod.Upgrade note. A workload created by an earlier release from a partially specified
CacheRuntime is missing the template's other requirements. The first reconcile after upgrade
restores them, which updates those pods once (in place where the cluster can resize them,
otherwise by recreating them). The end state is the intended one; the update is the repair.
Distinct from #6161 despite landing on the same lines. #6161 is "the CacheRuntime specifies no
resources at all and the template's values get cleared", fixed by #6165 with a nil check. A
partially filled value is still non-nil, so it survives that fix — the reproduction above was
run on a build that already contains #6165. Also distinct from #6166: that one is about the
tiered store quota being dropped from the container's memory accounting, this one about the
rest of the requirements being dropped from the workload. They share the code path but neither
fix depends on the other. This PR is based on a
masterthat already contains #6167: the tieredstore memory quota is charged on top of the merged baseline.