Extend CacheRuntime phase 2.3: remount when dataset mount changed or cache runtime master restarted#5834
Extend CacheRuntime phase 2.3: remount when dataset mount changed or cache runtime master restarted#5834xliuqq wants to merge 4 commits into
Conversation
Signed-off-by: xliuqq <xlzq1992@gmail.com> update openapi and crd Signed-off-by: xliuqq <xlzq1992@gmail.com> 添加脚本更新，以及说明文档 Signed-off-by: xliuqq <xlzq1992@gmail.com> fix test Signed-off-by: xliuqq <xlzq1992@gmail.com> add test
|
[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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements dynamic UFS mount updates for the generic cache runtime by executing a MountUFS script and parsing its JSON output to synchronize dataset status. The changes include API updates to CacheRuntimeStatus, new documentation for integration requirements, and the core logic in the engine to handle UFS changes and remounting. Review feedback identifies a high-severity issue where missing updates to dataset.Status.Mounts could cause infinite reconciliation loops. Other suggestions focus on making JSON parsing more robust, improving logging conciseness by avoiding full object serialization, and fixing typos in API documentation.
There was a problem hiding this comment.
Pull request overview
This PR extends the generic CacheRuntime “MountUFS” integration so the cache runtime can remount dynamically when Dataset.spec.mounts changes or when the master pod restarts, using a MountUFS script whose stdout is parsed as JSON to sync mount state.
Changes:
- Add CacheEngine UFS change detection + remount-on-master-restart logic, driven from
CacheEngine.Sync(). - Introduce/propagate
status.mountTimeand related status/CRD updates to track the last successful mount time. - Update Curvine e2e MountUFS script + docs to require MountUFS stdout to be JSON matching
CacheRuntimeMountUfsOutput, and add Sync unit tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| test/gha-e2e/curvine/mount.yaml | Adjust MountUFS script to be quiet on stdout and emit {"mounted":[...]} JSON for Fluid parsing. |
| pkg/ddc/cache/engine/ufs.go | Implement UFS delta detection, remount checks on master restart, MountUFS stdout JSON parsing, and dataset mount sync. |
| pkg/ddc/cache/engine/sync.go | Invoke UFS-change handling as part of CacheEngine Sync flow. |
| pkg/ddc/cache/engine/sync_test.go | Add Ginkgo unit tests covering Sync/configmap generation scenarios (currently limited coverage of new UFS path). |
| pkg/ddc/cache/engine/setup.go | Update Setup to call new PrepareUFS(runtimeClass) signature. |
| pkg/ddc/cache/engine/runtime.go | Add updateMountTime() helper to persist last mount timestamp in CacheRuntime status. |
| pkg/ddc/cache/engine/master.go | Change master pod/container resolution to derive container name from CacheRuntimeClass template. |
| pkg/ddc/cache/engine/fileutils.go | Return Mount command stdout so MountUFS output can be parsed by controller logic. |
| pkg/ddc/cache/engine/dataset.go | Update runtime MountTime when binding dataset. |
| docs/zh/dev/generic_cache_runtime_integration.md | Document MountUFS JSON stdout contract and usage scenarios (Step 2.7). |
| docs/en/dev/generic_cache_runtime_integration.md | English version of MountUFS JSON stdout contract and guidance (Step 2.7). |
| config/crd/bases/data.fluid.io_cacheruntimes.yaml | CRD schema updates for status.mountTime + status.mounts shape. |
| charts/fluid/fluid/crds/data.fluid.io_cacheruntimes.yaml | Helm CRD schema mirror of the same status changes. |
| api/v1alpha1/zz_generated.deepcopy.go | Deepcopy updates for new/changed status fields and MountUFS output type. |
| api/v1alpha1/status.go | Update RuntimeStatus comments around mounts (minor doc tweak). |
| api/v1alpha1/common.go | Add CacheRuntimeMountUfsOutput type definition. |
| api/v1alpha1/cacheruntimeclass_types.go | Update MountUFS comment to indicate JSON output requirement. |
| api/v1alpha1/cacheruntime_status.go | Replace older mount status struct with Mounts + MountTime fields on CacheRuntime status. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [ -z "$mountPoint" ] && { echo "mountPoint is not set or empty" >&2; exit 1; } | ||
| [ -z "$path" ] && { echo "path is not set or empty" >&2; exit 1; } |
| err := engine.Sync(ctx) | ||
| // Sync may fail at UpdateOnUFSChange due to missing master pod, but ConfigMap should be created | ||
|
|
||
| // Verify ConfigMap was created | ||
| configMap := &corev1.ConfigMap{} | ||
| err = fakeClient.Get(context.Background(), types.NamespacedName{ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5834 +/- ##
==========================================
+ Coverage 59.10% 59.64% +0.53%
==========================================
Files 480 480
Lines 32511 32668 +157
==========================================
+ Hits 19215 19484 +269
+ Misses 11746 11607 -139
- Partials 1550 1577 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for dynamic UFS mount updates and status synchronization within the generic cache runtime. Key changes include refactoring CacheRuntimeStatus to simplify mount tracking, implementing logic to detect dataset mount changes or master pod restarts, and requiring mount scripts to output status in a specific JSON format (CacheRuntimeMountUfsOutput). Comprehensive unit tests and documentation updates were also provided. Review feedback highlights opportunities to optimize performance by passing the CacheRuntime object through the call stack to avoid redundant API server requests in the UpdateOnUFSChange, shouldUpdateUFS, and checkIfRemountRequired methods.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 19 changed files in this pull request and generated 9 comments.
Files not reviewed (2)
- api/v1alpha1/openapi_generated.go: Language not supported
- api/v1alpha1/zz_generated.deepcopy.go: Language not supported
| // handle ufs change - support dynamic mount updates | ||
| err = e.updateOnUFSChange(runtime) | ||
| if err != nil { | ||
| e.Log.Error(err, "Failed to update UFS") | ||
| return err | ||
| } |
|
Review of PR #5834 — CacheRuntime phase 2.3: dynamic remount on dataset mount change or master restart. Design direction is sound and the core chain (shouldUpdateUFS → PrepareUFS → SyncDatasetMounts) follows existing Fluid patterns. Two blocking issues need fixing before merge:
Non-blocking: redundant runtimeClass fetch, checkIfRemountRequired error handling, config map timing, comment fixes. Test coverage for the new UFS path is critically low (4.67% patch coverage) — strongly recommend adding targeted unit tests before merge. |
| } | ||
|
|
||
| // 2. set update status to updating | ||
| err = utils.UpdateMountStatus(e.Client, e.name, e.namespace, datav1alpha1.UpdatingDatasetPhase) |
There was a problem hiding this comment.
When updateOnUFSChange sets the dataset phase to Updating, the UpdateMountStatus Updating path does NOT update dataset.Status.Mounts (only phase + condition). If the subsequent MountUFS execution or SyncDatasetMounts fails, the reconciler will re-enter Sync → updateOnUFSChange → shouldUpdateUFS. Since AnalyzePathsDelta compares spec.Mounts vs status.Mounts and the status.Mounts remain stale, shouldUpdateUFS will always return true, causing an infinite reconcile loop with repeated MountUFS invocations.
Consider adding a failure-path phase reset (e.g., revert to Bound on error) or a backoff/retry counter to break the cycle when the dataset gets stuck in Updating.
| } | ||
|
|
||
| return restart | ||
| } |
There was a problem hiding this comment.
updateOnUFSChange fetches runtimeClass via e.getRuntimeClass even though it could be passed in from the caller. Passing runtimeClass as a parameter would eliminate the redundant API call and reduce potential race conditions between multiple fetches of the same object.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
Ⅰ. Describe what this PR does
remount when dataset mount changed or cache runtime master restarted
Ⅱ. Does this pull request fix one issue?
part of #5412
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
unit test for
Syncmethod.Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews