diff --git a/pkg/controllers/provisioning/scheduling/topology.go b/pkg/controllers/provisioning/scheduling/topology.go index a78aecedc1..0f6b505d98 100644 --- a/pkg/controllers/provisioning/scheduling/topology.go +++ b/pkg/controllers/provisioning/scheduling/topology.go @@ -113,6 +113,10 @@ func buildDomainGroups(nodePools []*v1.NodePool, instanceTypes map[string][]*clo domainGroups := map[string]TopologyDomainGroup{} for npName, its := range instanceTypes { np := nodePoolIndex[npName] + // Requirements carried by every node this NodePool launches, regardless of instance type. + // Domains are attributed to them so that a pod which can't select the NodePool doesn't have + // its topology spread computed against domains only this NodePool can supply. + nodePoolRequirements := nodePoolDomainRequirements(np) for _, it := range its { // We need to intersect the instance type requirements with the current nodePool requirements. This // ensures that something like zones from an instance type don't expand the universe of valid domains. @@ -125,7 +129,7 @@ func buildDomainGroups(nodePools []*v1.NodePool, instanceTypes map[string][]*clo domainGroups[topologyKey] = NewTopologyDomainGroup() } for _, domain := range requirement.Values() { - domainGroups[topologyKey].Insert(domain, np.Spec.Template.Spec.Taints...) + domainGroups[topologyKey].Insert(domain, npName, np.Spec.Template.Spec.Taints, nodePoolRequirements) } } } @@ -138,7 +142,7 @@ func buildDomainGroups(nodePools []*v1.NodePool, instanceTypes map[string][]*clo domainGroups[key] = NewTopologyDomainGroup() } for _, value := range requirement.Values() { - domainGroups[key].Insert(value, np.Spec.Template.Spec.Taints...) + domainGroups[key].Insert(value, npName, np.Spec.Template.Spec.Taints, nodePoolRequirements) } } } @@ -146,6 +150,21 @@ func buildDomainGroups(nodePools []*v1.NodePool, instanceTypes map[string][]*clo return domainGroups } +// nodePoolDomainRequirements returns the requirements shared by every node the NodePool can launch: +// its template requirements and labels, plus the labels Karpenter stamps onto each of its NodeClaims. +// Instance type requirements are deliberately left out, since a domain can be offered by only some of +// the pool's instance types while this set has to hold for any node supplying the domain. +func nodePoolDomainRequirements(np *v1.NodePool) scheduling.Requirements { + requirements := scheduling.NewNodeSelectorRequirementsWithMinValues(np.Spec.Template.Spec.Requirements...) + requirements.Add(scheduling.NewLabelRequirements(np.Spec.Template.Labels).Values()...) + nodeClaimLabels := map[string]string{v1.NodePoolLabelKey: np.Name} + if ref := np.Spec.Template.Spec.NodeClassRef; ref != nil { + nodeClaimLabels[v1.NodeClassLabelKey(ref.GroupKind())] = ref.Name + } + requirements.Add(scheduling.NewLabelRequirements(nodeClaimLabels).Values()...) + return requirements +} + // topologyError allows lazily generating the error string in the topology error. If a pod fails to schedule, most often // we are only interested in the fact that it failed to schedule and not why. type topologyError struct { diff --git a/pkg/controllers/provisioning/scheduling/topology_test.go b/pkg/controllers/provisioning/scheduling/topology_test.go index 96ea487719..655d977c3b 100644 --- a/pkg/controllers/provisioning/scheduling/topology_test.go +++ b/pkg/controllers/provisioning/scheduling/topology_test.go @@ -170,6 +170,31 @@ var _ = Describe("Topology", func() { // should spread the two pods evenly across the only valid zones in our universe (the two zones from our single nodePool) ExpectSkew(ctx, env.Client, "default", &topology[0]).To(ConsistOf(2, 2)) }) + It("should not count zones that are only offered by NodePools the pod can't select", func() { + // Zones only reachable through another NodePool hold no pods, so counting them would peg the + // spread's minimum at zero and leave the pod's own zones permanently outside maxSkew. + nodePool.Spec.Template.Spec.Requirements = []v1.NodeSelectorRequirementWithMinValues{ + {Key: corev1.LabelTopologyZone, Operator: corev1.NodeSelectorOpIn, Values: []string{"test-zone-1", "test-zone-2"}}} + otherNodePool := test.NodePool(v1.NodePool{Spec: v1.NodePoolSpec{Template: v1.NodeClaimTemplate{Spec: v1.NodeClaimTemplateSpec{ + Requirements: []v1.NodeSelectorRequirementWithMinValues{ + {Key: corev1.LabelTopologyZone, Operator: corev1.NodeSelectorOpIn, Values: []string{"test-zone-3"}}}, + }}}}) + topology := []corev1.TopologySpreadConstraint{{ + TopologyKey: corev1.LabelTopologyZone, + WhenUnsatisfiable: corev1.DoNotSchedule, + LabelSelector: &metav1.LabelSelector{MatchLabels: labels}, + MaxSkew: 1, + }} + ExpectApplied(ctx, env.Client, nodePool, otherNodePool) + ExpectProvisioned(ctx, env.Client, cluster, cloudProvider, prov, + test.UnschedulablePods(test.PodOptions{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + NodeSelector: map[string]string{v1.NodePoolLabelKey: nodePool.Name}, + TopologySpreadConstraints: topology, + }, 4)..., + ) + ExpectSkew(ctx, env.Client, "default", &topology[0]).To(ConsistOf(2, 2)) + }) It("should respect NodePool zonal constraints (subset) with labels", func() { nodePool.Spec.Template.Labels = lo.Assign(nodePool.Spec.Template.Labels, map[string]string{corev1.LabelTopologyZone: "test-zone-1"}) topology := []corev1.TopologySpreadConstraint{{ diff --git a/pkg/controllers/provisioning/scheduling/topologydomaingroup.go b/pkg/controllers/provisioning/scheduling/topologydomaingroup.go index 680ad15fa3..6fa7ee9e18 100644 --- a/pkg/controllers/provisioning/scheduling/topologydomaingroup.go +++ b/pkg/controllers/provisioning/scheduling/topologydomaingroup.go @@ -22,51 +22,60 @@ import ( "sigs.k8s.io/karpenter/pkg/scheduling" ) -// TopologyDomainGroup tracks the domains for a single topology. Additionally, it tracks the taints associated with -// each of these domains. This enables us to determine which domains should be considered by a pod if its -// NodeTaintPolicy is honor. -type TopologyDomainGroup map[string][][]v1.Taint +// TopologyDomainSource is a NodePool that can supply a domain, along with the taints and +// requirements a node launched from that NodePool would carry. Taints answer whether a pod with +// NodeTaintsPolicy honor counts the domain; requirements answer the same for NodeAffinityPolicy. +type TopologyDomainSource struct { + Taints []v1.Taint + Requirements scheduling.Requirements +} + +// TopologyDomainGroup tracks the domains for a single topology, keyed by domain and then by the +// name of each NodePool that can supply it. +type TopologyDomainGroup map[string]map[string]TopologyDomainSource func NewTopologyDomainGroup() TopologyDomainGroup { - return map[string][][]v1.Taint{} + return TopologyDomainGroup{} } -// Insert either adds a new domain to the TopologyDomainGroup or updates an existing domain. -func (t TopologyDomainGroup) Insert(domain string, taints ...v1.Taint) { - // If the domain is not currently tracked, insert it with the associated taints. Additionally, if there are no taints - // provided, override the taints associated with the domain. Generally, we could remove any sets of taints for which - // the provided set is a proper subset. This is because if a pod tolerates the supersets, it will also tolerate the - // proper subset, and removing the superset reduces the number of taint sets we need to traverse. For now we only - // implement the simplest case, the empty set, but we could do additional performance testing to determine if - // implementing the general case is worth the precomputation cost. - if _, ok := t[domain]; !ok || len(taints) == 0 { - t[domain] = [][]v1.Taint{taints} - return +// Insert records that nodePool can supply domain, on nodes carrying the given taints and +// requirements. +func (t TopologyDomainGroup) Insert(domain string, nodePool string, taints []v1.Taint, requirements scheduling.Requirements) { + sources, ok := t[domain] + if !ok { + sources = map[string]TopologyDomainSource{} + t[domain] = sources } - if len(t[domain][0]) == 0 { - // This is the base case, where we're already tracking the empty set of taints for the domain. Pods will always - // be eligible for NodeClaims with this domain (based on taints), so there is no need to track additional taints. - return - } - t[domain] = append(t[domain], taints) + sources[nodePool] = TopologyDomainSource{Taints: taints, Requirements: requirements} } -// ForEachDomain calls f on each domain tracked by the topology group. If the taintHonorPolicy is honor, only domains -// available on nodes tolerated by the provided pod will be included. -func (t TopologyDomainGroup) ForEachDomain(pod *v1.Pod, taintHonorPolicy v1.NodeInclusionPolicy, f func(domain string)) { - for domain, taintGroups := range t { - if taintHonorPolicy == v1.NodeInclusionPolicyIgnore { - f(domain) - continue - } - // Since the taint policy is honor, we should only call f if there is a set of taints associated with the domain which - // the pod tolerates. - // Perf Note: We could consider hashing the pod's tolerations and using that to look up a set of tolerated domains. - for _, taints := range taintGroups { - if err := scheduling.Taints(taints).ToleratesPod(pod); err == nil { - f(domain) - break +// ForEachDomain calls f on each domain tracked by the topology group that the pod could actually +// land in, given at least one NodePool supplying that domain. If the taint policy is honor, the pod +// must tolerate that NodePool's taints; if the affinity policy is honor, the pod's node selector and +// required node affinity must not conflict with the NodePool's requirements. A domain is only +// dropped on an outright conflict, so a pod selecting a label that the NodePool leaves to its +// instance types keeps the domain. +// +// Honoring affinity here is what keeps a spread's global minimum meaningful in a cluster whose +// NodePools do not all offer the same domains. A pod pinned to one NodePool would otherwise count +// every domain reachable only through the other pools (for example the zones of a pool spanning +// another region), each with a pod count of zero, pinning the global minimum at zero and leaving no +// domain within maxSkew of it, so a DoNotSchedule spread could never be satisfied. +func (t TopologyDomainGroup) ForEachDomain(pod *v1.Pod, nodeFilter TopologyNodeFilter, f func(domain string)) { + for domain, sources := range t { + for _, source := range sources { + if nodeFilter.TaintPolicy != v1.NodeInclusionPolicyIgnore { + // Perf Note: We could consider hashing the pod's tolerations and using that to look up a set of + // tolerated domains. + if err := scheduling.Taints(source.Taints).ToleratesPod(pod); err != nil { + continue + } + } + if nodeFilter.AffinityPolicy == v1.NodeInclusionPolicyHonor && nodeFilter.ConflictsWithRequirements(source.Requirements) { + continue } + f(domain) + break } } } diff --git a/pkg/controllers/provisioning/scheduling/topologydomaingroup_internal_test.go b/pkg/controllers/provisioning/scheduling/topologydomaingroup_internal_test.go new file mode 100644 index 0000000000..66f7fc448e --- /dev/null +++ b/pkg/controllers/provisioning/scheduling/topologydomaingroup_internal_test.go @@ -0,0 +1,221 @@ +/* +Copyright The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package scheduling + +import ( + "slices" + "sort" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" + + v1 "sigs.k8s.io/karpenter/pkg/apis/v1" + "sigs.k8s.io/karpenter/pkg/cloudprovider" + "sigs.k8s.io/karpenter/pkg/scheduling" +) + +func zonalNodePool(name string, zones []string, taints ...corev1.Taint) *v1.NodePool { + return &v1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: v1.NodePoolSpec{ + Template: v1.NodeClaimTemplate{ + Spec: v1.NodeClaimTemplateSpec{ + Taints: taints, + Requirements: []v1.NodeSelectorRequirementWithMinValues{{ + Key: corev1.LabelTopologyZone, + Operator: corev1.NodeSelectorOpIn, + Values: zones, + }}, + }, + }, + }, + } +} + +func zonalInstanceTypes(zones []string) []*cloudprovider.InstanceType { + return []*cloudprovider.InstanceType{{ + Name: "default-instance-type", + Requirements: scheduling.NewRequirements( + scheduling.NewRequirement(corev1.LabelTopologyZone, corev1.NodeSelectorOpIn, zones...), + ), + }} +} + +func zoneSpreadPod(nodeSelector map[string]string, tolerations ...corev1.Toleration) *corev1.Pod { + return &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Labels: map[string]string{"app": "mimir"}}, + Spec: corev1.PodSpec{ + NodeSelector: nodeSelector, + Tolerations: tolerations, + TopologySpreadConstraints: []corev1.TopologySpreadConstraint{{ + TopologyKey: corev1.LabelTopologyZone, + WhenUnsatisfiable: corev1.DoNotSchedule, + MaxSkew: 1, + LabelSelector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "mimir"}}, + }}, + }, + } +} + +func spreadDomains(t *testing.T, pod *corev1.Pod, nodePools []*v1.NodePool, taintPolicy *corev1.NodeInclusionPolicy) []string { + t.Helper() + instanceTypes := map[string][]*cloudprovider.InstanceType{} + for _, np := range nodePools { + zones := scheduling.NewNodeSelectorRequirementsWithMinValues(np.Spec.Template.Spec.Requirements...).Get(corev1.LabelTopologyZone) + instanceTypes[np.Name] = zonalInstanceTypes(zones.Values()) + } + domainGroups := buildDomainGroups(nodePools, instanceTypes) + group := NewTopologyGroup( + TopologyTypeSpread, + corev1.LabelTopologyZone, + pod, + sets.New(pod.Namespace), + pod.Spec.TopologySpreadConstraints[0].LabelSelector, + pod.Spec.TopologySpreadConstraints[0].MaxSkew, + nil, + taintPolicy, + nil, + domainGroups[corev1.LabelTopologyZone], + ) + domains := sets.List(sets.KeySet(group.domains)) + sort.Strings(domains) + return domains +} + +// A pod pinned to one NodePool must not have the domains of NodePools it can never select counted +// in its spread: those domains hold no pods, so they would hold the group's minimum at zero and +// leave every reachable domain outside maxSkew, making the spread permanently unsatisfiable. +func TestTopologyGroupSpreadSkipsUnselectableNodePoolDomains(t *testing.T) { + nodePools := []*v1.NodePool{ + zonalNodePool("monitoring", []string{"us-west-2a", "us-west-2b", "us-west-2c"}), + zonalNodePool("accelerators", []string{"us-west-2a", "ap-northeast-1a"}), + } + pod := zoneSpreadPod(map[string]string{v1.NodePoolLabelKey: "monitoring"}) + + domains := spreadDomains(t, pod, nodePools, nil) + if want := []string{"us-west-2a", "us-west-2b", "us-west-2c"}; !slices.Equal(domains, want) { + t.Fatalf("expected only the domains of the selected nodepool %v, got %v", want, domains) + } +} + +func TestTopologyGroupSpreadCountsAllDomainsWithoutNodeSelector(t *testing.T) { + nodePools := []*v1.NodePool{ + zonalNodePool("monitoring", []string{"us-west-2a", "us-west-2b"}), + zonalNodePool("accelerators", []string{"ap-northeast-1a"}), + } + pod := zoneSpreadPod(nil) + + domains := spreadDomains(t, pod, nodePools, nil) + if want := []string{"ap-northeast-1a", "us-west-2a", "us-west-2b"}; !slices.Equal(domains, want) { + t.Fatalf("expected every domain %v, got %v", want, domains) + } +} + +// Required node affinity restricts the domains the same way a node selector does, since both are +// honored by the default NodeAffinityPolicy. +func TestTopologyGroupSpreadHonorsRequiredNodeAffinity(t *testing.T) { + nodePools := []*v1.NodePool{ + zonalNodePool("monitoring", []string{"us-west-2a", "us-west-2b"}), + zonalNodePool("accelerators", []string{"ap-northeast-1a"}), + } + pod := zoneSpreadPod(nil) + pod.Spec.Affinity = &corev1.Affinity{NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{NodeSelectorTerms: []corev1.NodeSelectorTerm{{ + MatchExpressions: []corev1.NodeSelectorRequirement{{ + Key: v1.NodePoolLabelKey, + Operator: corev1.NodeSelectorOpIn, + Values: []string{"monitoring"}, + }}, + }}}, + }} + + domains := spreadDomains(t, pod, nodePools, nil) + if want := []string{"us-west-2a", "us-west-2b"}; !slices.Equal(domains, want) { + t.Fatalf("expected only the domains of the affine nodepool %v, got %v", want, domains) + } +} + +// A NodePool leaves some labels to whichever instance type gets launched, so a pod selecting one of +// those keys must not lose the pool's domains: only an outright conflict drops a domain. +func TestTopologyGroupSpreadKeepsDomainsForInstanceTypeOnlyLabels(t *testing.T) { + nodePools := []*v1.NodePool{zonalNodePool("monitoring", []string{"us-west-2a", "us-west-2b"})} + pod := zoneSpreadPod(map[string]string{"example.com/flavor": "gpu"}) + + instanceTypes := map[string][]*cloudprovider.InstanceType{"monitoring": {{ + Name: "gpu-instance-type", + Requirements: scheduling.NewRequirements( + scheduling.NewRequirement(corev1.LabelTopologyZone, corev1.NodeSelectorOpIn, "us-west-2a", "us-west-2b"), + scheduling.NewRequirement("example.com/flavor", corev1.NodeSelectorOpIn, "gpu"), + ), + }}} + domainGroups := buildDomainGroups(nodePools, instanceTypes) + group := NewTopologyGroup(TopologyTypeSpread, corev1.LabelTopologyZone, pod, sets.New(pod.Namespace), + pod.Spec.TopologySpreadConstraints[0].LabelSelector, 1, nil, nil, nil, domainGroups[corev1.LabelTopologyZone]) + + domains := sets.List(sets.KeySet(group.domains)) + if want := []string{"us-west-2a", "us-west-2b"}; !slices.Equal(domains, want) { + t.Fatalf("expected the instance types' domains %v, got %v", want, domains) + } +} + +// An ignored NodeAffinityPolicy opts out of the filtering entirely, as it does upstream. +func TestTopologyGroupSpreadIgnoredAffinityPolicyCountsAllDomains(t *testing.T) { + nodePools := []*v1.NodePool{ + zonalNodePool("monitoring", []string{"us-west-2a"}), + zonalNodePool("accelerators", []string{"ap-northeast-1a"}), + } + pod := zoneSpreadPod(map[string]string{v1.NodePoolLabelKey: "monitoring"}) + ignore := corev1.NodeInclusionPolicyIgnore + pod.Spec.TopologySpreadConstraints[0].NodeAffinityPolicy = &ignore + + instanceTypes := map[string][]*cloudprovider.InstanceType{} + for _, np := range nodePools { + zones := scheduling.NewNodeSelectorRequirementsWithMinValues(np.Spec.Template.Spec.Requirements...).Get(corev1.LabelTopologyZone) + instanceTypes[np.Name] = zonalInstanceTypes(zones.Values()) + } + domainGroups := buildDomainGroups(nodePools, instanceTypes) + group := NewTopologyGroup(TopologyTypeSpread, corev1.LabelTopologyZone, pod, sets.New(pod.Namespace), + pod.Spec.TopologySpreadConstraints[0].LabelSelector, 1, nil, nil, &ignore, domainGroups[corev1.LabelTopologyZone]) + + domains := sets.List(sets.KeySet(group.domains)) + if want := []string{"ap-northeast-1a", "us-west-2a"}; !slices.Equal(domains, want) { + t.Fatalf("expected every domain %v, got %v", want, domains) + } +} + +// Taint filtering keeps working, including when it is the only thing excluding a domain. +func TestTopologyGroupSpreadHonorsTaints(t *testing.T) { + taint := corev1.Taint{Key: "accelerator", Value: "true", Effect: corev1.TaintEffectNoSchedule} + nodePools := []*v1.NodePool{ + zonalNodePool("monitoring", []string{"us-west-2a"}), + zonalNodePool("accelerators", []string{"us-west-2b"}, taint), + } + honor := corev1.NodeInclusionPolicyHonor + + domains := spreadDomains(t, zoneSpreadPod(nil), nodePools, &honor) + if want := []string{"us-west-2a"}; !slices.Equal(domains, want) { + t.Fatalf("expected the tainted nodepool's domain to be skipped, got %v", domains) + } + + tolerating := zoneSpreadPod(nil, corev1.Toleration{Key: "accelerator", Operator: corev1.TolerationOpExists}) + domains = spreadDomains(t, tolerating, nodePools, &honor) + if want := []string{"us-west-2a", "us-west-2b"}; !slices.Equal(domains, want) { + t.Fatalf("expected a tolerating pod to keep every domain %v, got %v", want, domains) + } +} diff --git a/pkg/controllers/provisioning/scheduling/topologygroup.go b/pkg/controllers/provisioning/scheduling/topologygroup.go index 13db69676c..15ef6c8648 100644 --- a/pkg/controllers/provisioning/scheduling/topologygroup.go +++ b/pkg/controllers/provisioning/scheduling/topologygroup.go @@ -105,7 +105,7 @@ func NewTopologyGroup( domains := map[string]int32{} emptyDomains := sets.New[string]() - domainGroup.ForEachDomain(pod, nodeFilter.TaintPolicy, func(domain string) { + domainGroup.ForEachDomain(pod, nodeFilter, func(domain string) { domains[domain] = 0 emptyDomains.Insert(domain) }) diff --git a/pkg/controllers/provisioning/scheduling/topologynodefilter.go b/pkg/controllers/provisioning/scheduling/topologynodefilter.go index 07fe888c5e..8dcac086e4 100644 --- a/pkg/controllers/provisioning/scheduling/topologynodefilter.go +++ b/pkg/controllers/provisioning/scheduling/topologynodefilter.go @@ -79,6 +79,23 @@ func (t TopologyNodeFilter) Matches(taints []corev1.Taint, requirements scheduli return matchesAffinity && matchesTaints } +// ConflictsWithRequirements returns true if no term of the pod's required node affinity can be met by a node with +// the given requirements. Unlike matchesRequirements it only reports outright conflicts: a key the requirements +// leave undefined isn't a mismatch, since a node's labels are also decided by the instance type picked at launch. +// This is the check to use against a NodePool rather than an actual node or a NodeClaim. +func (t TopologyNodeFilter) ConflictsWithRequirements(requirements scheduling.Requirements) bool { + if len(t.Requirements) == 0 || t.AffinityPolicy == corev1.NodeInclusionPolicyIgnore { + return false + } + // these are an OR, so the filter only conflicts if every term does + for _, req := range t.Requirements { + if requirements.Intersects(req) == nil { + return false + } + } + return true +} + // MatchesRequirements returns true if the TopologyNodeFilter doesn't prohibit a node with the requirements from // participating in the topology. This method allows checking the requirements from a scheduling.NodeClaim to see if the // node we will soon create participates in this topology.