add PDB and topologySpreadConstraints for multi replica deployments#35
Open
likesavabutworse wants to merge 3 commits intokafbat:mainfrom
Open
add PDB and topologySpreadConstraints for multi replica deployments#35likesavabutworse wants to merge 3 commits intokafbat:mainfrom
likesavabutworse wants to merge 3 commits intokafbat:mainfrom
Conversation
jcpunk
reviewed
Jan 7, 2026
Contributor
|
It might be worthwhile to add the trailing newlines to the end of the files. |
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds topologySpreadConstraints and an optional PodDisruptionBudget to the Kafka UI Helm chart, updates default values for scheduling/disruption controls and env mappings, and bumps the chart version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
charts/kafka-ui/values.yaml (2)
281-286: Consider documentingminDomainsimpact on single-zone clusters.With
minDomains: 2andwhenUnsatisfiable: DoNotSchedule, enabling topology spread constraints on a single-zone cluster will cause pod scheduling failures. Users might enable this feature without realizing their cluster doesn't meet the minimum domain requirement.Consider either:
- Defaulting
minDomainsto1(safer for varied cluster topologies), or- Adding a more prominent warning in the comment
📝 Suggested documentation improvement
# Topology Spread Constraints: https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/ +# WARNING: minDomains: 2 requires at least 2 availability zones. Pods will fail to schedule on single-zone clusters. topologySpreadConstraints: enabled: false maxSkew: 1 topologyKey: topology.kubernetes.io/zone - minDomains: 2 + minDomains: 2 # Set to 1 for single-zone clusters whenUnsatisfiable: DoNotSchedule # Must be DoNotSchedule if minDomains > 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/values.yaml` around lines 281 - 286, The topologySpreadConstraints block currently sets minDomains: 2 and whenUnsatisfiable: DoNotSchedule which will prevent pods from scheduling in single-zone clusters; update the chart by either lowering topologySpreadConstraints.minDomains to 1 (safer default) or add a prominent comment/warning next to topologySpreadConstraints.minDomains and whenUnsatisfiable explaining that enabling topologySpreadConstraints with minDomains > 1 requires multi-zone clusters and will cause scheduling failures on single-zone clusters so operators must adjust these values accordingly.
286-286: Add trailing newline at end of file.The file is missing a trailing newline, which was noted in the PR comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/values.yaml` at line 286, The file is missing a trailing newline at EOF; add a single newline character after the last line containing "whenUnsatisfiable: DoNotSchedule # Must be DoNotSchedule if minDomains > 1" so the file ends with a newline to satisfy POSIX/newline conventions and the repository's linter/CI checks.charts/kafka-ui/templates/deployment.yaml (1)
195-206: Topology spread constraints implementation looks correct.The implementation properly:
- Uses the shared
kafka-ui.selectorLabelshelper for consistency with the Deployment selector and PDB- Places
matchLabelKeysat the correct level (sibling tolabelSelector, not nested within it)- Uses
pod-template-hashinmatchLabelKeysto ensure spread is evaluated per-revision during rolling updatesOne minor issue: the file is missing a trailing newline at the end, and line 206 has trailing whitespace after
{{- end }}.📝 Proposed fix for trailing whitespace and newline
matchLabelKeys: - pod-template-hash - {{- end }} + {{- end }} +,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/templates/deployment.yaml` around lines 195 - 206, Remove the trailing whitespace after the closing template tag and ensure the file ends with a single newline: update the deployment.yaml area around the topologySpreadConstraints block (the section using .Values.topologySpreadConstraints and the include "kafka-ui.selectorLabels") so that the final "{{- end }}" has no trailing spaces and add a final newline character at EOF.charts/kafka-ui/templates/pod-disruption-budget.yaml (2)
11-11: Add trailing newline at end of file.The file is missing a trailing newline, which is a POSIX convention and was noted in the PR comments.
📝 Proposed fix
{{- include "kafka-ui.selectorLabels" . | nindent 6 }} -{{- end -}} +{{- end -}} +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/templates/pod-disruption-budget.yaml` at line 11, The file ends with the Helm template token "{{- end -}}" but lacks a POSIX trailing newline; update the template (the file containing the "{{- end -}}" token) to add a single newline character at the end of the file so it ends with '\n' (LF).
6-10: Potential selector conflict if user specifiesselectorinspec.The template renders
.Values.podDisruptionBudget.specviatoYamlon line 7, then hardcodes theselectoron lines 8-10. If a user accidentally adds aselectorfield within theirpodDisruptionBudget.specvalues, the resulting YAML will have duplicateselectorkeys, leading to undefined behavior or the hardcoded selector being ignored.Consider either:
- Documenting that
selectorshould not be provided inspec, or- Restructuring to only allow
minAvailable/maxUnavailablefields explicitly:♻️ Suggested restructure (optional)
spec: - {{- toYaml .Values.podDisruptionBudget.spec | nindent 2 }} + {{- with .Values.podDisruptionBudget.minAvailable }} + minAvailable: {{ . }} + {{- end }} + {{- with .Values.podDisruptionBudget.maxUnavailable }} + maxUnavailable: {{ . }} + {{- end }} selector: matchLabels: {{- include "kafka-ui.selectorLabels" . | nindent 6 }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kafka-ui/templates/pod-disruption-budget.yaml` around lines 6 - 10, The template may emit a duplicate selector because it renders .Values.podDisruptionBudget.spec via toYaml and then unconditionally writes a selector using include "kafka-ui.selectorLabels"; update the template so the rendered spec cannot include a selector: either render only allowed fields (e.g. explicitly output minAvailable and maxUnavailable from .Values.podDisruptionBudget.spec) or strip/omit the selector key before toYaml (so .Values.podDisruptionBudget.spec is rendered without a selector), then keep the hardcoded selector using include "kafka-ui.selectorLabels" to avoid key collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/kafka-ui/templates/deployment.yaml`:
- Around line 195-206: Remove the trailing whitespace after the closing template
tag and ensure the file ends with a single newline: update the deployment.yaml
area around the topologySpreadConstraints block (the section using
.Values.topologySpreadConstraints and the include "kafka-ui.selectorLabels") so
that the final "{{- end }}" has no trailing spaces and add a final newline
character at EOF.
In `@charts/kafka-ui/templates/pod-disruption-budget.yaml`:
- Line 11: The file ends with the Helm template token "{{- end -}}" but lacks a
POSIX trailing newline; update the template (the file containing the "{{- end
-}}" token) to add a single newline character at the end of the file so it ends
with '\n' (LF).
- Around line 6-10: The template may emit a duplicate selector because it
renders .Values.podDisruptionBudget.spec via toYaml and then unconditionally
writes a selector using include "kafka-ui.selectorLabels"; update the template
so the rendered spec cannot include a selector: either render only allowed
fields (e.g. explicitly output minAvailable and maxUnavailable from
.Values.podDisruptionBudget.spec) or strip/omit the selector key before toYaml
(so .Values.podDisruptionBudget.spec is rendered without a selector), then keep
the hardcoded selector using include "kafka-ui.selectorLabels" to avoid key
collisions.
In `@charts/kafka-ui/values.yaml`:
- Around line 281-286: The topologySpreadConstraints block currently sets
minDomains: 2 and whenUnsatisfiable: DoNotSchedule which will prevent pods from
scheduling in single-zone clusters; update the chart by either lowering
topologySpreadConstraints.minDomains to 1 (safer default) or add a prominent
comment/warning next to topologySpreadConstraints.minDomains and
whenUnsatisfiable explaining that enabling topologySpreadConstraints with
minDomains > 1 requires multi-zone clusters and will cause scheduling failures
on single-zone clusters so operators must adjust these values accordingly.
- Line 286: The file is missing a trailing newline at EOF; add a single newline
character after the last line containing "whenUnsatisfiable: DoNotSchedule #
Must be DoNotSchedule if minDomains > 1" so the file ends with a newline to
satisfy POSIX/newline conventions and the repository's linter/CI checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2396991-24b2-4f32-a6c4-1150b756caa3
📒 Files selected for processing (3)
charts/kafka-ui/templates/deployment.yamlcharts/kafka-ui/templates/pod-disruption-budget.yamlcharts/kafka-ui/values.yaml
Haarolean
approved these changes
Mar 28, 2026
Member
|
@likesavabutworse linter's failing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds optional
PodDisruptionBudgetandtopologySpreadConstraints. Both are disabled by default and can be enabled and configured when using deployments with more than1replicaSummary by CodeRabbit
New Features
Chores