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 #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 #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 #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 #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 (#6165, #6167, #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 #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>
Resolve the conflicts with #6186, #6223 and #6162. All five are additive: - constants.go: keep both RuntimeResourcesConflict and RuntimeVersionIncomplete. - transform_common.go(+_test): keep the resource merge/validation helpers and the runtime version completion helpers side by side. - sync.go: the master block reads the pod spec for validDesiredResources and completes the version against the template image, mirroring the worker block. - curvine test.sh: keep update_worker_resources_and_verify ahead of the scale checks rewritten for the scale subresource. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: cheyang <cheyang@163.com>
updateResources compared the container resources with Semantic.DeepEqual, which is order sensitive for claims. A reordered claim list carries no change for the Pod, but rolled the workload, and for an AdvancedStatefulSet any pod template change other than the image recreates the Pods. Compare claims by name instead. Also correct the SyncComponentSpec comment: the bundled AdvancedStatefulSet controller only updates container images in place; resources changes recreate the Pods. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: cheyang <cheyang@163.com>
The CacheRuntimeClass was read live on every reconcile and every data operation. Since #6165, #6177 and #6186, edits to a class template's resources and image reached the master and worker of every CacheRuntime using the class, in every namespace, within one requeue. The bundled AdvancedStatefulSet controller recreates Pods for any resources change and restarts containers for an image change, so a single class edit rebuilt the caches of all those runtimes. Class options, execution entries and data operation specs were read live as well, so a class upgrade would hand new scripts and options to components still running the old images. A CacheRuntime is now bound, at its first Setup, to a snapshot of its class, stored in an immutable ConfigMap fluid-runtime-class-snapshot-<runtime> owned by the runtime. Setup, Sync, the runtime config, UFS updates and DataLoad read the class only from the snapshot. Editing or deleting a class therefore only affects the CacheRuntimes created afterwards. A runtime created before snapshots existed is bound on its next reconcile. Its snapshot is taken from the current class, except for the master and worker resources and image, which are adopted from what their workloads run (the worker's tiered store quota taken back out, the keys the runtime overrides left out), so binding changes no running component. A component whose workload would still change is listed in the adoption-pending annotation of the snapshot: only its replicas are synced, with a RuntimeAdoptionPending Warning event, until the entry is removed or the workload already runs what the runtime declares. The pending set is returned from the snapshot read or created in the reconcile itself rather than read back, since the informer cache does not see a snapshot created moments ago; a create racing the cache waits for the existing snapshot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: cheyang <cheyang@163.com>
…ects Describe that a CacheRuntime is bound to its CacheRuntimeClass as it was at setup, how runtimes created by an earlier release are bound and when their adoption is held, and what a runtimeVersion or resources update does to the running components: the bundled AdvancedStatefulSet controller restarts the containers for an image change and recreates the Pods for a resources change. Drop the K8s >= 1.27 InPlacePodVerticalScaling and cgroupv1 caveats, which describe an in-place resize Fluid does not perform. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: cheyang <cheyang@163.com>
… runtime Edit the curvine class's worker template resources and options after the runtime is up, force a reconcile, and check that the worker AdvancedStatefulSet generation and resources and the runtime config are unchanged. The class is restored afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: cheyang <cheyang@163.com>
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6239 +/- ##
==========================================
+ Coverage 65.75% 65.94% +0.19%
==========================================
Files 486 488 +2
Lines 34459 34901 +442
==========================================
+ Hits 22660 23017 +357
- Misses 10044 10112 +68
- Partials 1755 1772 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
Stacked on #6177. Please review only the four commits after
5b16f186.5b16f186merges currentmasterinto #6177's head (94a82543) and resolves its conflicts with #6186, #6223 and #6162. This PR is not meant to merge on its own. Once both are approved, its commits go onto #6177's branch so that #6177 and this change land in one squash, and this PR is closed.Since #6165, #6177 and #6186, the controller reads the CacheRuntimeClass live on every reconcile. Editing a class's template
resourcesorimagereaches the master and worker of every CacheRuntime that uses the class, in every namespace, within one requeue (90s by default). The class is cluster-scoped and the controller does not watch it.The AdvancedStatefulSet controller that Fluid bundles (run in-process by
cmd/cache/app/cache.go) only updates container images in place:defaultCalculateInPlaceUpdateSpecaccepts nothing butreplaceoncontainers[x].image;NativeVerticalUpdateexists in the vendored code but is not wired up.So a single class edit restarts the containers (image) or recreates the Pods (resources) of all those runtimes, and they lose their memory and
emptyDircache.options,executionEntriesanddataOperationSpecsare also read live, through the runtime config ConfigMap, the mountUFS/reportSummary commands and DataLoad. A class is a versioned bundle of these, so following only part of it live would hand new scripts and options to components still running the old images.This PR binds a CacheRuntime, at its first Setup, to a snapshot of the whole class:
fluid-runtime-class-snapshot-<runtime>, controlled by the CacheRuntime. It is labelled with the runtime and annotated with the class name.Setup,Sync, the runtime config,UpdateOnUFSChangeand DataLoad read the class only through the snapshot. This replaces everygetRuntimeClasscall in the runtime controller path.runtimeClassNamechanged: the runtime stays bound to its snapshot and Fluid emits aRuntimeClassChangedWarning.RuntimeClassSnapshotInvalid) and never overwritten. Deleting it re-binds the runtime.resourcesandimage. Those are adopted from the workloads, without reading the class: the worker's tiered-store memory quota is taken back out, and keys the runtime overrides are left out.cacheruntime.fluid.io/adoption-pendingannotation.replicasis synced, with aRuntimeAdoptionPendingWarning. This lasts until the user removes the entry, or the workload already runs what the runtime declares.CreatehitsAlreadyExistsbecause of cache lag, it waits briefly for the existing snapshot instead of taking another.updateResourcesnow compares claims as a set keyed by name. A reordered claim list no longer rolls the workload.Ⅱ. Does this pull request fix one issue?
fixes #6213. It lands through #6177; this PR will be closed rather than merged.
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
runtime_class_snapshot_test.go(new, 23 specs):runtimeClassNamechange keeps the snapshot and warns.AlreadyExistswaits for the existing snapshot.Syncbehind a client that hides newly created ConfigMaps still holds the pending component (the case flagged in review).Syncend to end: a class edit leaves the ASTS and the runtime config ConfigMap untouched.adoptComponentBaseline: quota, master, quota not invertible, claims order, partialruntimeVersion, merge conflict.sync_component_spec_test.go:curvine e2e (
verify_runtime_class_edit_not_propagated) edits the class's worker template resources and options, forces a reconcile, and checks that:Mutation checks (run locally, then reverted):
getBoundRuntimeClassreading the live class fails 14 of the 15 snapshot specs;AlreadyExistsfails the wait spec.Ⅳ. Describe how to verify it
The kind e2e was not run locally; CI runs it.
On a cluster:
kubectl get cm fluid-runtime-class-snapshot-<name>.templateresources or image,options) and annotate the runtime to force a reconcile.metadata.generationand thefluid-runtime-config-<name>ConfigMap do not change.Ⅴ. Special notes for reviews
resourcesandimageleaves options, scripts and data operations live. A class upgrade would then mix versions inside a running runtime.resourcessilently drops the rest of the container's resource requirements #6173 are not repaired automatically. CacheRuntime has not been released, so few if any exist.cacheruntime_spec_update.md(en/zh) now states that a resources change recreates the Pods and an image change restarts the containers.InPlacePodVerticalScalingand cgroupv1 caveats are dropped; they describe a resize Fluid does not perform.