fix(cache): build RuntimeInfo from the worker tiered store - #6181
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. |
e925c57 to
baee002
Compare
CacheEngine.getRuntimeInfo built its RuntimeInfo with an empty TieredStore, so convertToTieredstoreInfo returned no levels, GetLevelStorageMap returned an empty map, and labelNodeWithCapacityInfo never added the memory or disk capacity labels while writing the total one unconditionally as 0B. A CacheRuntime declaring a real tiered store advertised no cache capacity on any node it landed on. Every other engine passes its runtime's tiered store here; CacheRuntime was the only one passing a zero value, because RuntimeTieredStore and the legacy TieredStore that base.WithTieredStore consumes describe the storage medium differently and no conversion existed. convertToLegacyTieredStore bridges the two. The legacy Level carries an explicit MEM/SSD/HDD enum while RuntimeTieredStoreLevel names the medium structurally, so only the memory-versus-disk distinction survives - which is all the consumers need, as GetLevelStorageMap buckets SSD and HDD together. Disk-backed levels are reported as HDD to match the medium type extractTieredStoreLevels already writes into the runtime config ConfigMap. Host path levels are converted through QuotaList rather than Quota so that per-path quotas keep their declared distribution instead of being averaged across the paths. Only the worker tiered store feeds RuntimeInfo. Nodes are labelled off worker pod placement in getDesiredNodesWithScheduleInfo, so a client tier would be counted where a worker happens to co-reside and dropped everywhere else. Note that the capacity labels are written once, when a node first enters the cache node set: calculateNodeDifferences only visits newly added nodes and addScheduleInfoToNode skips a node that already carries the runtime label. A CacheRuntime that existed before this change therefore keeps its 0B label until it is recreated. Fixes fluid-cloudnative#6174 Signed-off-by: btxu-db <btxu-db@outlook.com>
baee002 to
68e98ac
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6181 +/- ##
==========================================
+ Coverage 65.24% 65.28% +0.04%
==========================================
Files 486 486
Lines 34194 34239 +45
==========================================
+ Hits 22309 22354 +45
Misses 10135 10135
Partials 1750 1750 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|



Ⅰ. Describe what this PR does
CacheEngine.getRuntimeInfobuilt itsRuntimeInfowith an emptyTieredStore, soconvertToTieredstoreInforeturned no levels,GetLevelStorageMapreturned an empty map, andlabelNodeWithCapacityInfonever added the memory or disk capacity labels while writing the total one unconditionally as0B. A CacheRuntime declaring a real tiered store advertised no cache capacity on any node it landed on.Every other engine passes its runtime's tiered store here; CacheRuntime was the only one passing a zero value, because
RuntimeTieredStoreand the legacyTieredStorethatbase.WithTieredStoreconsumes describe the storage medium differently and no conversion existed.This PR adds
convertToLegacyTieredStoreto bridge the two, and wires it intogetRuntimeInfo.Two points from the issue that were product decisions rather than mechanical ones:
Levelcarries an explicitMEM/SSD/HDDenum whileRuntimeTieredStoreLevelnames the medium structurally throughProcessMemory,EmptyDirandHostPath. Only the memory-versus-disk distinction survives, which is all the consumers need —GetLevelStorageMapbucketsSSDandHDDtogether. Disk-backed levels are reported asHDD, matching the medium typeextractTieredStoreLevelsalready writes into the runtime config ConfigMap.RuntimeInfo. Nodes are labelled off worker pod placement ingetDesiredNodesWithScheduleInfo, so a client tier would be counted on nodes where a worker happens to co-reside and silently dropped everywhere else.Host path levels are converted through
QuotaListrather thanQuota, so per-path quotas keep their declared distribution instead of being averaged across the paths byconvertToTieredstoreInfo.Ⅱ. Does this pull request fix one issue?
fixes #6174
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Unit tests for
convertToLegacyTieredStoreinpkg/ddc/cache/engine/transform_tiered_store_test.go, covering:RuntimeTieredStoreproducing no levelsProcessMemorymapping toMEMwith the/dev/shmpath, and thehigh/lowwatermarks carried throughEmptyDirwithmedium: Memorymapping toMEM, and with the default medium to a disk mediumHostPathkeeping its per-path quotas instead of collapsing them into oneHostPathlevel whosepathsandquotasdisagree in length being skipped rather than emittedPlus one test that feeds the result through
convertToTieredstoreInfoto check the levels are accepted and their quotas summed correctly, which is the actual path from this conversion to the node labels.Ⅳ. Describe how to verify it
go test ./pkg/ddc/cache/engine/... ./pkg/ddc/base/... -gcflags=all=-lEnd to end, with the CacheRuntime from the issue:
fluid.io/s-h-cache-t-default-mooncake-demonow reports1GiBinstead of0B, andfluid.io/s-h-cache-d-default-mooncake-demois present. Switching the level toemptyDir: {quota: 1Gi, medium: Memory}or toprocessMemory: {quota: 1Gi}yieldsfluid.io/s-h-cache-m-...instead.Ⅴ. Special notes for reviews
The capacity labels are written once, when a node first enters the cache node set:
calculateNodeDifferencesonly visits newly added nodes, andaddScheduleInfoToNodeskips a node that already carries the runtime label. A CacheRuntime that existed before this change therefore keeps its0Blabel until it is recreated. Worth deciding separately whether that is worth addressing.Note this is a separate problem from the tiered store memory quota being dropped from the worker memory limit (#6166, fixed by #6167): that one concerns the container's memory accounting, this one concerns the node labels. They share the
tieredStorespec field but neither fix depends on the other.