feat: add scheduling.zone.pinning

[jdheyburn]

This PR closes #343

Summary

Adds spec.scheduling.zone.pinning, an ordered list of zones that assigns every pod a zone outright instead of asking the scheduler to balance across them. Phase 2 of #265, after the node axis (#310) and the zone axis (#340). Covers the zone placement half of #146.

It's opt-in. A cluster that doesn't set it renders exactly as it did before, so upgrading the operator won't reschedule anything.

User docs are in docs/valkeycluster.md under Scheduling.

Features / Behaviour Changes

spec.scheduling.zone.pinning.zones takes 1 to 32 unique zone names. A pod's zone is zones[(shardIndex + nodeIndex) % len(zones)], which lands on the pod as a topology.kubernetes.io/zone nodeSelector entry.

Each shard starts at a different offset, so a shard's pods spread across consecutive zones. Primaries end up in separate zones too, as long as you have at least as many zones as shards.

Adding shards or replicas won't move existing pods, since their indices don't change.

Admission rejects duplicate or empty zone names, pinning combined with any non-Disabled zone.spread, an in-place edit of the list, and a passthrough nodeSelector that sets the zone key itself.

Implementation

Same idea as #310 and #340. The cluster controller renders the curated config into plain scheduling primitives on the ValkeyNode, and ValkeyNode itself doesn't change.

• api/v1alpha1/valkeycluster_types.go gets the ZonePinning type and a Pinning pointer on ZoneScheduling, plus four schema bounds and four CEL rules
• internal/controller/scheduling.go gets three helpers sitting next to the existing effectiveZoneSpread/zoneSpreadTSCs. buildClusterValkeyNode changes by two lines.

I picked nodeSelector over nodeAffinity since we would have to insert the new condition into every nodeAffinity entry. It would simpler for me to pick nodeSelector since that gets ANDed to everything - plus it is just one label.

withZonePin returns its input untouched when pinning is off, which keeps a nil nodeSelector nil. DeepEqual treats nil and an empty map as different, so returning an empty map would roll every pod on upgrade and undo #317.

Length bounds needed on zones for the CEL cost estimator, I don't anticipate something needing 32 zones but it's some maximum limit. I am OK to lower this. MinLength=1 to validate that we're going to be adding a label value.

Also updated the docs for scheduling.affinity field doc to what really happens (labels are ANDed together).

Limitations

• If you use persistence, pin at creation time

  • Zonal volumes can't follow a pod, so switching pinning on for an existing persistent cluster strands every pod whose volume sits in the wrong zone
  • The operator can't catch that at admission because it has no idea where your PVs are
  • Removing pinning gets you back with no data loss
  • You also need a WaitForFirstConsumer StorageClass
  • With Immediate binding the volume is provisioned before the scheduler picks a node, so pinning can't work at all, even on a fresh cluster
  • The docs walk through the migration runbook and flag the case where a shard has no replicas and you'd genuinely lose data.

• The zone list is immutable while set

  • Changing it means removing pinning, reconciling, then adding it back, which on a persistent cluster is the hazard above.
  • We can always loosen this in the future

• Primaries only land in distinct zones while shards <= len(zones)

  • A primary is node-index 0, so its zone is zones[shardIndex % len(zones)], and six shards over three zones puts 0 and 3 together
  • This is intended

Two things admission can't see, so they aren't validated: a user affinity, or a passthrough topologySpreadConstraints on the zone key, that contradicts the pin. Both show up at runtime as Degraded with reason PodUnschedulable.

Testing

Unit tests in scheduling_test.go and valkeynode_resources_test.go. Five new functions covering the round robin at every shard/node position in a 3x2x3 example, the single zone case, pinning off, nil preservation, that the caller's map isn't mutated, key precedence, and the rendered ValkeyNode with and without pinning.

envtest in scheduling_validation_test.go. 14 cases against a real API server, one per rule plus the accept cases, including remove-then-re-add, an omitted spread block to exercise the CEL fallback, and pinning alongside node.spread to confirm the axes stay independent.

make test passes, make lint is clean, and make manifests generate produces no drift.

No e2e in this one. It would need a multi zone cluster or relabelled nodes, and the rendering is a map assignment the unit tests already cover, with envtest exercising the validation against a real API server which was the same decision in #340.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message explains what changed and why
• Tests are added or updated.
• Documentation files are updated.
• I have run pre-commit locally (pre-commit run --all-files or hooks on commit)

[jdheyburn]

@greptile-apps

[greptile-apps]

Greptile Summary

Adds deterministic per-pod zone pinning to ValkeyCluster scheduling, including API configuration and rendering of selected zones into workload node selectors.

A zone value containing an invalid Kubernetes label character, such as eu-west-1a!, is accepted by the new API constraints and propagated into topology.kubernetes.io/zone. Kubernetes rejects the resulting workload specification, preventing the affected ValkeyNode from becoming ready. This should be fixed before merge.

Confidence Score: 3/5

Not safe to merge until T-Rex findings are addressed.

T-Rex reproduced 2 failing behaviors at runtime in api/v1alpha1/valkeycluster_types.go; the change needs fixes before it is safe to merge.

Files Needing Attention: api/v1alpha1/valkeycluster_types.go

T-Rex Logs

What T-Rex did

• T-Rex produced a finding-comment-proof for a posted P1 finding about an invalid zone label value 'eu-west-1a!'.
• T-Rex produced a second finding-comment-proof for another posted P1 finding.
• T-Rex produced a general-contract-validation-proof confirming the label bounds and Kubernetes validation behavior for both valid and invalid label cases, and captured the separate envtest bootstrap run.

_{Ran code and verified through T-Rex}

Comments Outside Diff (1)

1. General comment

   Zone pinning accepts invalid Kubernetes label values that render into node selectors

   • Bug
     • A 1–63 character invalid label value such as eu-west-1a! passes zone.pinning.zones validation and reaches topology.kubernetes.io/zone in the rendered pod template. Kubernetes label validation rejects that value, so reconciliation produces a workload specification Kubernetes will reject.
   • Cause
     • api/v1alpha1/valkeycluster_types.go:256-257 constrain each zone only with MinLength=1 and MaxLength=63; they do not require Kubernetes label-value syntax.
   • Fix
     • Add an item-level Kubebuilder validation pattern matching Kubernetes label values (with the existing minimum length retained), regenerate the CRD, and add an admission regression test for eu-west-1a!.

   _{Ran code and verified through T-Rex}

_{Reviews (1): Last reviewed commit: "feat: add scheduling.zone.pinning" | Re-trigger Greptile}

[greptile-apps]

Zone pinning accepts invalid Kubernetes label values

zone.pinning.zones only enforces a 1–63-character length. Values such as eu-west-1a! therefore pass CRD validation and are copied unchanged into topology.kubernetes.io/zone node selectors. Kubernetes rejects ! in a label value when the resulting workload is created, leaving the affected ValkeyNode—and consequently cluster reconciliation—unable to become ready.

Require Kubernetes label-value syntax for each zone entry, regenerate the CRD, and add an admission regression case for an invalid value such as eu-west-1a!.

Artifacts

Targeted Go harness for invalid zone label value "eu-west-1a!"

• The exact executed harness checks the generated CRD bounds, renderer, pod template, and Kubernetes validator for "eu-west-1a!"; takeaway: the full reachable path is covered without production changes.

Successful targeted validation output for invalid label "eu-west-1a!"

• The executed Go test reports that "eu-west-1a!" is admitted by 1..63 bounds, rendered into both selectors, and rejected by Kubernetes label validation; takeaway: the defect is confirmed.

Envtest API-server bootstrap output while attempting invalid label "eu-west-1a!"

• The attempted envtest API admission run failed before specs ran because the bundled kube-apiserver did not become ready; takeaway: live API-server admission was environment-blocked, so the targeted direct-validator fallback was used.

Captured source for the invalid label "eu-west-1a!" harness

• This command capture records the copied exact harness source used to validate "eu-west-1a!"; takeaway: the executed test source is independently preserved.

_{Ran code and verified through T-Rex}