prevent ThinRuntime pointing to a CacheRuntime-backed Dataset and add docs#5901
Conversation
Signed-off-by: xliuqq <xlzq1992@gmail.com>
![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 introduces validation logic to ensure that CacheRuntime is not used as a physical runtime for reference datasets, alongside updated documentation and a new ThinRuntime example using Minio. The CheckIfReferenceDatasetIsSupported method was refactored to return errors, and unit tests were added to verify the new constraints. A review comment suggests making the configuration parsing script in the documentation more robust by reading the full file content instead of just the first line to avoid potential JSON parsing failures.
| with open("/etc/fluid/config/config.json", "r") as f: | ||
| lines = f.readlines() | ||
|
|
||
| rawStr = lines[0] |
There was a problem hiding this comment.
Reading the config file with f.readlines() and then taking the first line with lines[0] is not robust. If the JSON content is pretty-printed across multiple lines, this will fail to parse. It's safer to read the entire file content.
| with open("/etc/fluid/config/config.json", "r") as f: | |
| lines = f.readlines() | |
| rawStr = lines[0] | |
| with open("/etc/fluid/config/config.json", "r") as f: | |
| rawStr = f.read() |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5901 +/- ##
==========================================
+ Coverage 62.59% 63.35% +0.76%
==========================================
Files 480 481 +1
Lines 32801 33070 +269
==========================================
+ Hits 20533 20953 +420
+ Misses 10633 10442 -191
- Partials 1635 1675 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds controller-side validation to prevent ThinRuntime “reference dataset mode” (datasets mounting dataset://...) from referencing a physical Dataset backed by CacheRuntime, and updates sample documentation accordingly.
Changes:
- Extend
RuntimeReconciler.CheckIfReferenceDatasetIsSupportedto reject ThinRuntime reference datasets when the physical dataset’s runtime type isCacheRuntime. - Add/extend unit tests covering ThinRuntime reference dataset acceptance/rejection cases.
- Add/update ThinRuntime and cross-namespace dataset sharing docs (EN + ZH) with explicit constraints.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controllers/runtime_controller.go | Adds runtime-type validation for physical datasets referenced by ThinRuntime reference datasets. |
| pkg/controllers/runtime_controller_test.go | Updates and expands unit tests for reference dataset support checks. |
| docs/zh/samples/thinruntime.md | Adds a highlighted note describing ThinRuntime modes and the CacheRuntime restriction. |
| docs/zh/samples/dataset_across_namespace_with_sidecar.md | Adds a warning note about CacheRuntime not being supported as the physical runtime. |
| docs/zh/samples/dataset_across_namespace_with_csi.md | Adds a warning note about CacheRuntime not being supported as the physical runtime. |
| docs/en/samples/thinruntime.md | Adds a new English ThinRuntime sample doc (Minio example) including the CacheRuntime restriction note. |
| docs/en/samples/dataset_across_namespace_with_sidecar.md | Adds a warning note about CacheRuntime not being supported as the physical runtime. |
| docs/en/samples/dataset_across_namespace_with_csi.md | Adds a warning note about CacheRuntime not being supported as the physical runtime. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. Read the JSON string from the `/etc/fluid/config.json` file. Fluid stores and mounts the parameters required for Fuse client mounting to the `/etc/fluid/config.json` file in the Fuse container. | ||
| 2. Parse the JSON string and extract the parameters required for Fuse client mounting. For example, `url`, `bucket`, `minio-access-key`, `minio-access-secret`, and other parameters in the above example. | ||
| 3. After extracting the required parameters, output the mount script to the file `mount-minio.sh` | ||
|
|
||
| | ⚠️ Note: Starting from Fluid v1.0.0, encryption parameters defined in `dataset.spec.mounts[*].encryptOptions` cannot be directly obtained from the `/etc/fluid/config.json` file. The `/etc/fluid/config.json` file only provides the storage paths for each encryption parameter value, so the parameter parsing script needs to perform additional file reading operations (e.g., "export AWS_ACCESS_KEY_ID=\`cat $akId\`" in the above example). |
There was a problem hiding this comment.
keep the same as chinese version.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
| Fail("Expected an error but got nil") | ||
| } | ||
| Expect(supported).To(BeFalse()) | ||
| Expect(err.Error()).To(ContainSubstring("CacheRuntime is not supported")) |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
The core logic looks good — catching unsupported CacheRuntime at the controller level is the right call since the physical dataset's runtime type may not be available at admission time. A few things need fixing before merge though.
Design question (non-blocking): The current check only rejects CacheRuntime (blacklist style). If new incompatible runtime types appear later, this code needs updating. Worth considering a whitelist approach (only allow known-compatible types) to be more future-proof — but that's a follow-up discussion.
Missing test coverage: There's no test for the case where the physical dataset hasn't been bound yet (Status.Runtimes is empty). GetRuntimeInfo would fall into the default case and error. A test for this path would prevent regressions.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Previous review comments have been addressed. The CheckIfReferenceDatasetIsSupported logic now correctly validates physical runtime types, and the documentation typos are fixed. LGTM.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ⅰ. Describe what this PR does
prevent ThinRuntime pointing to a CacheRuntime-backed Dataset and add docs
Ⅱ. Does this pull request fix one issue?
fixes #5876
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
unit test
Ⅳ. Describe how to verify it
unit test
Ⅴ. Special notes for reviews