feat(service): support topologySpreadConstraints and priorityClassName (0.5.4) - #97
Merged
Merged
Conversation
Adds an optional `topologySpreadConstraints` list, rendered on both the Deployment and the Argo Rollout pod specs. Purely additive: when unset the field renders nothing, so every existing release is byte-identical. Motivation: journeys-website put all 5 replicas on a single class-c-spot node, so one node event took 100% of serving capacity. It already had a preferred podAntiAffinity on kubernetes.io/hostname, correctly applied and matching — but at scheduling time only one node in the pool had room for the pod (every other class-c node had 107-1264Mi free against a 1600Mi request, because the outgoing pods were still holding memory mid-rollout). A soft constraint with one feasible node has nothing to choose. podAntiAffinity cannot express the fix. `required` would demand one node per replica or leave pods Pending — on a pool at 97% of allocatable memory requests that forces continuous scale-up, and combined with a safe-to-evict:"false" annotation it pins one node per replica against consolidation. topologySpreadConstraints evaluates skew across the domains actually eligible for the pod, so `maxSkew: 1` lands 5 replicas 3/2 across two usable nodes rather than 5/0 or Pending. That is the behaviour we want and it degrades gracefully as pool capacity changes. Verified: helm lint clean; renders the block when set and nothing when unset; `kubectl apply --dry-run=server` accepts the rendered journeys-website Deployment against the live 1.35 API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ment Fixes the failing "Check all docs" job, which runs scripts/helm-docs.sh and diffs the committed README against a fresh render. Two changes: - Regenerated charts/service/README.md with helm-docs v1.14.2, matching the CI container image. Diff is the version badge (0.5.3 -> 0.5.4) plus one table row. - Restructured the topologySpreadConstraints comment in values.yaml. helm-docs concatenates every comment line following the `# --` marker into a single table cell, so the original long rationale rendered as one unreadable ~700-character row. The rationale and worked example now sit ABOVE the `# --` line, where helm-docs ignores them, and the marker itself carries a one-line description. Readers of values.yaml keep the full explanation; the README stays scannable. Verified locally with the exact CI command: `sh scripts/helm-docs.sh` -> "All chart README.md files are up to date". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds an optional `priorityClassName`, rendered on the Deployment and Argo Rollout
pod specs alongside topologySpreadConstraints. Additive: defaults to "" and is
wrapped in `{{- with }}`, so nothing renders unless a service opts in.
No PriorityClass in this cluster sets globalDefault, so unset continues to mean
priority 0 — which is what all 302 production pods run at today. Only
rudderstack-critical (1000000000) exists outside the system classes.
Documented in values.yaml, because two things about this field are easy to get
wrong:
- Raising priority enables PREEMPTION: the scheduler evicts lower-priority pods to
make room. With every production workload at priority 0, a high value means
displacing your own services. preemptionPolicy: Never on the PriorityClass gives
scheduling order without eviction.
- A name that does not exist fails late and quietly. Verified against the live
1.35 apiserver: `kubectl apply --dry-run=server` ACCEPTS a Deployment carrying a
bogus priorityClassName, so ArgoCD reports Synced/Healthy — but the Pod is then
rejected with "forbidden: no PriorityClass with name <x> was found", leaving the
new ReplicaSet at 0 replicas while maxUnavailable keeps the old pods serving.
Scope: Deployment and Rollout only, matching topologySpreadConstraints in this PR.
cronjob.yaml and validate-job.yaml also carry pod specs and could take the field
later; left out to keep this PR to one reviewable surface.
Verified: helm lint clean; helm-docs README up to date; renders on both Deployment
and Rollout when set and nothing when unset; all 243 fleet services still render
byte-identically against 0.5.3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Why
journeys-websitescheduled all 5 replicas onto a singleclass-c-spotnode, so one node event takes 100% of serving capacity. It already carried apreferredpodAntiAffinityonkubernetes.io/hostname— correctly applied, selector matching the pod labels — and it still happened.The reason is that a soft constraint needs somewhere to go. Reconstructing node state at scheduling time (
10:06:21Z, mid-rollout, when the outgoing pods still held their memory):One feasible node, so the anti-affinity preference had nothing to act on. The pool sits at 97% of allocatable memory requests.
Why podAntiAffinity can't express the fix
requiredDuringSchedulingIgnoredDuringExecutionis all-or-nothing: one node per replica, orPending. On a pool this full that means:Pendingon every rollout until the autoscaler adds nodescluster-autoscaler.kubernetes.io/safe-to-evict: "false"annotation (whichjourneys-websitenow needs to stop eviction churn), it pins one node per replica against consolidation — ~$129/node/month of pool capacity the autoscaler can never reclaimtopologySpreadConstraintsevaluates skew across the topology domains actually eligible for the pod. WithmaxSkew: 1, five replicas over two usable nodes is a 3/2 split — skew 1, allowed — instead of 5/0 orPending. It spreads over whatever room exists and adapts as pool capacity changes.What changed
Two optional scheduling fields, both rendered on the
deployment.yamlandargo-rollouts.yamlpod specs, plusChart.yaml0.5.3→0.5.4.topologySpreadConstraintspriorityClassNamepriorityClassNamewas the other fieldjourneys-websitewanted and this chart didn't template. Unset keeps today's behaviour exactly: no PriorityClass in this cluster setsglobalDefault, so priority stays 0 — where all 302 production pods already sit. Onlyrudderstack-critical(1000000000) exists outside the system classes.Two footguns are documented in
values.yaml, both worth reading before anyone sets it:preemptionPolicy: Neveron the PriorityClass gives scheduling order without eviction.priorityClassNameis accepted (--dry-run=server→configured), so ArgoCD reports Synced/Healthy. The Pod is then rejected —forbidden: no PriorityClass with name <x> was found— leaving the new ReplicaSet at 0 replicas whilemaxUnavailablekeeps the old pods serving. It degrades silently instead of erroring. Checkkubectl get priorityclassesfirst.Scope: Deployment and Rollout only.
cronjob.yamlandvalidate-job.yamlalso carry pod specs and could take both fields later; left out to keep this to one reviewable surface.Backward compatibility with 0.5.3 — verified against the whole fleet
targetRevisionis pinned fleet-wide atgitops/argocd/applicationsets/prod-eg.yaml:54(andnonprod-eg.yaml:53), so bumping to0.5.4upgrades every service in those ApplicationSets. Rather than argue from the diff, I rendered all of them both ways and compared.Method: for each of the 243 service directories the ApplicationSet generators match across
production,testingandintegration, render with the published0.5.3chart and with this branch, using ArgoCD's exactvalueFilesorder (base/defaults.yaml, the service'svalues.yaml, thenenvs/<env>/<segment>/vars.yamlwhen present) and the same computedreleaseName.243/243 byte-identical. Zero diffs, zero render failures, no asymmetric behaviour.
Two structural reasons it is a guaranteed no-op, not just an empirical one:
[]by default and wrapped in{{- with }}, which skips empty lists — so nothing is emitted unless a service opts in.service.labelsin_helpers.tplcontains nohelm.sh/chartlabel and the chart version appears nowhere in rendered output (verified: 0 occurrences of0.5.3/0.5.4/helm.sh/chartin a full render, both versions). A chart bump that put the version in the pod template would trigger a rolling restart of ~100 workloads on sync; this one changes no pod spec at all, so ArgoCD sees no drift for any service that doesn't set the field.Only
charts/serviceis touched —charts/oneuptimeandcharts/rudderstackare unmodified.I'd still suggest bumping
nonprod-eg.yamlfirst and letting it sit for a sync cycle, on general principle rather than because of a specific risk.Verification
Docs were regenerated with local
helm-docs v1.14.2, the same version as the CI container, and checked with the exact CI command rather than waiting on the job.The
Check all docsjob failed on the first push for a reason worth recording: helm-docs concatenates every comment line after the# --marker into one README table cell, so the original long rationale rendered as a single ~700-character row. Both fields now keep their rationale above the# --line, where helm-docs ignores it — full explanation for anyone readingvalues.yaml, one scannable line in the README.Follow-up (not in this PR)
targetRevisionto0.5.4—nonprod-eg.yamlfirst, thenprod-eg.yamljourneys-website'spodAntiAffinitywithmaxSkew: 1/DoNotSchedule, and correcting the now-stale comment atenvs/production/journeys-website/values.yaml:243that says the chart templates neither fieldpriorityClassNameis wanted forjourneys-website, a suitable PriorityClass has to be created first —rudderstack-criticalat 1e9 is not it, since preempting every priority-0 production pod is not the intent🤖 Generated with Claude Code