feat: optionally distribute replicas across architectures - #153
feat: optionally distribute replicas across architectures#153bschwedler wants to merge 2 commits into
Conversation
Adds a user-configurable topologySpreadConstraints field to Connect, Workbench, and Package Manager, passed straight through to the server Deployment's PodSpec. Enables spreading a product's replicas across topology domains (e.g. kubernetes.io/arch), which a hard maxSkew:1/DoNotSchedule constraint can use to force one replica per CPU architecture in a multi-arch cluster. The field is independent of the operator's existing hardcoded hostname-keyed soft anti-affinity (shared.go's ComponentSpecPodAntiAffinity) and composes with it without conflict, since Affinity and TopologySpreadConstraints are separate PodSpec fields. Scoped to the server Deployment only; Workbench session pods are unaffected. No Affinity override or podLabels field is added — out of scope for the immediate use case. Mirrors the pattern used for the Resources field (#146): spec field on each product type + the Site's nested Internal*Spec, controller wiring, Site-to-product propagation, and regenerated CRDs/deepcopy/client-go via `just mgenerate` + `just helm-generate`.
A TopologySpreadConstraint with no labelSelector matches zero pods under Kubernetes semantics (metav1.LabelSelectorAsSelector(nil) returns labels.Nothing()), which silently turns maxSkew and whenUnsatisfiable into a no-op. That's the exact shape used in the motivating multi-arch example, so shipping the raw pass-through from the prior commit would let the constraint compile, deploy, and pass review while never actually forcing anything. Neither Kubernetes nor the wider operator ecosystem solves this well: upstream closed a request to validate it as "not planned" (kubernetes/kubernetes#135797), and prometheus-operator's own StatefulSet builder does the same raw pass-through. Extracts the label-identity selector already used by ComponentSpecPodAntiAffinity into componentSelector, and adds ComponentSpecTopologySpreadConstraints, which fills in that selector on any constraint that omits one before it reaches the Deployment. An explicit caller-supplied selector is always left untouched, so the default only fires for the common case this field exists to serve: spreading a component's own replicas.
|
Claude finished @bschwedler's task in 3m 13s —— View job PR Review: feat: optionally distribute replicas across architectures
Overall this is a clean, well-scoped feature. The core logic in APIMissing No kubebuilder validation on the Shared logic (
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // TopologySpreadConstraints controls how server pod replicas are spread across topology | ||
| // domains (e.g. node architecture, zone, hostname). Composes with the operator's built-in | ||
| // hostname-keyed soft anti-affinity. | ||
| TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"` |
There was a problem hiding this comment.
The InternalConnectSpec.TopologySpreadConstraints field is missing a // +optional kubebuilder marker. All optional slice fields in this file use it (e.g. AdditionalRuntimeImages at line 350). Without it, the CRD schema technically marks the field as required in some generators.
Same applies to the TopologySpreadConstraints fields added to InternalPackageManagerSpec (line 223) and InternalWorkbenchSpec (line 434).
| } | ||
| out[i] = c | ||
| } | ||
| return out |
There was a problem hiding this comment.
Suggestion: When constraints is nil, this returns a non-nil empty slice (make([]corev1.TopologySpreadConstraint, 0)). Kubernetes treats both the same, but if any downstream code does a == nil check on the result, it would surprise. Consider early-returning nil when the input is empty:
| return out | |
| func ComponentSpecTopologySpreadConstraints( | |
| constraints []corev1.TopologySpreadConstraint, | |
| p product.KubernetesLabelser, | |
| ) []corev1.TopologySpreadConstraint { | |
| if len(constraints) == 0 { | |
| return nil | |
| } | |
| out := make([]corev1.TopologySpreadConstraint, len(constraints)) | |
| for i, c := range constraints { | |
| if c.LabelSelector == nil { | |
| c.LabelSelector = componentSelector(p) | |
| } | |
| out[i] = c | |
| } | |
| return out | |
| } |
| require.NotNil(t, got[0].LabelSelector, "an omitted LabelSelector must be auto-filled, or the constraint silently matches zero pods") | ||
| selector, err := metav1.LabelSelectorAsSelector(got[0].LabelSelector) | ||
| require.NoError(t, err) | ||
| assert.True(t, selector.Matches(labels.Set(c.KubernetesLabels())), |
There was a problem hiding this comment.
Good test: it verifies that the auto-filled selector actually matches the component's pod labels, not just that it's non-nil. The labels.Set(c.KubernetesLabels()) check is the right level of validation here.
| constraints []corev1.TopologySpreadConstraint, | ||
| p product.KubernetesLabelser, | ||
| ) []corev1.TopologySpreadConstraint { | ||
| out := make([]corev1.TopologySpreadConstraint, len(constraints)) |
There was a problem hiding this comment.
The componentSelector uses MatchExpressions (specifically In) rather than MatchLabels. Both produce equivalent Kubernetes behavior, but the anti-affinity helper directly above uses MatchExpressions for the same reason — so this is consistent.
One thing to note: the selector is built from KubernetesLabels() keys app.kubernetes.io/instance and posit.team/site. These are the selector labels, so they're stable and correct for pod matching. Looks good.
| } | ||
| out[i] = c | ||
| } | ||
| return out |
There was a problem hiding this comment.
Important: When constraints is nil (no constraints configured), this returns a non-nil empty slice (make([]corev1.TopologySpreadConstraint, 0)). Kubernetes treats an explicit empty slice the same as nil here, so this is harmless — but it's worth knowing that callers passing nil get back []TopologySpreadConstraint{} rather than nil. If downstream code ever does a == nil check on the result, it would behave unexpectedly.
Consider returning nil when the input is empty:
| return out | |
| func ComponentSpecTopologySpreadConstraints( | |
| constraints []corev1.TopologySpreadConstraint, | |
| p product.KubernetesLabelser, | |
| ) []corev1.TopologySpreadConstraint { | |
| if len(constraints) == 0 { | |
| return nil | |
| } | |
| out := make([]corev1.TopologySpreadConstraint, len(constraints)) | |
| for i, c := range constraints { | |
| if c.LabelSelector == nil { | |
| c.LabelSelector = componentSelector(p) | |
| } | |
| out[i] = c | |
| } | |
| return out | |
| } |
1 participant
Adds an optional topologySpreadConstraints field to Connect, Workbench,
and Package Manager, so a Site can guarantee its product replicas split
across topology domains — e.g. one amd64 and one arm64 replica in a
multi-arch cluster — instead of relying only on the operator's existing
soft, hostname-keyed anti-affinity.
An omitted labelSelector on a constraint is auto-filled to the
component's own pod labels, since Kubernetes otherwise treats a nil
selector as matching zero pods, silently turning the constraint into a
no-op. An explicit caller-supplied selector is never overridden.