fix(cache): merge CacheRuntime resources onto the CacheRuntimeClass baseline - #6177
fix(cache): merge CacheRuntime resources onto the CacheRuntimeClass baseline#6177btxu-db wants to merge 3 commits into
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. |
4c8960e to
766461f
Compare
…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 fluid-cloudnative#6165, settled here in favour of the template staying the baseline. 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>
766461f to
c6277f4
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6177 +/- ##
==========================================
+ Coverage 65.20% 65.24% +0.04%
==========================================
Files 486 486
Lines 34151 34181 +30
==========================================
+ Hits 22267 22303 +36
+ Misses 10134 10130 -4
+ Partials 1750 1748 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|



Ⅰ. 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.
Nil is preserved rather than normalised to an empty map:
Semantic.DeepEqualseparates thetwo, so an empty map would never compare equal to a workload rendered with nil and the sync
would patch on every reconcile. A component that neither side gives resources to still
resolves to
nil, which is the "leave the workload untouched" contract from #6165.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); returns nil 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.
requests stay nil and a second sync leaves the workload's
resourceVersionalone. Droppingthe empty-overlay guard in
mergeResourceListfails it.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.
Ⅴ. Special notes for reviews
The semantic decision this needs. Once the values are overlaid, a key the CacheRuntimeClass
template declares can be overridden but no longer removed by omitting it from the CacheRuntime.
This is the same "user can not remove resource" concern raised in the review of #6165, and it
is settled here in favour of the template staying the baseline: removing a requirement is a
change to the template, which is where the runtime's requirements are described in the first
place. Worth confirming this is the semantics the project wants before the behaviour ships,
since the alternative — a
nullsentinel or an explicit removal list — is an API change ratherthan a bug fix.
docs/{en,zh}/samples/cacheruntime/cacheruntime_spec_update.mdis updated tosay so.
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 rolls those pods once. The end state is the intended one; the roll 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 cut against
masterand does not include #6167, sowhichever lands second will need a small rebase in
syncRuntimeSpec.