feat(manifests): Support high availability for the controller manager in the Helm chart by myselfkunal · Pull Request #4147 · kubeflow/trainer

myselfkunal · 2026-10-01T05:35:30Z

Summary

Adds three opt-in manager values so the controller manager can run highly available, and documents a recommended anti-affinity in the chart README:

manager.podDisruptionBudget: disabled by default. Recommended with manager.replicas of 2 or more, since a PDB on a single replica blocks node drains. The chart does not reject any combination; Kubernetes validates the PDB itself.
manager.topologySpreadConstraints: empty by default, for stricter node or zone spreading.
manager.priorityClassName: empty by default. The chart never creates a PriorityClass.

manager.affinity is unchanged and stays {}. Instead, the new "High availability" section in the chart README shows a soft (preferred) pod anti-affinity scoped to the release's selector labels (app.kubernetes.io/name, app.kubernetes.io/instance, app.kubernetes.io/component).

Why

Leader election already supports multiple replicas, but the webhooks default to failurePolicy: Fail. Any moment with zero Ready manager pods rejects every TrainJob/TrainingRuntime create and update. That happens during a routine kubectl drain, an autoscaler scale-down, or GPU cluster preemption, since the manager runs at priority 0 today. These values give operators a documented way to avoid it.

Backwards compatibility

No change at default values. I rendered the chart from upstream/master and from this branch with identical default values, then diffed the two. Both outputs are 32,738 lines, and diff reports no differences.

podDisruptionBudget, topologySpreadConstraints, and priorityClassName only render when set, and affinity behaves exactly as on master.

Testing

helm-unittest: 107/107 passing across 23 suites, including a new Test manager PodDisruptionBudget suite covering the PDB not rendering by default or when disabled, rendering when enabled, and a custom minAvailable. Test manager Deployment covers affinity not rendering by default, and render-only-when-set for topologySpreadConstraints and priorityClassName.

helm lint: clean at defaults.

kind cluster (3 nodes: 1 control-plane, 2 workers), manager.replicas=2. This ran on an earlier revision of this PR, where a soft anti-affinity was the default. It is the same rule as the README example, with a part-of/component selector instead of the release-scoped one.

Both pods Ready and on different nodes.
Leader election held by a single pod, no flapping.
Compared the webhook caBundle against the cert Secret. SHA-256 fingerprints matched across the Secret, both webhook configs, and both pods' mounted cert files, immediately after install and again about 4.5 minutes later.
Applied a TrainJob from examples/yaml/01-multi-node.yaml. It admitted cleanly and the mutating webhook defaulted managedBy correctly.
Deleted the leader pod and immediately applied a second TrainJob. It admitted with no gap, and failover was clean with a single transition. The replacement pod logs "no cert refresh needed" from the cert-rotation logger, so it adopts the existing cert from the Secret rather than generating its own. That is why this does not race.
Grepped both pods' full logs for cert, webhook, TLS and error keywords. No warnings, no errors.

Rebased onto master at b514f69f. Tests and lint re-run clean after the rebase and the review follow-ups.

Review follow-ups

Affinity default dropped. Per @robert-bell, there is no default affinity; the recommended block is documented in the README instead.
PDB check removed. Per @robert-bell and @andreyvelich, the chart no longer fails when manager.replicas < 2. The README and values.yaml recommend 2 or more replicas instead.

Fixes #4083

Checklist:

Docs included if any changes are user facing

google-oss-prow · 2026-10-01T05:35:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gaocegege for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andreyvelich

/ok-to-test

andreyvelich

Thanks @myselfkunal for this PR! Note: this review was done with the help of AI tools.

andreyvelich · 2026-10-06T17:45:06Z

+          matchLabels:
+            app.kubernetes.io/part-of: kubeflow
+            app.kubernetes.io/component: manager


Use the chart's manager selector labels so the anti-affinity only matches this release's manager pods (it currently also matches other Kubeflow component: manager pods), and keeps working when manager.selectorLabels is set. Please update the expected labels in deployment_test.yaml to match.

Suggested change

matchLabels:

app.kubernetes.io/part-of: kubeflow

app.kubernetes.io/component: manager

matchLabels:

{{- include "trainer.manager.selectorLabels" . | nindent 12 }}

Agreed, will apply this along with the expected labels in deployment_test.yaml. Holding the push until we settle the default location in the deployment.yaml thread, since that decides whether this stays in the helper.

andreyvelich · 2026-10-06T17:45:07Z

        {{- toYaml . | nindent 8 }}
      {{- end }}
-      {{- with .Values.manager.affinity }}
+      {{- if .Values.manager.affinity }}


Neither affinity: {} nor affinity: null removes the default, so users can't opt out (e.g. if they want to rely only on topologySpreadConstraints). Can you document how to disable it, or add a toggle with a test?

@myselfkunal we can put the default in the values.yaml rather than in the helper.

Please update the inline doc - e.g.

# -- Affinity for manager pods. Defaults to soft pod anti-affinity on node. # Set to null to disable the default affinity rules.

@andreyvelich @robert-bell thanks for the review. Before I push, I want to check one thing, since the two suggestions pull in different directions.

Moving the default into values.yaml makes null a clean opt-out, since Helm drops the key and if .Values.manager.affinity becomes false. But values.yaml cannot run templates, so the anti-affinity can no longer use trainer.manager.selectorLabels. The selector would go back to part-of: kubeflow + component: manager, which also matches other Kubeflow managers. A user-supplied affinity would also be deep-merged with the default instead of replacing it. For example, setting only nodeAffinity keeps the soft anti-affinity.

Two options:

Default in values.yaml, rendered with tpl so the selector can use release-scoped labels (e.g. app.kubernetes.io/instance: "{{ .Release.Name }}"). Setting null disables it.

Keep the default in the helper with trainer.manager.selectorLabels, and add manager.defaultAffinity.enabled (default true) as the opt-out, with a unit test.

I lean towards option 2, since only the helper can use trainer.manager.selectorLabels (including user-set manager.selectorLabels), which Andrey asked for in the _helpers.tpl thread. Happy to go with option 1 if you prefer keeping the default in values.yaml. WDYT?

Ah good point about needing to template the default affinity.

In which case I'd lean towards not providing any default and instead just documenting the HA antiaffinity in the readme.

Both a default and an additional configuration option introduce some maintenance and complexity for a use case that users can easily configure themselves. Wdyt?

@robert-bell makes sense, agreed. It also means default installs render exactly the same as master again.

Plan:

Drop trainer.manager.defaultAffinity and keep manager.affinity: {} as on master.

Add a README section with a recommended soft pod anti-affinity for HA, using the release's selector labels (app.kubernetes.io/name, app.kubernetes.io/instance, app.kubernetes.io/component: manager).

Keep podDisruptionBudget, topologySpreadConstraints and priorityClassName as they are.

@andreyvelich this also covers the opt-out and selector label threads, since there is no default left to disable. Let me know if you would still prefer a default, otherwise I'll push this.

andreyvelich · 2026-10-06T17:45:07Z

+  # -- Affinity for manager pods. Empty by default, which falls back to a soft (preferred)
+  # pod anti-affinity that spreads manager replicas across different nodes by hostname
+  # (see `trainer.manager.defaultAffinity` in templates/manager/_helpers.tpl). This is a
+  # scheduling preference only: it never blocks a pod from being scheduled, including on a
+  # single-node cluster. Setting any value here fully replaces the default (it is not
+  # merged with it), so your own affinity is used exactly as given.


Can we keep this short, since helm-docs puts it in the README table?

Suggested change

# -- Affinity for manager pods. Empty by default, which falls back to a soft (preferred)

# pod anti-affinity that spreads manager replicas across different nodes by hostname

# (see `trainer.manager.defaultAffinity` in templates/manager/_helpers.tpl). This is a

# scheduling preference only: it never blocks a pod from being scheduled, including on a

# single-node cluster. Setting any value here fully replaces the default (it is not

# merged with it), so your own affinity is used exactly as given.

# -- Affinity for manager pods. If empty, defaults to a soft pod anti-affinity that spreads

# manager replicas across nodes. Setting a value fully replaces the default.

Will do. I'll shorten it once we settle the default location in the deployment.yaml thread, since the opt-out line depends on that (null vs a defaultAffinity.enabled toggle).

andreyvelich · 2026-10-06T17:46:08Z

/cc @robert-bell @Sridhar1030 @tariq-hasan appreciate your quick look too if you can!

… in the Helm chart Add four opt-in `manager` values so the controller manager can run highly available: podDisruptionBudget (disabled by default, fails the template if enabled with replicas < 2), topologySpreadConstraints (empty by default), priorityClassName (empty by default, no PriorityClass is created), and a soft preferred podAntiAffinity shipped as the new default for affinity. Leader election already allows multiple replicas, but the webhooks default to failurePolicy: Fail, so any gap with zero Ready manager pods rejects every TrainJob/TrainingRuntime create and update. These values close that gap without changing behavior for existing installs: `helm template` on this branch vs. upstream/master, same values, differs only in the new affinity default, which is a soft scheduling preference and never blocks a pod from being scheduled. Verified webhook cert management holds up at replicas=2 on a 3-node kind cluster, including after force-deleting the leader pod mid-flight: caBundle stayed in sync with the cert Secret throughout, and TrainJob admission had no gap. Fixes kubeflow#4083 Signed-off-by: myselfkunal <kunalshawip@gmail.com>

Signed-off-by: myselfkunal <kunalshawip@gmail.com>

…ti-affinity Signed-off-by: myselfkunal <kunalshawip@gmail.com>

robert-bell · 2026-10-21T06:11:09Z

Thanks @myselfkunal! Could you address #4147 (comment) please?

…Budget Signed-off-by: myselfkunal <kunalshawip@gmail.com>

myselfkunal · 2026-10-21T20:56:49Z

The CPU E2E Test (1.37.0) job failed during dependency install with a network error (Connection reset by peer while fetching kubeflow/trainer.git). The other 6 E2E jobs pass on the same commit. Could a maintainer please re-run the failed job when convenient?

robert-bell · 2026-10-22T18:26:13Z

Thanks @myselfkunal! You can rerun tests using a comment

/retest

github-project-automation Bot added this to Kubeflow Trainer Oct 1, 2026

github-project-automation Bot moved this to Needs Triage in Kubeflow Trainer Oct 1, 2026

google-oss-prow Bot requested review from jinchihe and robert-bell October 1, 2026 05:35

google-oss-prow Bot added the size/L label Oct 1, 2026

andreyvelich reviewed Oct 6, 2026

View reviewed changes

google-oss-prow Bot added the ok-to-test label Oct 6, 2026

andreyvelich reviewed Oct 6, 2026

View reviewed changes

myselfkunal force-pushed the feat/helm-manager-ha branch from c28b60e to 7653e8e Compare October 14, 2026 18:50

myselfkunal requested a review from andreyvelich October 14, 2026 20:08

myselfkunal added 3 commits October 21, 2026 06:10

chore(manifests): use year-less copyright header in new files

dd23c76

Signed-off-by: myselfkunal <kunalshawip@gmail.com>

refactor(manifests): drop default manager affinity and document HA an…

22750e6

…ti-affinity Signed-off-by: myselfkunal <kunalshawip@gmail.com>

myselfkunal force-pushed the feat/helm-manager-ha branch from 7653e8e to 22750e6 Compare October 21, 2026 01:03

refactor(manifests): remove replicas check from manager PodDisruption…

928f760

…Budget Signed-off-by: myselfkunal <kunalshawip@gmail.com>

Conversation

myselfkunal commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Backwards compatibility

Testing

Review follow-ups

Uh oh!

google-oss-prow Bot commented Oct 1, 2026

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

andreyvelich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myselfkunal Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreyvelich commented Oct 6, 2026

Uh oh!

robert-bell commented Oct 21, 2026

Uh oh!

myselfkunal commented Oct 21, 2026

Uh oh!

robert-bell commented Oct 22, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

myselfkunal commented Oct 1, 2026 •

edited

Loading

myselfkunal Oct 14, 2026 •

edited

Loading