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>
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new Helm PodDisruptionBudget template and corresponding values; the template conditionally renders a PDB when enabled and validates that exactly one of 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
Fail if neither minAvailable nor maxUnavailable is set when PDB is enabled. Refactor hasKey checks to local variables. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-1166 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
Summary
podDisruptionBudgetvalues to Helm chart (disabled by default)minAvailableandmaxUnavailable🤖 Generated with Claude Code