Repository navigation
fix(seaweedfs): soften master anti-affinity for small clusters - #3197
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Helm values override for SeaweedFS master pod anti-affinity, changing it from a hard hostname requirement to a weighted preference, and adds a Helm test that checks the rendered master StatefulSet uses the new preference. ChangesSeaweedFS Master Anti-Affinity Update
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
github-actions
Bot
added
area/storage
Summary of ChangesHello, 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 addresses a scheduling issue where the SeaweedFS master component would fail to deploy on clusters with fewer than three nodes due to strict anti-affinity rules. By softening these rules to a strong preference, the master pods can now be scheduled on smaller clusters while still maintaining node-level distribution on larger ones. This change ensures that installation processes like 'helm install --wait' complete successfully in resource-constrained environments. Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. You can react with 👍 and 👎 on Gemini (@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. Footnotes
|
![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 softens the SeaweedFS master hostname anti-affinity from a hard requirement to a preferred scheduling preference with a weight of 100, allowing the master StatefulSet to be scheduled on clusters with fewer than three nodes. It also adds a test suite to verify this behavior. The reviewer suggests wrapping the Helm template expressions in single quotes within the new affinity block to ensure consistency and prevent potential YAML parsing issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| app.kubernetes.io/name: {{ template "seaweedfs.name" . }} | ||
| app.kubernetes.io/instance: {{ .Release.Name }} |
There was a problem hiding this comment.
For consistency with the s3 affinity block below (lines 193 and 211) and to prevent any potential YAML parsing issues, it is recommended to wrap the template expressions in single quotes.
app.kubernetes.io/name: '{{ template "seaweedfs.name" . }}'
app.kubernetes.io/instance: '{{ .Release.Name }}'
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Correct root-cause fix via the idiomatic parent-values.yaml override; verified by rendering the master StatefulSet before/after and running the full helm-unittest suite (18/18 pass). One accepted trade-off worth recording.
Caveats
- Upgrade impact (existing multi-node clusters): the change edits
packages/system/seaweedfs/values.yaml:59-69, which feedsspec.template.spec.affinityon the master StatefulSet (charts/seaweedfs/templates/master/master-statefulset.yaml:55-57).spec.templateis a mutable StatefulSet field, so this is a legal Update, not an immutable-field trap — it triggers a rolling restart of the master pods only. Master data isemptyDir(values.yaml:31-32), so the rolling restart carries no data-loss risk. Verified by diffing the rendered STS betweenmainand HEAD: the only delta isrequiredDuringSchedulingIgnoredDuringExecution->preferredDuringSchedulingIgnoredDuringExecution(weight 100), samematchLabelsselector and sametopologyKey: kubernetes.io/hostname. No other churn. - Fresh-install impact: this is the scenario the PR targets. No new
PackageSource.dependsOn, no cozyrds/RBAC change (the ApplicationDefinition lives in the untouchedpackages/system/seaweedfs-rd/cozyrds/seaweedfs.yaml), novalues.schema.jsonin this system package (so nomake generateobligation), no migration and nomigrations.targetVersionbump required. The subchart consumes the override viatpl .Values.master.affinityand replaces the upstream default verbatim (string value, not a deep-merge), so the hard term is fully dropped — confirmed in the render. - Fault-tolerance trade-off (raft quorum): softening the hostname anti-affinity from
requiredtopreferredmeans that on a node under scheduling pressure, two or more of the three master pods may co-locate on one node. On a healthy multi-node cluster the weight-100 preference keeps them spread, but there is no longer a hard guarantee — a single node failure could take down 2 of 3 masters and lose raft quorum. This is the intended cost of enabling <3-node installs and mirrors the posture already applied to the s3 component (values.yaml:183-234), so it is an accepted trade-off, not a blocker. The code comment (values.yaml:48-58) documents the motivation but does not name this specific quorum consequence — worth a sentence if you want future maintainers to see the trade-off without re-deriving it. - Not a vendored-chart edit: the change lives entirely in the package's own
values.yaml; nothing underpackages/system/seaweedfs/charts/is modified, so the "silent revert onmake update" concern and thepatches/*.diffrequirement do not apply. Verified against theMakefileupdatetarget.
Recommended follow-ups
- Optional: add one line to the
values.yaml:48-58comment (or the test header) naming the raft-quorum consequence of co-location, so the accepted trade-off is legible at the point of change rather than only in this PR discussion.
The vendored subchart pins the master to a hard hostname anti-affinity (requiredDuringSchedulingIgnoredDuringExecution). With the master StatefulSet at three replicas, the surplus Pods stay Pending forever on clusters with fewer than three nodes, so `helm install --wait` never returns and the S3 stack never comes up. Soften the term to a strong preference (weight 100) so the masters still spread across distinct nodes when the cluster has them, while single- and dual-node clusters can bring the master up. Mirrors the posture already used for the s3 component. Selector and topologyKey are unchanged; only required -> preferred. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
ec4bbf4 to
9ab046e
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM
Softening the master anti-affinity from required to preferred (weight 100) is
safe on both small and large clusters, verified against source:
- Master metadata is on
emptyDirand rebuilt from volume servers on restart, so
co-location adds no data-durability risk beyond the already-accepted availability
trade-off; large clusters keep spreading as before. spec.template.spec.affinityis a mutable StatefulSet field: the upgrade applies
in place as a normal rolling restart, no immutability trap, no PVC/data changes.helm templateconfirms the hard term is gone and exactly one preferred term
remains (weight 100,topologyKey: kubernetes.io/hostname, correct selector).helm unittestis green (18/18), including the newmaster_affinity_test.yaml
that guards against the hard term reappearing on a subchart bump.
Nit (non-blocking): the commit body says "same posture already used for the s3
component" — s3 actually also adds a zone anti-affinity term and uses weight 10 for
the hostname term, so the parallel is only rough. No code change needed.
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — verified locally: the overlay renders the master podAntiAffinity as a weighted preference (weight 100, kubernetes.io/hostname, component: master), the subchart default is confirmed a hard required term, and the new helm-unittest passes as part of the package suite (helm unittest . → 7 suites / 18 tests). HA isn't regressed — weight-100 preferred still spreads masters on 3+ node clusters and only co-locates when forced, which is exactly the small-cluster case where 3-way HA is impossible anyway.
One non-blocking note: the description/comment frame this as fixing "clusters with fewer than three nodes," but only the master is softened. The volume and filer components (2 replicas each) still carry hard hostname anti-affinity, so this actually unblocks the two-node case — a single-node install would still hang on those. Worth either narrowing the wording to two-node, or softening volume/filer too if single-node S3 is a real target. Doesn't block merge.
3 participants
What this PR does
Overrides the seaweedfs master anti-affinity in the package values from a hard
requiredDuringSchedulingIgnoredDuringExecutionhostname term to a strong preference (preferredDuringSchedulingIgnoredDuringExecution, weight 100). The selector and topologyKey are unchanged.The vendored subchart pins the master to a hard hostname anti-affinity — its own comment notes that emptying it is what allows single-node deployment. With the master StatefulSet at three replicas, the surplus master Pods stay Pending forever on clusters with fewer than three nodes, so
helm install --waitnever returns and the S3 stack never comes up.Softening to a strong preference keeps the masters spread across distinct nodes whenever the cluster has them (the scheduler only co-locates when there is no alternative — exactly the small-cluster case), while letting single- and dual-node clusters bring the master up. This mirrors the posture already applied to the s3 component in the same values file.
A helm-unittest pins the softened term (asserts the hard term is gone and the preferred term carries the right weight, topologyKey, and component selector) so a future subchart bump cannot silently re-harden it.
Screenshots
N/A — no UI changes.
Release note
Summary by CodeRabbit