Skip to content

fix: only count topology domains the pod can actually reach - #57

Open
lukeraphael wants to merge 3 commits into
mainfrom
devin/1787902558-topology-domain-affinity
Open

fix: only count topology domains the pod can actually reach#57
lukeraphael wants to merge 3 commits into
mainfrom
devin/1787902558-topology-domain-affinity

Conversation

@lukeraphael

@lukeraphael lukeraphael commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

A topology spread group seeded its domains from every NodePool's instance-type offerings, even domains no node the pod could land on can supply. In a cluster where some NodePools span another region and a pod is pinned to one pool (karpenter.sh/nodepool=monitoring), those foreign zones stay at count 0, so domainMinCount is pinned at 0 and no domain is within maxSkew: 1 of it — a DoNotSchedule zone spread becomes permanently unsatisfiable and drift/consolidation never converges (unsatisfiable topology constraint for topology spread, key=topology.kubernetes.io/zone). kube-scheduler doesn't hit this because it only counts zones of nodes matching the pod.

TopologyDomainGroup now remembers which NodePool contributed each domain, along with that pool's taints and requirements, and ForEachDomain takes the whole pod-aware TopologyNodeFilter instead of just the taint policy:

-type TopologyDomainGroup map[string][][]v1.Taint
+type TopologyDomainGroup map[string]map[string]TopologyDomainSource // domain -> nodepool -> {Taints, Requirements}

 func (t TopologyDomainGroup) ForEachDomain(pod, nodeFilter TopologyNodeFilter, f func(domain string)) {
     // keep a domain if *any* nodepool supplying it can host the pod
     taints tolerated (unless TaintPolicy==Ignore) &&
     !nodeFilter.ConflictsWithRequirements(source.Requirements) (unless AffinityPolicy==Ignore)
 }

The affinity check is deliberately conflict-only (Requirements.Intersects, not Compatible): a NodePool's stored requirements are the ones common to every node it can launch, so a label the pool leaves to the instance type is undefined there and must not count as a mismatch. Only an outright value conflict (e.g. pod wants nodepool=monitoring, source is nodepool=accelerators) drops a domain, so the filter can never hide a domain that a node from that pool could actually provide.

Domain groups stay candidate-invariant and cacheable — the pod-dependent filtering happens at NewTopologyGroup time.

Testing

  • topologydomaingroup_internal_test.go: node selector, required node affinity, NodeAffinityPolicy: Ignore, taints/tolerations, and a pod selecting an instance-type-only label (domains kept).
  • topology_test.go: end-to-end spread of 4 pods pinned to a 2-zone NodePool while another pool offers a third zone — expects skew 2,2. Reverting the affinity condition reproduces the exact production error and fails this test.
  • Full pkg/controllers/provisioning/scheduling, provisioning, and disruption suites pass.

Link to Devin session: https://app.devin.ai/sessions/e2c5be51371140be96fe3a13f15e0260
Open in Devin Desktop: https://app.devin.ai/desktop/session/e2c5be51371140be96fe3a13f15e0260?variant=devin
Requested by: @lukeraphael


Devin Review

Topology spread groups were seeded with every domain any NodePool could
supply, so a pod pinned to one NodePool counted zero-pod domains that only
other NodePools offer. That holds the group minimum at zero and puts every
reachable domain outside maxSkew, making a DoNotSchedule spread permanently
unsatisfiable and blocking drift/consolidation replacement.

Track the NodePool requirements alongside the taints for each domain and skip
domains no NodePool can supply to the pod when its NodeAffinityPolicy is honor.
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Devin Review: 1 flag

Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

@exa-heron-staging exa-heron-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heron review

global-review-orchestrator-guardian

Verdict: ✅ Approve · 0 findings · head b90646ba14 · openai/gpt-5.6-luna · 3m 37s · session

Last updated 2026-08-28 08:02 UTC · reply @heron dismiss <reason> to dismiss on its comment thread

Comment thread pkg/controllers/provisioning/scheduling/topology.go
@devin-ai-integration devin-ai-integration Bot changed the title scheduling: only count topology domains the pod can actually reach fix: only count topology domains the pod can actually reach Aug 28, 2026
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