Conversation
🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1166 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
WalkthroughA new PodDisruptionBudget template and corresponding Helm values are added to enable optional pod disruption budget configuration. The template conditionally renders the PDB resource with validation to prevent simultaneous specification of minAvailable and maxUnavailable constraints. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds PodDisruptionBudget (PDB) support to the lfx-v2-project-service Helm chart, allowing operators to configure disruption budgets to maintain availability during voluntary disruptions like node drains or cluster upgrades. The feature is disabled by default.
Changes:
- Adds a new
pdb.yamlHelm template that conditionally creates aPodDisruptionBudgetresource with configurableminAvailableandmaxUnavailablesettings - Adds
podDisruptionBudgetconfiguration values tovalues.yaml(disabled by default)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
charts/lfx-v2-project-service/templates/pdb.yaml |
New template that creates a PodDisruptionBudget when enabled, using the same app: {{ .Chart.Name }} selector as the Deployment |
charts/lfx-v2-project-service/values.yaml |
Adds podDisruptionBudget section with enabled, minAvailable, and maxUnavailable options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| podDisruptionBudget: | ||
| enabled: false | ||
| # minAvailable: 1 | ||
| # maxUnavailable: 1 |
There was a problem hiding this comment.
The podDisruptionBudget section is missing the descriptive comment above it that every other top-level section in this file has (e.g., # image is the configuration for the container images, # replicaCount is the number of replicas for the deployment, # resources is the configuration for container resource limits and requests). Please add a comment like # podDisruptionBudget is the configuration for the PodDisruptionBudget resource to stay consistent with the rest of the file.
| {{- if .Values.podDisruptionBudget.enabled }} | ||
| --- | ||
| apiVersion: policy/v1 | ||
| kind: PodDisruptionBudget | ||
| metadata: | ||
| name: {{ .Chart.Name }} | ||
| namespace: {{ .Release.Namespace }} | ||
| spec: | ||
| selector: | ||
| matchLabels: | ||
| app: {{ .Chart.Name }} | ||
| {{- with .Values.podDisruptionBudget.minAvailable }} | ||
| minAvailable: {{ . }} | ||
| {{- end }} | ||
| {{- with .Values.podDisruptionBudget.maxUnavailable }} | ||
| maxUnavailable: {{ . }} | ||
| {{- end }} |
There was a problem hiding this comment.
If podDisruptionBudget.enabled is true but neither minAvailable nor maxUnavailable is specified, this template will create a PDB with no disruption constraint — effectively allowing all pods to be disrupted simultaneously. This is a valid Kubernetes resource but almost certainly a misconfiguration. Consider adding a validation check to fail with a helpful error message when the PDB is enabled without either field, for example:
{{- if and .Values.podDisruptionBudget.enabled (not (or .Values.podDisruptionBudget.minAvailable .Values.podDisruptionBudget.maxUnavailable)) }}
{{- fail "podDisruptionBudget.enabled is true but neither minAvailable nor maxUnavailable is set" }}
{{- end }}
🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1166 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
![coderabbitai[bot]](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 `@charts/lfx-v2-project-service/templates/pdb.yaml`:
- Around line 3-6: The template currently checks that both minAvailable and
maxUnavailable are not set simultaneously but misses validation that at least
one must be provided when .Values.podDisruptionBudget.enabled is true; update
the pdb.yaml logic (the block using hasKey .Values.podDisruptionBudget
"minAvailable" and hasKey ... "maxUnavailable") to also fail when neither key is
present by adding a condition like if enabled and not hasKey(... "minAvailable")
and not hasKey(... "maxUnavailable") then call fail with a clear message (e.g.,
"podDisruptionBudget: either minAvailable or maxUnavailable must be set when
enabled is true") so the chart enforces one constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63cb8daf-e182-4e20-abbe-08de1e22892c
📒 Files selected for processing (2)
charts/lfx-v2-project-service/templates/pdb.yamlcharts/lfx-v2-project-service/values.yaml
| {{- if .Values.podDisruptionBudget.enabled }} | ||
| {{- if and (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable") }} | ||
| {{- fail "podDisruptionBudget: cannot set both minAvailable and maxUnavailable" }} | ||
| {{- end }} |
There was a problem hiding this comment.
Add validation for when neither constraint is specified.
The template validates that both cannot be set, but doesn't validate that at least one must be set. If enabled: true without either minAvailable or maxUnavailable, the resulting PDB will have no constraint and won't provide disruption protection.
🛡️ Proposed fix to add validation
{{- if .Values.podDisruptionBudget.enabled }}
{{- if and (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable") }}
{{- fail "podDisruptionBudget: cannot set both minAvailable and maxUnavailable" }}
{{- end }}
+{{- if not (or (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable")) }}
+ {{- fail "podDisruptionBudget: must set either minAvailable or maxUnavailable when enabled" }}
+{{- end }}
---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- if .Values.podDisruptionBudget.enabled }} | |
| {{- if and (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable") }} | |
| {{- fail "podDisruptionBudget: cannot set both minAvailable and maxUnavailable" }} | |
| {{- end }} | |
| {{- if .Values.podDisruptionBudget.enabled }} | |
| {{- if and (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable") }} | |
| {{- fail "podDisruptionBudget: cannot set both minAvailable and maxUnavailable" }} | |
| {{- end }} | |
| {{- if not (or (hasKey .Values.podDisruptionBudget "minAvailable") (hasKey .Values.podDisruptionBudget "maxUnavailable")) }} | |
| {{- fail "podDisruptionBudget: must set either minAvailable or maxUnavailable when enabled" }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/lfx-v2-project-service/templates/pdb.yaml` around lines 3 - 6, The
template currently checks that both minAvailable and maxUnavailable are not set
simultaneously but misses validation that at least one must be provided when
.Values.podDisruptionBudget.enabled is true; update the pdb.yaml logic (the
block using hasKey .Values.podDisruptionBudget "minAvailable" and hasKey ...
"maxUnavailable") to also fail when neither key is present by adding a condition
like if enabled and not hasKey(... "minAvailable") and not hasKey(...
"maxUnavailable") then call fail with a clear message (e.g.,
"podDisruptionBudget: either minAvailable or maxUnavailable must be set when
enabled is true") so the chart enforces one constraint.
Summary
podDisruptionBudgetvalues to Helm chart (disabled by default)minAvailableandmaxUnavailable🤖 Generated with Claude Code