feat(reliability): add PodDisruptionBudget and pod anti-affinity for Temporal workers#68
feat(reliability): add PodDisruptionBudget and pod anti-affinity for Temporal workers#68kiwueke2 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise 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 (1)
📝 WalkthroughWalkthroughThe Helm chart updates Temporal worker labels, adds PodDisruptionBudgets, enables preferred anti-affinity for both worker deployments, updates monitoring selection, and documents the resulting reliability and verification behavior. ChangesTemporal Worker Reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/templates/temporal.yaml`:
- Around line 778-794: The PodDisruptionBudget selector is too broad and matches
both worker pods via the shared app/component labels. Update the
nv-config-manager worker pod template to add a unique identifying label, then
change this PDB’s selector in the temporal.yaml template to match that label
instead of only app: {{ $temporalName }} and component: worker. Use the existing
nv-config-manager.temporalWorkerName resource and its pod template labels to
keep the selector scoped to the intended worker only.
In `@deploy/helm/values.yaml`:
- Around line 1731-1742: The anti-affinity settings are attached at the wrong
level, so `configManagerWorker.affinity` remains empty. Move the `affinity`
block to be nested under `temporal.configManagerWorker` in the Helm values,
keeping the existing `podAntiAffinity` structure intact so the worker Deployment
reads `.Values.temporal.configManagerWorker.affinity` correctly.
In `@docs/temporal/temporal-deployment.mdx`:
- Around line 60-69: The verification example only checks the Temporal server
worker pods, so it does not actually cover both worker Deployments described in
this section. Update the verification guidance near the pod anti-affinity
explanation to include the nv-config-manager workflow-executor pods as well,
either by documenting its selector explicitly or by using the new unique label
once it exists, so the command sequence verifies spread for both worker sets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ae18d1e-0b29-4fd8-afc8-dea22ff0e7c0
📒 Files selected for processing (3)
deploy/helm/templates/temporal.yamldeploy/helm/values.yamldocs/temporal/temporal-deployment.mdx
|
Verifications on local deployment
|
…Temporal worker pods Adds two PDBs (maxUnavailable: 1) to prevent simultaneous eviction of all worker replicas during node drains. Activates preferred pod anti-affinity on kubernetes.io/hostname for both the Temporal server worker service and the nv-config-manager workflow executor so replicas spread across nodes. Also adds app.kubernetes.io/component: temporal-worker label to the worker pod template so the PDB selector and anti-affinity rule land on the correct pods, and documents the reliability configuration in the deployment runbook. Signed-off-by: Kezie Iwueke <kiwueke@nvidia.com>
…coverage for configManagerWorker Signed-off-by: Kezie Iwueke <kiwueke@nvidia.com>
847b5a7 to
6389177
Compare
|
/ok to test 6389177 |
…r, correct docs disruption count Signed-off-by: Kezie Iwueke <kiwueke@nvidia.com>
Description
Adds a PodDisruptionBudget (maxUnavailable: 1) for both Temporal worker deployments — the Temporal server worker role and the nv-config-manager workflow executor so node drains can't evict all replicas at once.
Also activates the pod anti-affinity rules that were already in values.yaml but commented out. With preferredDuringSchedulingIgnoredDuringExecution on kubernetes.io/hostname, the scheduler will spread the 3 replicas across different nodes rather than potentially stacking them on one. Added the app.kubernetes.io/component: temporal-worker label to the worker pod template so the PDB selector and anti-affinity rule actually land on the right pods.
Validation
Verified locally with helm template command that both PDBs render with correct selectors, the new label appears on the worker pod template, and the anti-affinity block is present in the Deployment spec.
The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for review,
run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite,or narrow it only while debugging.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit