Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scheduler: expose Run[Pre]ScorePlugins functions to PreemptionHandle(through PluginRunner) #93534

Merged
merged 2 commits into from Oct 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/scheduler/framework/v1alpha1/interface.go
Expand Up @@ -555,6 +555,10 @@ type PodNominator interface {
// This is used by preemption PostFilter plugins when evaluating the feasibility of
// scheduling the pod on nodes when certain running pods get evicted.
type PluginsRunner interface {
// RunPreScorePlugins runs the set of configured PreScore plugins for pod on the given nodes
RunPreScorePlugins(context.Context, *CycleState, *v1.Pod, []*v1.Node) *Status
// RunScorePlugins runs the set of configured Score plugins for pod on the given nodes
RunScorePlugins(context.Context, *CycleState, *v1.Pod, []*v1.Node) (PluginToNodeScores, *Status)
// RunFilterPlugins runs the set of configured filter plugins for pod on the given node.
RunFilterPlugins(context.Context, *CycleState, *v1.Pod, *NodeInfo) PluginToStatus
// RunPreFilterExtensionAddPod calls the AddPod interface for the set of configured PreFilter plugins.
Expand Down
57 changes: 53 additions & 4 deletions test/integration/scheduler/framework_test.go
Expand Up @@ -407,10 +407,17 @@ func (pp *PostFilterPlugin) PostFilter(ctx context.Context, state *framework.Cyc
if err != nil {
return nil, framework.NewStatus(framework.Error, err.Error())
}

ph := pp.fh.PreemptHandle()
for _, nodeInfo := range nodeInfos {
ph.RunFilterPlugins(ctx, state, pod, nodeInfo)
}
var nodes []*v1.Node
for _, nodeInfo := range nodeInfos {
nodes = append(nodes, nodeInfo.Node())
}
ph.RunScorePlugins(ctx, state, pod, nodes)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we need some caveat here or in PluginsRunner interface to make it clear that this is potentially for out-of-tree plugins only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. I think I don't need comments here and PluginsRunner because the scheduler framework is essentially for out-of-tree plugins.

Copy link
Member

Choose a reason for hiding this comment

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

okay, thanks!


if pp.failPostFilter {
return nil, framework.NewStatus(framework.Error, fmt.Sprintf("injecting failure for pod %v", pod.Name))
}
Expand Down Expand Up @@ -578,30 +585,52 @@ func TestPostFilterPlugin(t *testing.T) {
numNodes := 1
tests := []struct {
name string
numNodes int
rejectFilter bool
failScore bool
rejectPostFilter bool
expectFilterNumCalled int
expectScoreNumCalled int32
expectPostFilterNumCalled int
}{
{
name: "Filter passed",
name: "Filter passed and Score success",
numNodes: 3,
Copy link
Contributor Author

@everpeace everpeace Aug 4, 2020

Choose a reason for hiding this comment

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

ref: #93534 (comment)
I introduced numNodes in test case. I set it to 3 here and others to 1

rejectFilter: false,
failScore: false,
rejectPostFilter: false,
expectFilterNumCalled: numNodes,
expectFilterNumCalled: 3,
expectScoreNumCalled: 3,
expectPostFilterNumCalled: 0,
},
{
name: "Filter failed and PostFilter passed",
numNodes: numNodes,
rejectFilter: true,
failScore: false,
rejectPostFilter: false,
expectFilterNumCalled: numNodes * 2,
expectScoreNumCalled: 1,
expectPostFilterNumCalled: 1,
},
{
name: "Filter failed and PostFilter failed",
numNodes: numNodes,
rejectFilter: true,
failScore: false,
rejectPostFilter: true,
expectFilterNumCalled: numNodes * 2,
expectScoreNumCalled: 1,
expectPostFilterNumCalled: 1,
},
{
name: "Score failed and PostFilter failed",
numNodes: numNodes,
rejectFilter: true,
failScore: true,
rejectPostFilter: true,
expectFilterNumCalled: numNodes * 2,
expectScoreNumCalled: 1,
expectPostFilterNumCalled: 1,
},
}
Expand All @@ -611,12 +640,15 @@ func TestPostFilterPlugin(t *testing.T) {
// Create a plugin registry for testing. Register a combination of filter and postFilter plugin.
var (
filterPlugin = &FilterPlugin{}
scorePlugin = &ScorePlugin{}
postFilterPlugin = &PostFilterPlugin{}
)
filterPlugin.rejectFilter = tt.rejectFilter
scorePlugin.failScore = tt.failScore
postFilterPlugin.rejectPostFilter = tt.rejectPostFilter
registry := frameworkruntime.Registry{
filterPluginName: newPlugin(filterPlugin),
scorePluginName: newPlugin(scorePlugin),
postfilterPluginName: newPostFilterPlugin(postFilterPlugin),
}

Expand All @@ -629,6 +661,16 @@ func TestPostFilterPlugin(t *testing.T) {
{Name: filterPluginName},
},
},
Score: &schedulerconfig.PluginSet{
Enabled: []schedulerconfig.Plugin{
{Name: scorePluginName},
},
// disable default in-tree Score plugins
// to make it easy to control configured ScorePlugins failure
Disabled: []schedulerconfig.Plugin{
{Name: "*"},
},
},
PostFilter: &schedulerconfig.PluginSet{
everpeace marked this conversation as resolved.
Show resolved Hide resolved
Enabled: []schedulerconfig.Plugin{
{Name: postfilterPluginName},
Expand All @@ -646,7 +688,7 @@ func TestPostFilterPlugin(t *testing.T) {
testCtx := initTestSchedulerForFrameworkTest(
t,
testutils.InitTestMaster(t, fmt.Sprintf("postfilter%v-", i), nil),
numNodes,
tt.numNodes,
scheduler.WithProfiles(prof),
scheduler.WithFrameworkOutOfTreeRegistry(registry),
)
Expand All @@ -662,9 +704,13 @@ func TestPostFilterPlugin(t *testing.T) {
if err = wait.Poll(10*time.Millisecond, 10*time.Second, podUnschedulable(testCtx.ClientSet, pod.Namespace, pod.Name)); err != nil {
t.Errorf("Didn't expect the pod to be scheduled.")
}

if filterPlugin.numFilterCalled < tt.expectFilterNumCalled {
t.Errorf("Expected the filter plugin to be called at least %v times, but got %v.", tt.expectFilterNumCalled, filterPlugin.numFilterCalled)
}
if numScoreCalled := atomic.LoadInt32(&scorePlugin.numScoreCalled); numScoreCalled < tt.expectScoreNumCalled {
t.Errorf("Expected the score plugin to be called at least %v times, but got %v.", tt.expectScoreNumCalled, numScoreCalled)
everpeace marked this conversation as resolved.
Show resolved Hide resolved
}
if postFilterPlugin.numPostFilterCalled < tt.expectPostFilterNumCalled {
t.Errorf("Expected the postfilter plugin to be called at least %v times, but got %v.", tt.expectPostFilterNumCalled, postFilterPlugin.numPostFilterCalled)
}
Expand All @@ -675,6 +721,9 @@ func TestPostFilterPlugin(t *testing.T) {
if filterPlugin.numFilterCalled != tt.expectFilterNumCalled {
t.Errorf("Expected the filter plugin to be called %v times, but got %v.", tt.expectFilterNumCalled, filterPlugin.numFilterCalled)
}
if numScoreCalled := atomic.LoadInt32(&scorePlugin.numScoreCalled); numScoreCalled != tt.expectScoreNumCalled {
t.Errorf("Expected the score plugin to be called %v times, but got %v.", tt.expectScoreNumCalled, numScoreCalled)
}
if postFilterPlugin.numPostFilterCalled != tt.expectPostFilterNumCalled {
t.Errorf("Expected the postfilter plugin to be called %v times, but got %v.", tt.expectPostFilterNumCalled, postFilterPlugin.numPostFilterCalled)
}
Expand Down Expand Up @@ -748,7 +797,7 @@ func TestScorePlugin(t *testing.T) {
}
}

if scorePlugin.numScoreCalled == 0 {
if numScoreCalled := atomic.LoadInt32(&scorePlugin.numScoreCalled); numScoreCalled == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned in #93534 (review)

BTW: can you also update L792 to make the scoreNum is obtained atomically?

Sure, I updated to read numScoreCalled with atomic in TestPostFilterPlugin, too.

t.Errorf("Expected the score plugin to be called.")
}

Expand Down