Skip to content

[CELEBORN-2446] Support topologySpreadConstraints for master and worker pods in Helm chart - #3831

Open
strelok89 wants to merge 1 commit into
apache:mainfrom
strelok89:helm-topology-spread-constraints
Open

[CELEBORN-2446] Support topologySpreadConstraints for master and worker pods in Helm chart#3831
strelok89 wants to merge 1 commit into
apache:mainfrom
strelok89:helm-topology-spread-constraints

Conversation

@strelok89

@strelok89 strelok89 commented Aug 29, 2026

Copy link
Copy Markdown

What changes were proposed in this pull request?

Add support for topologySpreadConstraints on Celeborn master and worker pods in the Helm chart:

  • charts/celeborn/templates/master/statefulset.yaml and charts/celeborn/templates/worker/statefulset.yaml — render topologySpreadConstraints from the corresponding value, following the existing {{- with }} idiom already used for nodeSelector, affinity and tolerations.
  • charts/celeborn/values.yaml — declare topologySpreadConstraints: [] for both roles.
  • charts/celeborn/tests/{master,worker}/statefulset_test.yaml — unit tests covering both the unset (field absent) and set (field rendered) cases.

Example usage:

worker:
  topologySpreadConstraints:
    - maxSkew: 1
      topologyKey: topology.kubernetes.io/zone
      whenUnsatisfiable: DoNotSchedule
      labelSelector:
        matchLabels:
          app.kubernetes.io/name: celeborn
          app.kubernetes.io/role: worker

Why are the changes needed?

The chart already exposes nodeSelector, affinity, tolerations and priorityClass for both master and worker pods, but not topologySpreadConstraints. Setting the value today is silently ignored — Helm accepts worker.topologySpreadConstraints without error and nothing is rendered on the StatefulSet, so the constraint simply does not exist.

podAntiAffinity — which the chart defaults to, on kubernetes.io/hostname — cannot substitute for a multi-AZ deployment. Pod anti-affinity is one-or-none per topology domain: it can express "at most one pod per zone", but not "N pods per zone" or "spread evenly across zones within a bounded skew". That is what topologySpreadConstraints is for.

This also lines up with item 8, "Enhanced K8S support", on the improvements schedule in CONTRIBUTING.md.

Does this PR resolve a correctness bug?

  • Yes

Does this PR introduce any user-facing change?

  • Yes

Two new chart values, master.topologySpreadConstraints and worker.topologySpreadConstraints, both defaulting to []. When they are unset the rendered StatefulSet is unchanged, so this is purely additive.

How was this patch tested?

  • helm unittest charts/celeborn --file "tests/**/*_test.yaml" --strict — all chart unit tests pass, including the four new cases. Reverting only the template hunks makes exactly those new assertions fail, confirming they exercise the change.
  • helm lint charts/celeborn — passes.
  • helm template with a zone-spread constraint set renders topologySpreadConstraints on the worker StatefulSet; helm template with default values renders no topologySpreadConstraints key anywhere.

…er pods in Helm chart

The Helm chart exposes nodeSelector, affinity, tolerations and priorityClass
for both master and worker pods, but not topologySpreadConstraints. Setting
the value is silently ignored: Helm accepts it and nothing is rendered on the
StatefulSet.

podAntiAffinity cannot substitute for a multi-AZ deployment, because it is
one-or-none per topology domain and cannot express "N pods per zone".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@strelok89 strelok89 changed the title [CELEBORN-2446] Support topologySpreadConstraints for master and work… [CELEBORN-2446] Support topologySpreadConstraints for master and worker pods in Helm chart Aug 29, 2026
@strelok89

Copy link
Copy Markdown
Author

@SteNicholas pinging you for review, its small change but have lots of value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant