feat: support new topologySpread scheduling constraints #852

jmdeal · 2023-12-08T19:28:43Z

Fixes #430

Description
This PR adds support for the following topology spread constraint fields:

matchLabelKeys
nodeAffinityPolicy
nodeTaintsPolicy

How was this change tested?
make test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

coveralls · 2023-12-08T19:43:34Z

Pull Request Test Coverage Report for Build 13251614430

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

210 of 213 (98.59%) changed or added relevant lines in 10 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.1%) to 81.467%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controllers/provisioning/scheduling/topology.go	106	109	97.25%

Files with Coverage Reduction	New Missed Lines	%
pkg/controllers/provisioning/scheduling/topologygroup.go	2	97.85%
pkg/controllers/provisioning/scheduling/topologynodefilter.go	2	95.45%

Totals
Change from base Build 13211152753:	0.1%
Covered Lines:	9249
Relevant Lines:	11353

💛 - Coveralls

pkg/controllers/provisioning/scheduling/topology_test.go

pkg/controllers/provisioning/scheduling/topologydomaingroup.go

pkg/controllers/provisioning/scheduling/topologygroup.go

jmdeal · 2023-12-13T18:14:20Z

Holding to include matchLabelKeys for pod affinity with k8s v1.29.

jonathan-innis · 2025-02-05T01:34:14Z

This is awesome work! This is definitely getting close! I think mostly a few small things -- we should write-down things that we want to refactor here since there were some ideas thrown out about how we could move existing nodes into topologyGroups and avoid iterating through stateNodes in countDomains -- also some things around naming of functions -- also also capturing nodeAffinities in eligible domains for topologyDomainGroups

jonathan-innis

Nice work!

pkg/controllers/provisioning/scheduling/topology.go

pkg/controllers/provisioning/scheduling/scheduling_benchmark_test.go

jonathan-innis · 2025-02-11T00:47:58Z

pkg/controllers/provisioning/scheduling/topologydomaingroup.go

+	"sigs.k8s.io/karpenter/pkg/scheduling"
+)
+
+// TopologyDomainGroup tracks the domains for a single topology. Additionally, it tracks the taints associated with


Yeah, I think my confusion here was just based around when this was used -- this is only used when constructing the topologyGroup -- it's not actually used when we are doing any counting since we should have already discovered all of the available domains at the top of the loop

jonathan-innis · 2025-02-11T00:58:28Z

pkg/controllers/provisioning/scheduling/topologynodefilter.go

 	}

 	return filter
 }

 // Matches returns true if the TopologyNodeFilter doesn't prohibit node from the participating in the topology
-func (t TopologyNodeFilter) Matches(node *v1.Node) bool {
-	return t.MatchesRequirements(scheduling.NewLabelRequirements(node.Labels))
+func (t TopologyNodeFilter) Matches(taints []corev1.Taint, requirements scheduling.Requirements, compatibilityOptions ...option.Function[scheduling.CompatibilityOptions]) bool {


Can you confirm if these compatibility options are still needed? Because the pod requirements are being checked against the node requirements, I believe there is a hard requirement that all of the nodeRequirements must exist when evaluating Counts (undefined doesn't mean it should count since this is really only true for NodeClaims that are still in-flight and we've already discussed that it's already too complicated to try and figure out if this is right or not). Anyways, the current behavior is that an undefined label doesn't mean the pod is compatible with it and I don't think that we should drop or change that behavior

Right now we only allow missing WellKnownLabels when evaluating if an inflight NodeClaim should count against a domain. I don't think we should drop that here because that would result in us not counting a nodeclaim for a topology group when we should have. I think this is the right side for us to error on - being to conservative and overcounting NodeClaims rather than undercounting them.

pkg/controllers/provisioning/scheduling/topology_test.go

jonathan-innis

/lgtm
/approve

k8s-ci-robot · 2025-02-11T22:10:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmdeal, jonathan-innis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jonathan-innis]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jonathan-innis · 2025-02-11T22:10:49Z

/hold

Wait for confirmation from @jmdeal that this one is good to merge

jmdeal · 2025-02-12T18:36:11Z

Should be good to go 👍

jmdeal · 2025-02-12T18:36:19Z

/remove-hold

codeeong · 2025-02-13T10:09:40Z

When will a new release with this change be rolled out? Thanks!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 8, 2023

k8s-ci-robot requested review from engedaam and jackfrancis December 8, 2023 19:28

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 8, 2023

jmdeal changed the title ~~feat: support new topologySpread scheduling constraints~~ [WIP] feat: support new topologySpread scheduling constraints Dec 8, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2023

jmdeal force-pushed the support-affinity-and-taint-policy branch from aed9f49 to 54af319 Compare December 8, 2023 19:34

jmdeal force-pushed the support-affinity-and-taint-policy branch 7 times, most recently from 4329131 to f0127f2 Compare December 9, 2023 01:46

tzneal reviewed Dec 9, 2023

View reviewed changes

pkg/controllers/provisioning/scheduling/topology_test.go Outdated Show resolved Hide resolved

tzneal reviewed Dec 9, 2023

View reviewed changes

pkg/controllers/provisioning/scheduling/topologydomaingroup.go Outdated Show resolved Hide resolved

tzneal reviewed Dec 9, 2023

View reviewed changes

pkg/controllers/provisioning/scheduling/topologygroup.go Outdated Show resolved Hide resolved

jmdeal force-pushed the support-affinity-and-taint-policy branch 3 times, most recently from b23a605 to 0408162 Compare December 11, 2023 21:59

jmdeal changed the title ~~[WIP] feat: support new topologySpread scheduling constraints~~ feat: support new topologySpread scheduling constraints Dec 11, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2023

jmdeal force-pushed the support-affinity-and-taint-policy branch from 0408162 to 6673b5b Compare December 11, 2023 23:14

jmdeal changed the title ~~feat: support new topologySpread scheduling constraints~~ [WIP] feat: support new topologySpread scheduling constraints Dec 13, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2023

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 14, 2023

jmdeal mentioned this pull request Dec 14, 2023

docs: remove topologySpreadConstraints compatibility note aws/karpenter-provider-aws#5336

Closed

3 tasks

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 10, 2025

jmdeal force-pushed the support-affinity-and-taint-policy branch from da3c39a to 3feab07 Compare February 10, 2025 22:21

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2025

jmdeal force-pushed the support-affinity-and-taint-policy branch from 3feab07 to 3441351 Compare February 10, 2025 22:43

tzneal and others added 7 commits February 10, 2025 15:55

support node affinity and taints policies for topology spread

d494683

correctly handle domains from NodePools when honoring taints

9d7aa88

support matchLabelKeys for topology spread

fc36483

inject expression, rather than overwrite label

9066785

remaining feedback

8e0f061

review updates + record fix

f869658

updated comment

58005bf

jmdeal force-pushed the support-affinity-and-taint-policy branch from dd3ce78 to 58005bf Compare February 10, 2025 23:55

jonathan-innis reviewed Feb 11, 2025

View reviewed changes

more review updates

19836eb

jmdeal force-pushed the support-affinity-and-taint-policy branch from e42e1c8 to 19836eb Compare February 11, 2025 10:10

jonathan-innis approved these changes Feb 11, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2025

k8s-ci-robot merged commit 45f73ec into kubernetes-sigs:main Feb 12, 2025
13 checks passed

codeeong mentioned this pull request Feb 13, 2025

Support new topologySpread scheduling constraints #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support new topologySpread scheduling constraints #852

feat: support new topologySpread scheduling constraints #852

jmdeal commented Dec 8, 2023 •

edited

Loading

coveralls commented Dec 8, 2023 •

edited

Loading

jmdeal commented Dec 13, 2023

jonathan-innis commented Feb 5, 2025

jonathan-innis left a comment

jonathan-innis Feb 11, 2025

jonathan-innis Feb 11, 2025

jmdeal Feb 11, 2025

jonathan-innis left a comment

k8s-ci-robot commented Feb 11, 2025

jonathan-innis commented Feb 11, 2025

jmdeal commented Feb 12, 2025

jmdeal commented Feb 12, 2025

codeeong commented Feb 13, 2025

feat: support new topologySpread scheduling constraints #852

feat: support new topologySpread scheduling constraints #852

Conversation

jmdeal commented Dec 8, 2023 • edited Loading

coveralls commented Dec 8, 2023 • edited Loading

Pull Request Test Coverage Report for Build 13251614430

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jmdeal commented Dec 13, 2023

jonathan-innis commented Feb 5, 2025

jonathan-innis left a comment

Choose a reason for hiding this comment

jonathan-innis Feb 11, 2025

Choose a reason for hiding this comment

jonathan-innis Feb 11, 2025

Choose a reason for hiding this comment

jmdeal Feb 11, 2025

Choose a reason for hiding this comment

jonathan-innis left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 11, 2025

jonathan-innis commented Feb 11, 2025

jmdeal commented Feb 12, 2025

jmdeal commented Feb 12, 2025

codeeong commented Feb 13, 2025

jmdeal commented Dec 8, 2023 •

edited

Loading

coveralls commented Dec 8, 2023 •

edited

Loading