register unschedulable plugin for those plugins that PreFilter's PreFilterResult filter out some nodes #122251

olderTaoist · 2023-12-10T13:30:57Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

unschedulable plugin isn't registered for those plugins, when some PreFilter filter out some Nodes. In this case, some changes in the cluster may change the result from those PreFilter and may make this Pod schedulable.

Which issue(s) this PR fixes:

Fixes #122018

Special notes for your reviewer:

@sanposhiho

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2023-12-10T13:31:03Z

Please note that we're already in Test Freeze for the release-1.29 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.29.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sun Dec 10 10:17:03 UTC 2023.

k8s-ci-robot · 2023-12-10T13:31:05Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2023-12-10T13:31:07Z

Hi @olderTaoist. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

AxeZhan · 2023-12-11T04:56:31Z

/ok-to-test

pkg/scheduler/framework/runtime/framework.go

pkg/scheduler/framework/interface.go

pkg/scheduler/framework/runtime/framework_test.go

pkg/scheduler/framework/runtime/framework.go

sanposhiho · 2023-12-22T02:16:57Z

/assign

I'll review this in this weekend hopefully, during the new year holiday at the latest.

olderTaoist · 2024-06-07T01:16:11Z

Actually, #119779, which is just merged, changed how we treat PreFilterResult. In this PR, I guess we can do something like:

add diagnosis into RunPreFilterPlugins function's parameters.

update diagnosis.UnschedulablePlugins and diagnosis.NodeToStatusMap when PreFilter returns PreFilterResult.

Then we can cleanup some logic in findNodesThatFitPod that modify diagnosis after RunPreFilterPlugins.

@olderTaoist @sanposhiho

I think this pr needs some rework/refactor.

This pr has done huge changes to achieve update diagnosis.NodeToStatusMap when PreFilter returns PreFilterResult. However, this action will greatly harm the performance. We're planning to remove this in recent pr(#125197 (comment)). This is because we try to implement option2 proposed here.

So, after #125197, I think all we have to do is

add diagnosis into RunPreFilterPlugins function's parameters.

update diagnosis.UnschedulablePlugins when PreFilter returns PreFilterResult with allNodes() false.

kubernetes/pkg/scheduler/framework/runtime/framework.go

Lines 729 to 731 in 6938c29

if !r.AllNodes() {

pluginsWithNodes = append(pluginsWithNodes, pl.Name())

}
 if !r.AllNodes() { 
 	pluginsWithNodes = append(pluginsWithNodes, pl.Name()) 
        diagnosis.UnschedulablePlugins.Insert(pl.Name())
 } 

I have read the relevant diagnosis.NodeToStatusMap PR carefully recently, and I agree with your idea, please review again @AxeZhan .

olderTaoist · 2024-06-07T03:36:28Z

Sry, just realized that there are out-of-tree plugins which may return Unschedulable during preFilter. Then we should also update diagnosis.NodeToStatusMap, but only when preFilter returns an Unschedulable status?

kubernetes/pkg/scheduler/framework/runtime/framework.go

Lines 720 to 726 in 39c6bc3

if s.Code() == framework.Unschedulable {

// In this case, the preemption should happen later in this scheduling cycle.

// So we need to execute all PreFilter.

// https://github.com/kubernetes/kubernetes/issues/119770

returnStatus = s

continue

}

when all nodes are filtered by PreFilter that returns PreFilterResult with allNodes() false, RunPreFilterPlugins will return an UnschedulableAndUnresolvable status

kubernetes/pkg/scheduler/framework/runtime/framework.go

Lines 733 to 740 in 6938c29

    
           if !result.AllNodes() && len(result.NodeNames) == 0 { 
        
           	msg := fmt.Sprintf("node(s) didn't satisfy plugin(s) %v simultaneously", pluginsWithNodes) 
        
           	if len(pluginsWithNodes) == 1 { 
        
           		msg = fmt.Sprintf("node(s) didn't satisfy plugin %v", pluginsWithNodes[0]) 
        
           	} 
        
           	// When PreFilterResult filters out Nodes, the framework considers Nodes that are filtered out as getting "UnschedulableAndUnresolvable". 
        
           	return result, framework.NewStatus(framework.UnschedulableAndUnresolvable, msg)

The original logic will update diagnosis.NodeToStatusMap no matter UnschedulableAndUnresolvable or Unschedulable, I am confused about whether it is necessary to update diagnosis.NodeToStatusMap when there are some prefilters that returns PreFilterResult with allNodes() false and status is nil. @alculquicondor @gabesaba . this pr just focus on diagnosis.UnschedulablePlugins

AxeZhan · 2024-06-07T03:55:21Z

I am confused about whether it is necessary to update diagnosis.NodeToStatusMap when there are some prefilters that returns PreFilterResult with allNodes() false and status is nil.

No need to update those NodeStatus, nodes passed prefilter will get their status updated during filter stage.
In preFilter, we only make sure if prefilter returned Unschedulable, we set status for all nodes to let preemption select candidates.

And we'll need to find a better way than iterating all nodes and set status in a map. But that is out of this pr's scope.

pkg/scheduler/framework/runtime/framework.go

pkg/scheduler/extender_test.go

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

pkg/scheduler/framework/runtime/framework_test.go

pkg/scheduler/schedule_one_test.go

gabesaba · 2024-06-07T10:50:41Z

pkg/scheduler/framework/interface.go

@@ -588,7 +588,7 @@ type Framework interface {
 	// cycle is aborted.
 	// It also returns a PreFilterResult, which may influence what or how many nodes to
 	// evaluate downstream.
-	RunPreFilterPlugins(ctx context.Context, state *CycleState, pod *v1.Pod) (*PreFilterResult, *Status)
+	RunPreFilterPlugins(ctx context.Context, state *CycleState, pod *v1.Pod, diagnosis *Diagnosis) (*PreFilterResult, *Status)


another option would be to add UnschedulablePlugins to the PreFilterResult and have the caller of RunPrefilterPlugins use that result to update diagnosis. It avoids changing the interface, and makes the data flow clearer.

Yes, we can do that.
Me myself prefer the current implementation.

Is there an advantage to the current implementation that I'm not seeing which justifies having side effects? My concern is that readers/maintainers of this code will have to read the implementation of RunPreFilterPlugins to see why Diagnosis is passed, and which fields in it are read/mutated. Passing this type also makes it easier in the future for other fields in Diagnosis to be modified without careful consideration, increasing complexity (suppose NodeToStatusMap is also modified inside of RunPreFilterPlugins, rather than just by findNodesThatFitPod).

While the data flow/mutability is my primary concern, this PR also becomes smaller (8 -> 4 files), if we include result in PreFilterResult

I agree with @gabesaba's rationale.
Please update the PR.

already updated, please review again @AxeZhan @gabesaba @alculquicondor

Can we just return UnschedulablePlugins as a return value from RunPreFilterPlugins, instead of a new field in PreFilterResult?

PreFilterResult is a data structure we also use between the framework and plugins, that is, supposed to be exposed to plugin developers, while UnschedulablePlugins would not be a field for plugins developers. It looks better not to expose such field in PreFilterResult, if we haven't got a special reason doing so.

@gabesaba has raised a similar point. #122251 (comment)

We can do this as a follow-up?

I believe this should be done in this PR.

AxeZhan

Thanks! Only few nits.

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

pkg/scheduler/schedule_one_test.go

k8s-ci-robot · 2024-06-12T01:30:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: olderTaoist
Once this PR has been reviewed and has the lgtm label, please ask for approval from sanposhiho. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/scheduler/OWNERS

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

gabesaba · 2024-06-12T10:04:33Z

pkg/scheduler/framework/interface.go

@@ -740,6 +743,7 @@ func (p *PreFilterResult) Merge(in *PreFilterResult) *PreFilterResult {
 	}

 	r.NodeNames = p.NodeNames.Intersection(in.NodeNames)
+	r.UnschedulablePlugins = p.UnschedulablePlugins.Clone()


how do we handle case when in has UnschedulablePlugins?

in will be no UnschedulablePlugins

in will be no UnschedulablePlugins

In current implementation in this pr, yes. But I also prefer a union here for future usage.

gabesaba · 2024-06-12T10:15:27Z

pkg/scheduler/framework/runtime/framework.go

@@ -726,10 +726,14 @@ func (f *frameworkImpl) RunPreFilterPlugins(ctx context.Context, state *framewor
 			}


do we need to record unschedulable plugins in the branches above - lines 715 and 720? or only when the plugin filters out some nodes (as title of PR indicates)

The previous implementation that unschedulable plugins was also added in lines 713 and 714

I think I understand now. We set the plugin name, and later we add it via this call?

But if more than one plugin returns Unschedulable (or that, +a plugin returns UnschedulableAndUnresolvable), we may miss some plugins, right?

gabesaba · 2024-06-12T10:20:18Z

pkg/scheduler/framework/runtime/framework.go

+		result = result.Merge(r)
 		if !r.AllNodes() {
+			if result.UnschedulablePlugins == nil {
+				result.UnschedulablePlugins = sets.New[string]()
+			}
+			result.UnschedulablePlugins.Insert(pl.Name())
 			pluginsWithNodes = append(pluginsWithNodes, pl.Name())
 		}


Suggested change

result = result.Merge(r)

if !r.AllNodes() {

if result.UnschedulablePlugins == nil {

result.UnschedulablePlugins = sets.New[string]()

}

result.UnschedulablePlugins.Insert(pl.Name())

pluginsWithNodes = append(pluginsWithNodes, pl.Name())

}

if !r.AllNodes() {

r.UnschedulablePlugins = sets.New(pl.Name())

pluginsWithNodes = append(pluginsWithNodes, pl.Name())

}

result = result.Merge(r)

or, if we want fewer allocs, create set outside of the loop and assign to result before final return. (requires small refactor, to break outside of loop on line 744, rather than return)

Is there anything wrong with me writing this?

The current implementation works, but there is room for simplification (fewer branches) and less code. After all, we just want to do a set union of unschedulable plugin names, not merge some complex data structure.

create set outside of the loop and assign to result before final return

I think this is the simplest option, as we don't have to worry about the merge logic (which, in the general case, becomes complex if we want to properly handle all combinations of nil/non-nil)

like this？

Yes, your new version is even better than what I had in mind :)

gabesaba · 2024-06-12T10:28:49Z

pkg/scheduler/framework/interface.go

@@ -718,6 +718,9 @@ type PreFilterResult struct {
 	// The set of nodes that should be considered downstream; if nil then


perhaps outside of the scope of this PR (something we can fix as followup), but a question for @AxeZhan, @alculquicondor: Does it make sense to share this result type between PreFilter and RunPreFilterPlugins? As now, there is a field which users of the framework should never need to set

Could you please elaborate? I don't quite get it.

RunPreFilterPlugins acts on all plugins, while PreFilter runs a single plugin, but they both return the same PreFilterResult type. My concerns are:

With the addition to PreFilterResult I suggested, the field UnschedulablePlugins should only be used by RunPreFilterPlugins. As PreFilter shouldn't modify this field, this may be confusing in the interface, and we may have to defensively code against the case when it was modified.

We assume we can fold or "merge" these PreFilterResults into a single PreFilterResult. This may not always be true, and it is already a little clumsy. All we want to do is a set intersection on NodeNames, and a set union on UnschedulablePlugins. We don't need a complex merge function for that, where we have to safely handle one or both sides being nil. We just need two sets in RunPreFilterPlugins.

I don't think this should block this PR, but waited to raise it as something we might clean up after, and therefore wanted to get your feedback. If you agree with this, I'm happy to make these changes after this PR merges.

gabesaba

only a few nits left. otherwise it looks good. thanks for addressing the comments!

gabesaba · 2024-06-13T12:04:02Z

pkg/scheduler/schedule_one.go

+	if preRes != nil && preRes.UnschedulablePlugins != nil {
+		diagnosis.UnschedulablePlugins = preRes.UnschedulablePlugins.Clone()
+	}


Suggested change

if preRes != nil && preRes.UnschedulablePlugins != nil {

diagnosis.UnschedulablePlugins = preRes.UnschedulablePlugins.Clone()

}

if preRes != nil {

diagnosis.UnschedulablePlugins = preRes.UnschedulablePlugins

}

nit: since we don't use this field elsewhere, we may omit copy (and propagating nil value is fine)

gabesaba · 2024-06-13T12:07:09Z

pkg/scheduler/framework/runtime/framework.go

+		result.UnschedulablePlugins = sets.New[string]()
+		result.UnschedulablePlugins.Insert(pluginsWithNodes...)


Suggested change

result.UnschedulablePlugins = sets.New[string]()

result.UnschedulablePlugins.Insert(pluginsWithNodes...)

result.UnschedulablePlugins = sets.New(pluginsWithNodes...)

does this compile?

gabesaba · 2024-06-13T12:11:37Z

pkg/scheduler/framework/interface.go

@@ -718,6 +718,9 @@ type PreFilterResult struct {
 	// The set of nodes that should be considered downstream; if nil then
 	// all nodes are eligible.
 	NodeNames sets.Set[string]
+
+	// UnschedulablePlugins are plugins that returns Unschedulable or UnschedulableAndUnresolvable.


Suggested change

// UnschedulablePlugins are plugins that returns Unschedulable or UnschedulableAndUnresolvable.

// UnschedulablePlugins are plugins which filter out nodes.

if plugin returns UnschedulableAndUnresolvable or Unschedulable, we don't add to this set. How about this comment?

We should update this comment too, as we will now include plugins which return success, but filtered some nodes.

kubernetes/pkg/scheduler/framework/types.go

Lines 345 to 346 in b498eb9

// UnschedulablePlugins are plugins that returns Unschedulable or UnschedulableAndUnresolvable.

UnschedulablePlugins sets.Set[string]

sanposhiho · 2024-06-15T04:11:05Z

@olderTaoist Also, please fill in the release note, e.g.,

The scheduler now retries Pods rejected with PreFilterResult more appropriately.

(Maybe we can say more, but no need to explain very deeply.)

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 10, 2023

k8s-ci-robot requested review from chendave and damemi December 10, 2023 13:31

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Dec 10, 2023

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 10, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 11, 2023

AxeZhan reviewed Dec 11, 2023

View reviewed changes

pkg/scheduler/framework/runtime/framework.go Outdated Show resolved Hide resolved

olderTaoist force-pushed the unschedulable-plugin branch from ad02730 to 9b41295 Compare December 12, 2023 08:47

AxeZhan reviewed Dec 13, 2023

View reviewed changes

pkg/scheduler/framework/interface.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/runtime/framework_test.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2023

olderTaoist force-pushed the unschedulable-plugin branch from 0bf09c0 to 5ecdfec Compare December 20, 2023 11:28

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

AxeZhan reviewed Dec 20, 2023

View reviewed changes

pkg/scheduler/framework/runtime/framework.go Outdated Show resolved Hide resolved

olderTaoist force-pushed the unschedulable-plugin branch 3 times, most recently from 7a9b583 to bea0b63 Compare December 21, 2023 01:25

k8s-ci-robot assigned sanposhiho Dec 22, 2023

AxeZhan reviewed Jun 7, 2024

View reviewed changes

olderTaoist force-pushed the unschedulable-plugin branch 2 times, most recently from e0db92a to 8a8c040 Compare June 7, 2024 10:14

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2024

olderTaoist requested a review from AxeZhan June 7, 2024 10:19

gabesaba reviewed Jun 7, 2024

View reviewed changes

AxeZhan reviewed Jun 7, 2024

View reviewed changes

pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go Outdated Show resolved Hide resolved

pkg/scheduler/schedule_one_test.go Outdated Show resolved Hide resolved

gabesaba mentioned this pull request Jun 10, 2024

REQUEST: New membership for gabesaba kubernetes/org#5008

Closed

11 tasks

olderTaoist force-pushed the unschedulable-plugin branch from 8a8c040 to 8a1b092 Compare June 11, 2024 01:05

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 11, 2024

olderTaoist force-pushed the unschedulable-plugin branch from 8a1b092 to 87035b6 Compare June 11, 2024 01:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 11, 2024

olderTaoist requested a review from AxeZhan June 11, 2024 01:29

olderTaoist force-pushed the unschedulable-plugin branch from 87035b6 to 605d4b1 Compare June 12, 2024 01:29

register unscheduable plugin when prefileter with NodeNames

fe6b4b5

olderTaoist force-pushed the unschedulable-plugin branch from 605d4b1 to fe6b4b5 Compare June 12, 2024 01:37

olderTaoist requested review from gabesaba and alculquicondor June 12, 2024 01:42

gabesaba reviewed Jun 12, 2024

View reviewed changes

small refactor

f08c60c

olderTaoist requested a review from gabesaba June 13, 2024 11:33

gabesaba reviewed Jun 13, 2024

View reviewed changes

		@@ -726,10 +726,14 @@ func (f frameworkImpl) RunPreFilterPlugins(ctx context.Context, state framewor
		}

		@@ -718,6 +718,9 @@ type PreFilterResult struct {
		// The set of nodes that should be considered downstream; if nil then

		result.UnschedulablePlugins = sets.New[string]()
		result.UnschedulablePlugins.Insert(pluginsWithNodes...)

	result.UnschedulablePlugins = sets.New[string]()
	result.UnschedulablePlugins.Insert(pluginsWithNodes...)
	result.UnschedulablePlugins = sets.New(pluginsWithNodes...)

	// UnschedulablePlugins are plugins that returns Unschedulable or UnschedulableAndUnresolvable.
	// UnschedulablePlugins are plugins which filter out nodes.

	// UnschedulablePlugins are plugins that returns Unschedulable or UnschedulableAndUnresolvable.
	UnschedulablePlugins sets.Set[string]

register unschedulable plugin for those plugins that PreFilter's PreFilterResult filter out some nodes #122251

Are you sure you want to change the base?

register unschedulable plugin for those plugins that PreFilter's PreFilterResult filter out some nodes #122251

Conversation

olderTaoist commented Dec 10, 2023

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?

k8s-ci-robot commented Dec 10, 2023

k8s-ci-robot commented Dec 10, 2023

k8s-ci-robot commented Dec 10, 2023

AxeZhan commented Dec 11, 2023

sanposhiho commented Dec 22, 2023

olderTaoist commented Jun 7, 2024 • edited

olderTaoist commented Jun 7, 2024

AxeZhan commented Jun 7, 2024

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

sanposhiho Jun 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AxeZhan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 12, 2024

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

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

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

gabesaba left a 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

Choose a reason for hiding this comment

sanposhiho commented Jun 15, 2024

olderTaoist commented Jun 7, 2024 •

edited

sanposhiho Jun 13, 2024 •

edited