feat(seaweedfs): add storage pools support for tiered storage#2097
feat(seaweedfs): add storage pools support for tiered storage#2097
Conversation
📝 WalkthroughWalkthroughThis pull request extends the SeaweedFS Helm chart to support hierarchical pool and zone configurations. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @sircthulhu, 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 significantly enhances the SeaweedFS Helm chart by introducing robust support for tiered storage through configurable storage pools. This feature enables users to define distinct storage tiers based on disk types (e.g., SSD, HDD, NVMe), each managed by its own set of volume servers. The changes ensure seamless integration with both Simple and MultiZone cluster topologies and lay the groundwork for future COSI driver integration, allowing for more granular control over data placement and cost optimization. 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. Changelog
Activity
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
|
![gemini-code-assist[bot]](proxy.php?url=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 support for storage pools in SeaweedFS, allowing for tiered storage based on disk types. The changes are comprehensive, covering configuration, validation, resource generation for both Simple and MultiZone topologies, and integration points for COSI. The documentation has also been updated.
My review has identified a few issues:
- A potential critical issue in the usage of
cozy-lib.resources.defaultingSanitizewhich may lead to incorrect resource allocation. - Some documentation gaps regarding the defaulting behavior of pool properties in MultiZone topology.
- A minor error in the
README.mdparameter table.
Overall, this is a great feature addition. Addressing these points will improve its correctness and usability.
| {{ with $zone.replicas }} | ||
| replicas: {{ . }} | ||
| {{- end }} | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list $.Values.volume.resourcesPreset $.Values.volume.resources $) | nindent 12 }} |
There was a problem hiding this comment.
There appears to be a logic issue in the cozy-lib.resources.defaultingSanitize helper function that this PR introduces new usages of. The current implementation in cozy-lib seems to prioritize preset values over user-provided resource values due to the order of mergeOverwrite. This is contrary to the function's documentation and will lead to user-specified resources being ignored in favor of the preset.
For example, deepCopy (default (dict) $resources) | mergeOverwrite $presetMap will result in values from $presetMap overwriting values in $resources. It should be the other way around.
This will cause incorrect resource allocation for the new volume statefulsets. Please verify the implementation of mergeOverwrite and correct the merge order in cozy-lib/_resources.tpl, or implement the defaulting logic directly here to avoid using the potentially buggy helper.
packages/extra/seaweedfs/README.md
Outdated
| | `storagePools[name].resources` | Explicit CPU and memory configuration. When omitted, the preset defined in `resourcesPreset` is applied. | `object` | `{}` | | ||
| | `storagePools[name].resources.cpu` | Number of CPU cores allocated. | `quantity` | `""` | | ||
| | `storagePools[name].resources.memory` | Amount of memory allocated. | `quantity` | `""` | | ||
| | `storagePools[name].resourcesPreset` | Default sizing preset used when `resources` is omitted. Defaults to volume.resourcesPreset. | `string` | `{}` | |
There was a problem hiding this comment.
The default value for storagePools[name].resourcesPreset is documented as {}, but its type is string. This is inconsistent and may confuse users. It should be "" to indicate that it falls back to volume.resourcesPreset.
| | `storagePools[name].resourcesPreset` | Default sizing preset used when `resources` is omitted. Defaults to volume.resourcesPreset. | `string` | `{}` | | |
| | `storagePools[name].resourcesPreset` | Default sizing preset used when `resources` is omitted. Defaults to volume.resourcesPreset. | `string` | `""` | |
packages/extra/seaweedfs/values.yaml
Outdated
| ## @field {int} [replicas] - Number of volume replicas. Defaults to volume.replicas. | ||
| ## @field {quantity} [size] - Persistent Volume size. Defaults to volume.size. |
There was a problem hiding this comment.
The documentation for replicas and size within a StoragePool is incomplete for the MultiZone topology. The implementation uses a pool -> zone -> global fallback chain (e.g., {{ $pool.replicas | default $zone.replicas | default $.Values.volume.replicas }}), but the documentation only mentions defaulting to the global volume values. This should be clarified to avoid confusion.
A similar clarification is needed for size.
## @field {int} [replicas] - Number of volume replicas. In MultiZone, defaults to `volume.zones.<zoneName>.replicas`, then `volume.replicas`. In Simple topology, defaults to `volume.replicas`.
## @field {quantity} [size] - Persistent Volume size. In MultiZone, defaults to `volume.zones.<zoneName>.size`, then `volume.size`. In Simple topology, defaults to `volume.size`.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for storage pools, enabling tiered storage for SeaweedFS. The changes are comprehensive, covering Helm templates, configuration, schema, and documentation. You've added robust validation for the new configuration options, which is great for preventing misconfigurations. The implementation correctly handles both Simple and MultiZone topologies, creating separate Volume StatefulSets and corresponding WorkloadMonitor resources for each pool. The addition of BucketClass resources for COSI integration is a thoughtful preparation for future work.
I have a couple of minor suggestions. One is a documentation consistency fix in the README.md. The other points out a small pre-existing issue in the dashboard-resourcemap.yaml that becomes more apparent with the new topology-specific logic you've added. Overall, this is a solid feature addition.
packages/extra/seaweedfs/README.md
Outdated
| | `storagePools[name].resources` | Explicit CPU and memory configuration. When omitted, the preset defined in `resourcesPreset` is applied. | `object` | `{}` | | ||
| | `storagePools[name].resources.cpu` | Number of CPU cores allocated. | `quantity` | `""` | | ||
| | `storagePools[name].resources.memory` | Amount of memory allocated. | `quantity` | `""` | | ||
| | `storagePools[name].resourcesPreset` | Default sizing preset used when `resources` is omitted. Defaults to volume.resourcesPreset. | `string` | `{}` | |
There was a problem hiding this comment.
The default Value for storagePools[name].resourcesPreset is documented as {}, but its Type is string. This is inconsistent with other string-typed parameters and likely a copy-paste error from the resources field which is an object. For other optional string parameters that default to another value, the Value is typically "". Please update for consistency.
| | `storagePools[name].resourcesPreset` | Default sizing preset used when `resources` is omitted. Defaults to volume.resourcesPreset. | `string` | `{}` | | |
| | `storagePools[name].resourcesPreset` | Default sizing preset used when `resources` is omitted. Defaults to volume.resourcesPreset. | `string` | `""` | |
| {{- if eq .Values.topology "Simple" }} | ||
| {{- range $poolName, $pool := .Values.storagePools }} | ||
| - {{ $.Release.Name }}-volume-{{ $poolName }} | ||
| {{- end }} | ||
| {{- else if eq .Values.topology "MultiZone" }} | ||
| {{- range $zoneName, $zone := .Values.volume.zones }} | ||
| - {{ $.Release.Name }}-volume-{{ $zoneName }} | ||
| {{- end }} | ||
| {{- range $poolName, $pool := .Values.storagePools }} | ||
| {{- range $zoneName, $zone := $.Values.volume.zones }} | ||
| - {{ $.Release.Name }}-volume-{{ $zoneName }}-{{ $poolName }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
The logic to add WorkloadMonitor resource names for different topologies is a good addition. However, I've noticed a related issue on line 28 (an unchanged context line): {{ $.Release.Name }}-volume is included unconditionally. The corresponding WorkloadMonitor resource is only created when .Values.topology is Simple. In a MultiZone deployment, this will cause the Role to reference a non-existent resource. To fix this, - {{ $.Release.Name }}-volume should be moved from line 28 to inside this new if eq .Values.topology "Simple" block (e.g., before line 32).
Add optional storagePools configuration that creates separate Volume StatefulSets per disk type (SSD/HDD/NVMe), enabling tiered storage within a single SeaweedFS tenant. Each pool gets its own BucketClass and BucketAccessClass to prepare infrastructure for COSI driver integration. Supported in both Simple and MultiZone topologies: - Simple: one StatefulSet per pool - MultiZone: one StatefulSet per zone×pool combination Also adds volume.diskType field for tagging default volume servers. Co-Authored-By: Claude <[email protected]> Signed-off-by: Kirill Ilin <[email protected]>
…s inheritance Make storageClass optional in storagePools — pools inherit from volume.storageClass when not explicitly set. Add full COSI resource set per storage pool: BucketClass, BucketClass-worm (Retain + object lock), BucketAccessClass readwrite, and BucketAccessClass readonly. Co-Authored-By: Claude <[email protected]> Signed-off-by: Kirill Ilin <[email protected]>
- Document MultiZone fallback chain for pool replicas and size - Move `-volume` WorkloadMonitor reference inside Simple topology block in dashboard-resourcemap.yaml (it is only created for Simple) Co-Authored-By: Claude <[email protected]> Signed-off-by: Kirill Ilin <[email protected]>
- Remove dots from pool name regex (K8s resources don't allow dots) - Add zone×pool name collision validation for MultiZone topology - Use conditional storageClass rendering to omit empty values - Fix README resourcesPreset default value Co-Authored-By: Claude <[email protected]> Signed-off-by: Kirill Ilin <[email protected]>
…s and zone.pools Redesign storage pools architecture: - Move storagePools map from top-level into volume.pools (Simple topology) and volume.zones[name].pools (MultiZone topology) - Add nodeSelector, storageClass, dataCenter overrides for zones - Add reserved suffix validation (-worm, -readonly) for pool names - Block volume.pools usage in MultiZone (must use zone.pools instead) - Use ternary/hasKey pattern for all optional replicas to handle 0 correctly - Fix nodeSelector rendering for multiline values using indent - Use disk: parameter (not diskType:) for COSI driver v0.3.0 BucketClass - Bump seaweedfs-cosi-driver tag to v0.3.0 - Add minimum: 1 constraint for volume/zone/pool replicas in schema - Regenerate README, CRD, and openAPISchema Co-Authored-By: Claude <[email protected]> Signed-off-by: Kirill Ilin <[email protected]>
sircthulhu
force-pushed
the
feat/seaweedfs-storage-pools
branch
from
f8fa302 to
b0b0dad
Compare
sircthulhu
requested review from
IvanHunters,
androndo,
kvaps,
lexfrei and
lllamnyp
as code owners
dosubot
bot
added
size:XL
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/extra/seaweedfs/templates/seaweedfs.yaml`:
- Around line 71-75: The template only checks $composedName against existing
zone keys ($.Values.volume.zones) but doesn't ensure uniqueness among all
composed "{zone}-{pool}" names; update the Helm template to build a set/map of
composed names while iterating zones and pools and fail when a composed name is
already present to prevent collisions (reference the $composedName variable, the
zoneName and poolName loop variables, and $.Values.volume.zones); in practice,
create a composedNames map before/while generating names, use hasKey on that map
to detect duplicates and call fail with a clear message when a duplicate
composedName is found, otherwise add the composedName to the map.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/extra/seaweedfs/README.mdpackages/extra/seaweedfs/images/seaweedfs-cosi-driver.tagpackages/extra/seaweedfs/templates/dashboard-resourcemap.yamlpackages/extra/seaweedfs/templates/seaweedfs.yamlpackages/extra/seaweedfs/templates/storage-pool-bucket-classes.yamlpackages/extra/seaweedfs/values.schema.jsonpackages/extra/seaweedfs/values.yamlpackages/system/seaweedfs-rd/cozyrds/seaweedfs.yaml
| {{- $composedName := printf "%s-%s" $zoneName $poolName }} | ||
| {{- if hasKey $.Values.volume.zones $composedName }} | ||
| {{- fail (printf "Composed volume name '%s' (from zone '%s' and pool '%s') collides with an existing zone name." $composedName $zoneName $poolName) }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Add uniqueness validation for composed zone-pool names.
Current logic checks collision against existing zone names, but not collisions between different {zone, pool} pairs (e.g., a+b-c vs a-b+c). That can create duplicate generated keys/names and unstable manifests.
💡 Proposed fix
{{- if eq .Values.topology "MultiZone" }}
+{{- $seenComposedPoolNames := dict }}
{{- range $zoneName, $zone := .Values.volume.zones }}
{{- range $poolName, $pool := (dig "pools" dict $zone) }}
@@
{{- $_ := set $allPools $poolName $pool.diskType }}
{{- $composedName := printf "%s-%s" $zoneName $poolName }}
+{{- if hasKey $seenComposedPoolNames $composedName }}
+{{- fail (printf "Composed volume name '%s' is ambiguous between '%s' and '%s/%s'." $composedName (get $seenComposedPoolNames $composedName) $zoneName $poolName) }}
+{{- end }}
+{{- $_ := set $seenComposedPoolNames $composedName (printf "%s/%s" $zoneName $poolName) }}
{{- if hasKey $.Values.volume.zones $composedName }}
{{- fail (printf "Composed volume name '%s' (from zone '%s' and pool '%s') collides with an existing zone name." $composedName $zoneName $poolName) }}
{{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/extra/seaweedfs/templates/seaweedfs.yaml` around lines 71 - 75, The
template only checks $composedName against existing zone keys
($.Values.volume.zones) but doesn't ensure uniqueness among all composed
"{zone}-{pool}" names; update the Helm template to build a set/map of composed
names while iterating zones and pools and fail when a composed name is already
present to prevent collisions (reference the $composedName variable, the
zoneName and poolName loop variables, and $.Values.volume.zones); in practice,
create a composedNames map before/while generating names, use hasKey on that map
to detect duplicates and call fail with a clear message when a duplicate
composedName is found, otherwise add the composedName to the map.
3 participants
What this PR does
volume.pools(Simple topology) andvolume.zones[name].pools(MultiZone topology) for creating separate Volume StatefulSets per disk type (SSD/HDD/NVMe)nodeSelector,storageClass, anddataCenteroverrides for zones in MultiZone topologyBucketClassandBucketAccessClassCOSI resources (including WORM and readonly variants)diskparameter support in BucketClass)volume.diskTypefield to tag default volume servers with a disk typeHow It Works
Simple Topology
Each storage pool in
volume.poolscreates an additional Volume StatefulSet alongside the default one. All pods (default + pool) may run on the same nodes. SeaweedFS distinguishes storage via the-disk=<type>flag on volume servers.MultiZone Topology
Pools are defined per-zone in
volume.zones[name].pools. A StatefulSet is created for each zone × pool combination (e.g.,us-east-ssd,us-west-ssd), inheriting nodeSelector and dataCenter from its parent zone.In Simple topology,
volume.poolsis used. In MultiZone,volume.zones[name].poolsis used —volume.poolsis explicitly blocked to prevent BucketClasses without backing StatefulSets.Zone Overrides (MultiZone)
Zones now support:
nodeSelector— YAML string, defaults totopology.kubernetes.io/zone: <zoneName>storageClass— defaults tovolume.storageClassdataCenter— SeaweedFS data center name, defaults to zone nameCOSI Resources
Each unique pool name generates 4 cluster-scoped COSI resources:
<namespace>-<pool>BucketClass (Delete policy,disk: <type>)<namespace>-<pool>-wormBucketClass (Retain policy, object lock)<namespace>-<pool>BucketAccessClass (readwrite)<namespace>-<pool>-readonlyBucketAccessClass (readonly)Validation
-wormor-readonly(reserved COSI suffixes)diskTypeis required and must be lowercase alphanumericdiskTypemust differ fromvolume.diskTypevolume.poolsis blocked in Client and MultiZone topologiesminimum: 1in JSON schemaInheritance Chain
replicassizestorageClassresourcesBackward Compatibility
volume.pools: {}produces identical output to current chartvolume.diskType: ""adds no extra flagsTest plan
helm templatewith empty pools — output identical to currenthelm templatewith Simple + volume.pools — additional volume StatefulSets, BucketClasses, WorkloadMonitorshelm templatewith MultiZone + zone.pools — zone × pool cross-product StatefulSetshelm templatewithvolume.diskType: hdd— extraArgs includes-disk=hddhelm templatewith Client + volume.pools — fails with validation errorhelm templatewith MultiZone + volume.pools — fails with validation errorhelm templatewith reserved pool name suffix — fails with validationRelease note
Summary by CodeRabbit
New Features
Documentation
Chores