Implement MinDomains on Pod Topology Spread #108362

sanposhiho · 2022-02-26T06:08:04Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implement MinDomains and tests

This PR contains the changes on #107674, so should be merged after #107674. The changes in this PR is only f3bd5e5
/hold

Which issue(s) this PR fixes:

Part of #105291

Special notes for your reviewer:

Should I add e2e tests in this PR?

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/3022

sanposhiho · 2022-02-26T06:16:55Z

/cc @alculquicondor

sanposhiho · 2022-02-26T11:56:27Z

/retest

kerthcet · 2022-02-28T09:10:33Z

pkg/scheduler/framework/plugins/podtopologyspread/filtering.go

@@ -57,11 +59,15 @@ func (s *preFilterState) Clone() framework.StateData {
 		// Constraints are shared because they don't change.
 		Constraints:          s.Constraints,
 		TpKeyToCriticalPaths: make(map[string]*criticalPaths, len(s.TpKeyToCriticalPaths)),
+		TpKeyToDomainsNum:    make(map[string]int32, len(s.TpKeyToDomainsNum)),


Any reason we don't use int as TpPairToMatchNum below?

kerthcet · 2022-02-28T09:17:41Z

pkg/scheduler/framework/plugins/podtopologyspread/filtering.go

 		minMatchNum := paths[0].MatchNum
+


Would it be readable to arrange this to a function like MinMatchNum()? It can be a method of preFilterState.

+1. You can move this logic to MinMatchNum(), and return (int, error) so that the detailed error message can be composed properly.

kerthcet · 2022-02-28T09:31:12Z

pkg/apis/core/types.go

+	// When value is different from 1, WhenUnsatisfiable must be DoNotSchedule.
+	//
+	// For example, in a 3-zone cluster, MaxSkew is set to 2, MinDomains is set to 5 and pods with the same
+	// labelSelector spread as 2/2/2:


I was wondering what will happen if we set MaxSkew here to 10.

This is described in the above part. (L5666)

// As a result, when the number of eligible domains is less than minDomains,
// scheduler won't schedule more than maxSkew Pods to those domains.

Even if we set MaxSkew=10 (and the other parameters and situation are the same as this example), global minimum is treated as 0. (because the number of domains is less than 5(MinDomains))
But, before spreading pods as 10/10/10, scheduling will not fail as the example.
In Filter, we judged the Nodes with the following criteria,

('existing matching num' + 'if self-match (1 or 0)' - 'global min matching num') <= 'maxSkew'

and, before spreading pods as 10/10/10, x + 1 - 0 <= 10 will be true in at least one Node .

Do you think that the current documentation is not enough? If this documentation is difficult to understand for someone who understands scheduler (includes you), then many users also may not be able to understand this.

So, even we have less eligible domains than MinDomains, we can still schedule pods until we reach the Skew limit, right? I think this was the point I'm confusing. I thought MinDomains was a hard constraint. I wonder whether this will also be mis-understood by end-users.

Yes, your understanding is right.
Small change, but I added the statement that skew is calculated after MinDomains are taken into account in 03a6294.

@kerthcet this was discussed in the original KEP, and had a consensus that the global minimum is read as 0 (like a virtual topology not existing yet) if the total number of topologies is less than minDomains.

alculquicondor · 2022-02-28T16:46:36Z

Will review after @kerthcet gives an LGTM

kerthcet · 2022-03-01T07:59:34Z

pkg/api/pod/util_test.go

+		},
+	}
+	for _, tc := range testCases {
+		tc := tc


kerthcet · 2022-03-01T08:10:27Z

pkg/apis/core/types.go

+	// The number of domains is less than 5(MinDomains), so "global minimum" is treated as 0.
+	// In this situation, new pod with the same labelSelector cannot be scheduled,
+	// because computed skew will be 3(3 - 0) if new Pod is scheduled to any of the three zones,
+	// it will violate MinDomains.


s/it will violate MinDomains/it will violate MaxSkew/

kerthcet · 2022-03-01T08:41:51Z

pkg/apis/core/validation/validation.go

@@ -6518,6 +6518,9 @@ func validateTopologySpreadConstraints(constraints []core.TopologySpreadConstrai
 		if err := ValidateMaxSkew(subFldPath.Child("maxSkew"), constraint.MaxSkew); err != nil {
 			allErrs = append(allErrs, err)
 		}
+		if err := validateMinDomains(subFldPath.Child("minDomains"), constraint.MinDomains); err != nil {
+			allErrs = append(allErrs, err)
+		}
 		if err := ValidateTopologyKey(subFldPath.Child("topologyKey"), constraint.TopologyKey); err != nil {


Add validations for MinDomains!=1 and WhenUnsatisfiable=ScheduleAnyway combination to fail fast at api level? Tests also.

This has been implemented in #107674. (Sorry I hadn't rebased this PR branch on latest #107674 branch)

kerthcet · 2022-03-01T08:54:38Z

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go

@@ -633,13 +634,13 @@ func TestDryRunPreemption(t *testing.T) {
 		{
 			name: "preemption to resolve pod topology spread filter failure",
 			registerPlugins: []st.RegisterPluginFunc{
-				st.RegisterPluginAsExtensions(podtopologyspread.Name, podtopologyspread.New, "PreFilter", "Filter"),
+				st.RegisterPluginAsExtensions(podtopologyspread.Name, schedruntime.FactoryAdapter(feature.Features{}, podtopologyspread.New), "PreFilter", "Filter"),


Let's name var podTopologySpreadFunc = schedruntime.FactoryAdapter(feature.Features{}, podtopologyspread.New) at the file header and reuse it.

kerthcet · 2022-03-01T10:04:21Z

pkg/scheduler/framework/plugins/podtopologyspread/filtering.go

+		if !ok {
+			// error which should not happen.
+			// If this error occurs, MinDomains will be ignored in judge.
+			klog.ErrorS(nil, "Internal error occurred while retrieving the number of eligible domains from topology key. MinDomains will be ignored.", "topologyKey", tpKey, "domainsNums", s.TpKeyToDomainsNum)


s/from topology key/by topology key/

kerthcet · 2022-03-01T10:27:28Z

pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go

@@ -1295,142 +1405,65 @@ func TestSingleConstraint(t *testing.T) {
 			},
 		},
 		{
-			name: "pods spread across nodes as 2/1/0/3, maxSkew is 2, node-b and node-x fit",


Why remove these tests? Seems like these tests can work well if we set proper MinDomains.

Deletion seems to be a mistake. Will revert this change. Thanks

kerthcet · 2022-03-01T10:32:26Z

pkg/scheduler/framework/plugins/podtopologyspread/scoring_test.go

@@ -732,7 +742,7 @@ func TestPodTopologySpreadScore(t *testing.T) {
 			allNodes := append([]*v1.Node{}, tt.nodes...)
 			allNodes = append(allNodes, tt.failedNodes...)
 			state := framework.NewCycleState()
-			pl := plugintesting.SetupPluginWithInformers(ctx, t, New, &config.PodTopologySpreadArgs{DefaultingType: config.SystemDefaulting}, cache.NewSnapshot(tt.existingPods, allNodes), tt.objs)
+			pl := plugintesting.SetupPluginWithInformers(ctx, t, frameworkruntime.FactoryAdapter(feature.Features{}, New), &config.PodTopologySpreadArgs{DefaultingType: config.SystemDefaulting}, cache.NewSnapshot(tt.existingPods, allNodes), tt.objs)


Declare var podTopologySpreadFunc frameworkruntime.FactoryAdapter(feature.Features{}, New) for reuse.

kerthcet · 2022-03-01T11:03:00Z

pkg/scheduler/framework/plugins/podtopologyspread/filtering.go

+		if !ok {
+			// error which should not happen.
+			// If this error occurs, MinDomains will be ignored in judge.
+			klog.ErrorS(nil, "Internal error occurred while retrieving the number of eligible domains from topology key. MinDomains will be ignored.", "topologyKey", tpKey, "domainsNums", s.TpKeyToDomainsNum)


domainsNum here will always be 0, the same as minMatchNum, this is not right. Change to something like:

if domainsNum, ok := s.TpKeyToDomainsNum[c.TopologyKey]; ok { if domainsNum < c.MinDomains { minMatchNum = 0 } } else { // When the number of eligible domains with matching topology keys is less than `minDomains`, // it treats "global minimum" as 0. klog.ErrorS(nil, "Internal error occurred while retrieving the number of eligible domains from topology key. MinDomains will be ignored.", "topologyKey", tpKey, "domainsNums", s.TpKeyToDomainsNum) }

Thanks for the nice catch 🙏

alculquicondor · 2022-03-01T16:40:28Z

Should I add e2e tests in this PR?

We prefer integration tests were possible, because they run faster

sanposhiho · 2022-03-11T11:44:46Z

Thanks. squashed the commits into 2 commits.
And, change the release notes as suggestion.

alculquicondor · 2022-03-14T13:19:39Z

/lgtm
/approve
/hold

sanposhiho · 2022-03-14T16:42:36Z

@alculquicondor rebased this pr.

alculquicondor · 2022-03-14T17:00:44Z

/approve
/lgtm

k8s-ci-robot · 2022-03-14T17:01:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanposhiho

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:

~~pkg/scheduler/OWNERS~~ [alculquicondor]
~~test/integration/scheduler/OWNERS~~ [alculquicondor]

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

sanposhiho · 2022-03-15T00:01:09Z

/retest

sanposhiho · 2022-03-15T00:39:34Z

@alculquicondor Could you /unhold if looks good?

alculquicondor · 2022-03-15T13:13:22Z

/hold cancel

alculquicondor · 2022-03-15T13:50:19Z

/retest

🤔 I'm not seeing other failures for TestVolumeBinding/bound_and_unbound_unsatisfied in https://storage.googleapis.com/k8s-triage/index.html?date=2022-03-06&pr=1&text=TestVolumeBinding&job=pull-kubernetes-unit

If you see it again, please open an issue.

alculquicondor · 2022-03-15T14:22:20Z

/retest

k8s-triage-robot · 2022-03-15T15:18:19Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

sanposhiho · 2022-03-15T16:53:48Z

/retest

k8s-ci-robot requested review from ahg-g and caesarxuchao February 26, 2022 06:09

sanposhiho mentioned this pull request Feb 26, 2022

Add MinDomains API to TopologySpreadConstraints field #107674

Merged

k8s-ci-robot requested a review from alculquicondor February 26, 2022 06:16

sanposhiho force-pushed the implement-mindomains branch from e6f3111 to 904c94f Compare February 26, 2022 07:05

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

kerthcet reviewed Feb 28, 2022

View reviewed changes

kerthcet reviewed Mar 1, 2022

View reviewed changes

sanposhiho force-pushed the implement-mindomains branch from 904c94f to bd3c551 Compare March 1, 2022 18:04

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

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 11, 2022

k8s-ci-robot assigned alculquicondor Mar 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2022

Implement MinDomains

6141aa5

sanposhiho force-pushed the implement-mindomains branch from a1e98ee to 6141aa5 Compare March 14, 2022 16:39

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2022

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

k8s-ci-robot merged commit 2b1b849 into kubernetes:master Mar 15, 2022

k8s-ci-robot added this to the v1.24 milestone Mar 15, 2022

sanposhiho mentioned this pull request Mar 16, 2022

Min domains in PodTopologySpread kubernetes/enhancements#3022

Closed

12 tasks

alculquicondor mentioned this pull request May 4, 2022

Flaky scheduler integration tests #109783

Closed

kerthcet mentioned this pull request May 27, 2023

Add kerthcet and sanposhiho as SIG scheduling approvers #118294

Merged

Implement MinDomains on Pod Topology Spread #108362

Implement MinDomains on Pod Topology Spread #108362

Conversation

sanposhiho commented Feb 26, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

sanposhiho commented Feb 26, 2022

sanposhiho commented Feb 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Feb 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Mar 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Mar 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Mar 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet Mar 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Mar 1, 2022 • edited

sanposhiho commented Mar 11, 2022 • edited

alculquicondor commented Mar 14, 2022

sanposhiho commented Mar 14, 2022 • edited

alculquicondor commented Mar 14, 2022

k8s-ci-robot commented Mar 14, 2022

sanposhiho commented Mar 15, 2022

sanposhiho commented Mar 15, 2022

alculquicondor commented Mar 15, 2022

alculquicondor commented Mar 15, 2022

alculquicondor commented Mar 15, 2022

k8s-triage-robot commented Mar 15, 2022

sanposhiho commented Mar 15, 2022

sanposhiho commented Feb 26, 2022 •

edited

sanposhiho Feb 28, 2022 •

edited

sanposhiho Mar 1, 2022 •

edited

sanposhiho Mar 1, 2022 •

edited

sanposhiho Mar 1, 2022 •

edited

kerthcet Mar 1, 2022 •

edited

alculquicondor commented Mar 1, 2022 •

edited

sanposhiho commented Mar 11, 2022 •

edited

sanposhiho commented Mar 14, 2022 •

edited