feat(fluid): Init generic cacheruntime api #5396
Conversation
Summary of ChangesHello @Syspretor, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational generic Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5396 +/- ##
=======================================
Coverage 57.52% 57.53%
=======================================
Files 444 444
Lines 30744 30744
=======================================
+ Hits 17687 17690 +3
+ Misses 11443 11441 -2
+ Partials 1614 1613 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 a new generic CacheRuntime API, which is a significant and well-structured addition to the project. The new API definitions are clear and follow existing patterns. I have identified a few minor areas for improvement, including a naming inconsistency in a JSON tag, a misleading comment due to copy-pasting, a couple of empty structs that could benefit from explanatory comments, and some commented-out code that should be cleaned up. Overall, the changes are solid.
api/v1alpha1/cacheruntime_status.go
Outdated
| RuntimeComponentStatusCollection `json:",inline"` | ||
|
|
||
| // MountPointsStatuses represents the status of mount points specified in the bounded dataset | ||
| MountPointsStatuses []MountPointStatus `json:"mountPointsStatus,omitempty"` |
There was a problem hiding this comment.
The JSON tag mountPointsStatus is singular, while the field name MountPointsStatuses is plural. For consistency with other fields and to avoid potential confusion, it's recommended to make the JSON tag plural as well.
| MountPointsStatuses []MountPointStatus `json:"mountPointsStatus,omitempty"` | |
| MountPointsStatuses []MountPointStatus `json:"mountPointsStatuses,omitempty"` |
api/v1alpha1/cacheruntime_types.go
Outdated
| type CacheRuntimeClientSpec struct { | ||
| GenericCacheRuntimeComponentCommonSpec `json:",inline"` | ||
|
|
||
| // TieredStore defines tiered storage configuration used by worker |
There was a problem hiding this comment.
The comment for TieredStore within CacheRuntimeClientSpec states that it's for the 'worker'. This appears to be a copy-paste error from CacheRuntimeWorkerSpec. Please update the comment to correctly refer to the 'client' component.
| // TieredStore defines tiered storage configuration used by worker | |
| // TieredStore defines tiered storage configuration used by client |
| type CacheRuntimeComponentHeadlessService struct { | ||
| } |
There was a problem hiding this comment.
The CacheRuntimeComponentHeadlessService struct is currently empty. If this is intended as a placeholder for future enhancements, adding a comment to that effect would improve code clarity for other developers.
| type CacheRuntimeComponentHeadlessService struct { | |
| } | |
| type CacheRuntimeComponentHeadlessService struct { | |
| // This is a placeholder for future extensions. | |
| } |
| type EncryptOptionConfig struct { | ||
| } |
There was a problem hiding this comment.
The EncryptOptionConfig struct is empty. If this is a placeholder for future use, please add a comment to clarify its purpose. This helps maintainers understand the intent.
| type EncryptOptionConfig struct { | |
| } | |
| type EncryptOptionConfig struct { | |
| // This is a placeholder for future extensions. | |
| } |
api/v1alpha1/common.go
Outdated
| //tieredStore: | ||
| // levels: | ||
| // - quota: 8Gi # quota will add to component.container.resource.request/limit.memory | ||
| // high: "0.99" | ||
| // low: "0.99" | ||
| // mediumSource: | ||
| // processMemory: {} | ||
|
|
||
| // ProcessMemoryMediumSource describes the process memory medium source | ||
| type ProcessMemoryMediumSource struct { | ||
| } | ||
|
|
||
| //tieredStore: | ||
| // levels: | ||
| // - quota: 8Gi | ||
| // high: "0.99" | ||
| // low: "0.99" | ||
| // path: /dev/shm | ||
| // mediumSource: | ||
| // emptyDir:{} | ||
| // ephemeral: | ||
| // volumeClaimTemplate:{} | ||
| // hostPath:{} |
There was a problem hiding this comment.
These YAML examples are commented out directly in the code. To improve code clarity and maintainability, please consider moving these examples into the GoDoc for the respective structs (ProcessMemoryMediumSource and VolumeMediumSource) or removing them if they are temporary notes.
There was a problem hiding this comment.
Pull request overview
This PR introduces a generic CacheRuntime API framework for Fluid, providing a standardized way to define and manage cache runtimes. The implementation includes:
- New CacheRuntime and CacheRuntimeClass CRD definitions
- Generic component specifications for master, worker, and client components
- Flexible tiered storage configuration with support for process memory and volume-based storage
- Comprehensive status tracking for runtime components and mount points
Reviewed changes
Copilot reviewed 7 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/zh/dev/api_doc.md | Added comprehensive API documentation for CacheRuntime, CacheRuntimeClass, and related types with detailed field descriptions |
| config/crd/bases/data.fluid.io_cacheruntimes.yaml | New CRD definition for CacheRuntime with full OpenAPI schema validation |
| charts/fluid/fluid/crds/data.fluid.io_cacheruntimes.yaml | Helm chart CRD definition (identical to config/crd/bases version) |
| api/v1alpha1/zz_generated.deepcopy.go | Auto-generated deep copy methods for all new types |
| api/v1alpha1/common.go | New shared types for tiered storage configuration including MediumSource definitions |
| api/v1alpha1/cacheruntime_status.go | Status definitions for GenericCacheRuntime with component-level status tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Syspretor
force-pushed
the
feat/init-generic-cacheruntime-api
branch
from
f52dd2b to
c190d7a
Compare
|
[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 review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new generic CacheRuntime API, which is a significant and well-structured feature addition. The new API types generally follow Kubernetes conventions. I've included a few suggestions to enhance consistency and clarity in the API definitions and their accompanying documentation.
api/v1alpha1/cacheruntime_status.go
Outdated
| // ReadyReplicas is the number of pods with a Ready condition. | ||
| // +optional | ||
| ReadyReplicas int32 `json:"ready,omitempty"` | ||
|
|
||
| // CurrentReplicas is the current number of replicas running for the component | ||
| // +optional | ||
| CurrentReplicas int32 `json:"currentReplicas,omitempty"` | ||
|
|
||
| // DesiredReplicas is the desired number of replicas as specified in the spec. | ||
| // +optional | ||
| DesiredReplicas int32 `json:"desired,omitempty"` |
There was a problem hiding this comment.
The JSON tags for ReadyReplicas and DesiredReplicas are ready and desired, respectively. This is slightly inconsistent with other replica-related fields like currentReplicas and deviates from common Kubernetes API conventions (e.g., readyReplicas in DeploymentStatus). To improve clarity and maintain consistency, I recommend using readyReplicas and desiredReplicas for the JSON tags.
| // ReadyReplicas is the number of pods with a Ready condition. | |
| // +optional | |
| ReadyReplicas int32 `json:"ready,omitempty"` | |
| // CurrentReplicas is the current number of replicas running for the component | |
| // +optional | |
| CurrentReplicas int32 `json:"currentReplicas,omitempty"` | |
| // DesiredReplicas is the desired number of replicas as specified in the spec. | |
| // +optional | |
| DesiredReplicas int32 `json:"desired,omitempty"` | |
| // ReadyReplicas is the number of pods with a Ready condition. | |
| // +optional | |
| ReadyReplicas int32 `json:"readyReplicas,omitempty"` | |
| // CurrentReplicas is the current number of replicas running for the component | |
| // +optional | |
| CurrentReplicas int32 `json:"currentReplicas,omitempty"` | |
| // DesiredReplicas is the desired number of replicas as specified in the spec. | |
| // +optional | |
| DesiredReplicas int32 `json:"desiredReplicas,omitempty"` |
api/v1alpha1/cacheruntime_types.go
Outdated
| // - "OnDemand": Clean up the FUSE pod when it is no longer needed on a node | ||
| // - "OnRuntimeDeleted": Clean up the FUSE pod only when the CacheRuntime is deleted | ||
| // Defaults to "OnRuntimeDeleted". | ||
| // +kubebuilder:validation:Enum=OnRuntimeDeleted;"";OnDemand |
There was a problem hiding this comment.
The +kubebuilder:validation:Enum for CleanPolicy includes an empty string "", but the field's comment only mentions OnDemand and OnRuntimeDeleted as supported policies. This could be confusing for users.
If the empty string is a valid value with specific semantics (e.g., for defaulting), this should be documented. Otherwise, if it's not intended for user configuration, it should be removed from the enum validation to prevent confusion.
| // +kubebuilder:validation:Enum=OnRuntimeDeleted;"";OnDemand | |
| // +kubebuilder:validation:Enum=OnRuntimeDeleted;OnDemand |
| //tieredStore: | ||
| // levels: | ||
| // - quota: 8Gi # quota will add to component.container.resource.request/limit.memory | ||
| // high: "0.99" | ||
| // low: "0.99" | ||
| // mediumSource: | ||
| // processMemory: {} | ||
|
|
||
| // ProcessMemoryMediumSource describes process memory as a storage medium. | ||
| // When specified, cache data will be stored in the process's memory space, | ||
| // and the quota will be added to the container's resource requests and limits. | ||
| type ProcessMemoryMediumSource struct { | ||
| } | ||
|
|
||
| //tieredStore: | ||
| // levels: | ||
| // - quota: 8Gi | ||
| // high: "0.99" | ||
| // low: "0.99" | ||
| // path: /dev/shm | ||
| // mediumSource: | ||
| // emptyDir:{} | ||
| // ephemeral: | ||
| // volumeClaimTemplate:{} | ||
| // hostPath:{} | ||
|
|
There was a problem hiding this comment.
The commented-out YAML examples for tieredStore contain incorrect indentation and structural issues that could be misleading.
Specifically:
- In both examples, the
lowfield has incorrect indentation. - In the second example (lines 279-290):
pathandmediumSourceare incorrectly indented.- The
pathfield expects a list of strings (e.g.,["/dev/shm"]), but the example shows a single string. - The example lists
emptyDir,ephemeral, andhostPathundermediumSource, but only one can be specified at a time.
To avoid confusion, I recommend correcting these examples. Here is a revised version of the second example for reference:
// tieredStore:
// levels:
// - quota: 8Gi
// high: "0.99"
// low: "0.8"
// path: ["/dev/shm"]
// mediumSource:
// emptyDir: {}
// # Or one of the following:
// # ephemeral:
// # volumeClaimTemplate: {}
// # hostPath: {}There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/lgtm |
Signed-off-by: 玖宇 <[email protected]>
Syspretor
force-pushed
the
feat/init-generic-cacheruntime-api
branch
from
c190d7a to
2d9028a
Compare
|
New changes are detected. LGTM label has been removed. |
|
4 participants
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews