Skip to content

feat(traefik): make workload ingress highly available and Burstable#340

Open
timtalbot wants to merge 1 commit into
mainfrom
traefik-workload-ha
Open

feat(traefik): make workload ingress highly available and Burstable#340
timtalbot wants to merge 1 commit into
mainfrom
traefik-workload-ha

Conversation

@timtalbot

@timtalbot timtalbot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

Workload Traefik ingress ran a single BestEffort replica. That combination made it both a single point of failure and the first pod evicted under node pressure: when a node saturated, the lone Traefik pod crash-looped/rescheduled and the LoadBalancer briefly had zero backends, taking a workload's entire external edge offline.

The control-room Traefik already runs HA with resource requests; the workload path was a straight port that never got the same treatment.

Changes

Hardens both the EKS (helm_aws.go) and AKS (clusters_azure.go) Traefik paths:

  • Replicas default to 3, configurable per workload via traefik_deployment_replicas (mirrors the control-room knob). Defaults to 3 when unset.
  • Resource requests/limits (req 200m/256Mi, lim 1000m/512Mi) move Traefik out of BestEffort into Burstable QoS.
  • topologySpreadConstraints spread replicas across hosts (maxSkew: 1, ScheduleAnyway — soft, so a temporarily single-schedulable cluster still schedules and drains cleanly).
  • podDisruptionBudget (maxUnavailable: 1) keeps ≥2 replicas up during node drains.
  • Dedicated cluster-scoped traefik-critical PriorityClass (value below the reserved system-* range) so ingress pods resist eviction. system-cluster-critical is admission-restricted to kube-system, so it can't be reused.

Traefik is stateless here — certs come from cert-manager K8s secrets, there is no local ACME/acme.json — so running multiple replicas needs zero coordination.

Reviewer notes

  • The traefik_deployment_replicas knob is added to both the AWS (AWSWorkloadClusterComponents) and Azure (AzureWorkloadClusterComponentConfig) component structs, since the two clouds have separate config types.
  • The AKS Traefik chart is pinned to a different (older) chart version than the EKS default; all five hardening keys used here are valid in both chart majors, so no version bump was made. Worth confirming via a dry-run render.
  • No config-repo change is required for the default; a per-workload override is just traefik_deployment_replicas: N under components.

Testing

  • go build ./..., go vet, gofmt — clean.
  • Unit tests added/updated: default-3 + override resolvers (AWS & Azure), and value-map assertions (replicas, resources, topologySpread, PDB, priorityClassName, PriorityClass resource) for both the EKS and AKS builders. go test ./steps/... ./types/... passes.

Rollout (pending — not done in this PR)

  • Dry-run on a single production AKS workload; confirm Traefik Deployment 1 → 3 + resources + topologySpread + PDB + priorityClass in the diff.
  • Apply; verify 3/3 ready, pods on distinct nodes, 3 endpoint backends, QoS Burstable, PDB present, edge probe stays green across a rolling pod delete.
  • Roll out to remaining EKS + AKS workloads in batches, confirming per-workload apply policy first.

Workload Traefik ran a single BestEffort replica, making it a SPOF and
the first pod evicted under node pressure - a saturated node could take
a workload's entire edge offline.

Harden both the EKS (helm_aws) and AKS (clusters_azure) paths:
- deployment.replicas defaults to 3, configurable per workload via
  traefik_deployment_replicas (mirrors the control-room knob)
- resource requests/limits move Traefik into Burstable QoS
- topologySpreadConstraints spread replicas across hosts (soft)
- podDisruptionBudget keeps >=2 replicas up during drains
- dedicated cluster-scoped traefik-critical PriorityClass

Traefik is stateless here (certs come from cert-manager secrets, no
local ACME), so multiple replicas need no coordination.
@timtalbot timtalbot marked this pull request as ready for review July 8, 2026 20:49
@timtalbot timtalbot requested a review from a team as a code owner July 8, 2026 20:49
@timtalbot

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @timtalbot's task in 3m 21s —— View job


PR Review: feat(traefik): make workload ingress highly available and Burstable

  • Gathering context
  • Reading changed files
  • Comparing with control-room Traefik implementation
  • Reviewing test coverage
  • Posting review feedback

Overall this is a solid, well-motivated change. The hardening strategy (replicas, resources, topology spread, PDB, PriorityClass) is correct, stateless-safe, and consistent with the motivation. Test coverage is thorough. A few things worth addressing before merge:

Findings

1. maxUnavailable: 1 on a 3-replica PDB is weaker than it looks at the defaultminAvailable: 2 (or equivalently maxUnavailable: 1) is fine when replicas ≥ 3, but maxUnavailable: 1 is vacuously permissive when someone overrides traefik_deployment_replicas: 1. With a single replica, a node drain would still proceed and you'd have zero backends. Consider documenting the assumption or adding a validation guard (or using minAvailable: 2 to make the floor explicit). Low-priority, but worth noting in operator docs.

2. AKS ResolveAzureComponents() called once but wastefully (clusters_azure.go:629)

traefikReplicas := clusterCfg.Components.ResolveAzureComponents().TraefikDeploymentReplicas

ResolveAzureComponents() builds and returns a full ResolvedAzureComponents struct with all field defaults evaluated. It's called only to extract one field, while other component versions are read directly off clusterCfg.Components via individual resolveString calls elsewhere. This is consistent with the existing Azure pattern (there's no resolved variable used in this function like on the AWS side), so it's not a bug — but it allocates a full struct to read one field. Not a correctness issue; style-only. Fix this →

3. topologySpreadConstraints uses ScheduleAnyway — soft constraint — as noted in the PR description, this is intentional for single-schedulable-node situations. But it means 3 replicas can all land on the same node if only one is available. The PR description already documents this tradeoff; no code change needed — just making sure reviewers are aware before sign-off.

4. TraefikDeploymentReplicas field has both json and yaml tags on the AWS type but only yaml on the Azure type — this is consistent with the existing patterns in each struct (AWS structs use both; Azure structs are yaml-only) so it's correct as-is. Noting it only for awareness.

5. EKS PriorityClass gets an alias for Python state adoption; AKS does not — correct, since PriorityClass is net-new in both paths. The AKS Helm release still carries withTraefikAlias() for Python adoption of the existing Traefik resources. No issue.

6. Minor: large comment block on HA hardening in clusters_azure.go — the multi-line comment at clusters_azure.go:636-640 explains why resources/PDB/etc. are added, which is valuable. However per CLAUDE.md guidelines comments should document the non-obvious why, not narrate the code. The single-line comment on each section (resources, PDB, priorityClass) would be cleaner than the block preamble. This is a style nit; the behavior is correct.

Test Coverage Assessment

Tests are thorough and well-structured:

  • TestResolveAWSComponentsTraefikDeploymentReplicas / TestResolveAzureComponentsTraefikDeploymentReplicas — default and override for both cloud types ✓
  • TestAwsHelmTraefikHA — asserts replicas, resources, topologySpread, PDB, priorityClassName, and the PriorityClass Kubernetes resource itself ✓
  • TestAwsHelmTraefikReplicasOverride — confirms the override path ✓
  • TestAzureClustersDeployTraefikHA / TestAzureClustersDeployTraefikReplicasOverride — mirrors the AWS test set for AKS ✓

The AWS test smartly deserializes valuesContent YAML to verify the actual Helm values rather than just checking the map[string]interface{} passed to the CR spec. That's the right level of fidelity for a HelmChart CR.

Pre-merge checklist reminder

As called out in the PR description: a dry-run on a real AKS workload to confirm chart version 33.2.1 accepts podDisruptionBudget.enabled is still pending. 37.1.2 (EKS) and 33.2.1 (AKS) are different chart majors — worth verifying the AKS render before rolling broadly.


Summary: Correctness looks good, the implementation is clean and consistent between clouds, tests are solid. The PDB maxUnavailable: 1 / single-replica edge case and the dry-run verification are the two things I'd want resolved before rolling to production. No blocking issues.

@timtalbot

Copy link
Copy Markdown
Contributor Author

Verified on internal AWS and Azure workloads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant