From 2c205e291d539e749b50cd857e57f1d098f65c7f Mon Sep 17 00:00:00 2001 From: Kante Yin Date: Thu, 5 Jan 2023 16:59:19 +0800 Subject: [PATCH] Distinguish unschedulable with unresolvable in scheduler Before, in RunPostFilterPlugins, we didn't distinguish between unschedulable and unresolvable because we only have one postFilterPlugin by default, now, we have at least two, we should make sure that once a postFilterPlugin returns unresolvable, we'll return directly Signed-off-by: Kante Yin --- pkg/scheduler/framework/interface.go | 53 +++----------- pkg/scheduler/framework/interface_test.go | 37 ---------- pkg/scheduler/framework/runtime/framework.go | 21 ++++-- .../framework/runtime/framework_test.go | 42 +++++++++++ .../scheduler/plugins/plugins_test.go | 71 ++++++++++++++----- 5 files changed, 120 insertions(+), 104 deletions(-) diff --git a/pkg/scheduler/framework/interface.go b/pkg/scheduler/framework/interface.go index 3d988a7df5a..97a18f7d03a 100644 --- a/pkg/scheduler/framework/interface.go +++ b/pkg/scheduler/framework/interface.go @@ -79,13 +79,14 @@ const ( // Error is used for internal plugin errors, unexpected input, etc. Error // Unschedulable is used when a plugin finds a pod unschedulable. The scheduler might attempt to - // preempt other pods to get this pod scheduled. Use UnschedulableAndUnresolvable to make the - // scheduler skip preemption. + // run other postFilter plugins like preemption to get this pod scheduled. + // Use UnschedulableAndUnresolvable to make the scheduler skipping other postFilter plugins. // The accompanying status message should explain why the pod is unschedulable. Unschedulable // UnschedulableAndUnresolvable is used when a plugin finds a pod unschedulable and - // preemption would not change anything. Plugins should return Unschedulable if it is possible - // that the pod can get scheduled with preemption. + // other postFilter plugins like preemption would not change anything. + // Plugins should return Unschedulable if it is possible that the pod can get scheduled + // after running other postFilter plugins. // The accompanying status message should explain why the pod is unschedulable. UnschedulableAndUnresolvable // Wait is used when a Permit plugin finds a pod scheduling should wait. @@ -97,15 +98,6 @@ const ( // This list should be exactly the same as the codes iota defined above in the same order. var codes = []string{"Success", "Error", "Unschedulable", "UnschedulableAndUnresolvable", "Wait", "Skip"} -// statusPrecedence defines a map from status to its precedence, larger value means higher precedent. -var statusPrecedence = map[Code]int{ - Error: 3, - UnschedulableAndUnresolvable: 2, - Unschedulable: 1, - // Any other statuses we know today, `Skip` or `Wait`, will take precedence over `Success`. - Success: -1, -} - func (c Code) String() string { return codes[c] } @@ -255,7 +247,10 @@ func (s *Status) Equal(x *Status) bool { if !cmp.Equal(s.err, x.err, cmpopts.EquateErrors()) { return false } - return cmp.Equal(s.reasons, x.reasons) + if !cmp.Equal(s.reasons, x.reasons) { + return false + } + return cmp.Equal(s.failedPlugin, x.failedPlugin) } // NewStatus makes a Status out of the given arguments and returns its pointer. @@ -278,36 +273,6 @@ func AsStatus(err error) *Status { } } -// PluginToStatus maps plugin name to status it returned. -type PluginToStatus map[string]*Status - -// Merge merges the statuses in the map into one. The resulting status code have the following -// precedence: Error, UnschedulableAndUnresolvable, Unschedulable. -func (p PluginToStatus) Merge() *Status { - if len(p) == 0 { - return nil - } - - finalStatus := NewStatus(Success) - for _, s := range p { - if statusPrecedence[s.Code()] > statusPrecedence[finalStatus.code] { - finalStatus.code = s.Code() - // Same as code, we keep the most relevant failedPlugin in the returned Status. - finalStatus.failedPlugin = s.FailedPlugin() - } - - reasons := s.Reasons() - if finalStatus.err == nil { - finalStatus.err = s.err - reasons = s.reasons - } - for _, r := range reasons { - finalStatus.AppendReason(r) - } - } - return finalStatus -} - // WaitingPod represents a pod currently waiting in the permit phase. type WaitingPod interface { // GetPod returns a reference to the waiting pod. diff --git a/pkg/scheduler/framework/interface_test.go b/pkg/scheduler/framework/interface_test.go index a8c5fbc0b4a..10612cd1940 100644 --- a/pkg/scheduler/framework/interface_test.go +++ b/pkg/scheduler/framework/interface_test.go @@ -139,43 +139,6 @@ func assertStatusCode(t *testing.T, code Code, value int) { } } -func TestPluginToStatusMerge(t *testing.T) { - tests := []struct { - name string - statusMap PluginToStatus - wantCode Code - }{ - { - name: "merge Error and Unschedulable statuses", - statusMap: PluginToStatus{"p1": NewStatus(Error), "p2": NewStatus(Unschedulable)}, - wantCode: Error, - }, - { - name: "merge Success and Unschedulable statuses", - statusMap: PluginToStatus{"p1": NewStatus(Success), "p2": NewStatus(Unschedulable)}, - wantCode: Unschedulable, - }, - { - name: "merge Success, UnschedulableAndUnresolvable and Unschedulable statuses", - statusMap: PluginToStatus{"p1": NewStatus(Success), "p2": NewStatus(UnschedulableAndUnresolvable), "p3": NewStatus(Unschedulable)}, - wantCode: UnschedulableAndUnresolvable, - }, - { - name: "merge nil status", - wantCode: Success, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - gotStatus := test.statusMap.Merge() - if test.wantCode != gotStatus.Code() { - t.Errorf("wantCode %v, gotCode %v", test.wantCode, gotStatus.Code()) - } - }) - } -} - func TestPreFilterResultMerge(t *testing.T) { tests := map[string]struct { receiver *PreFilterResult diff --git a/pkg/scheduler/framework/runtime/framework.go b/pkg/scheduler/framework/runtime/framework.go index 3ca801de891..521a8251ee4 100644 --- a/pkg/scheduler/framework/runtime/framework.go +++ b/pkg/scheduler/framework/runtime/framework.go @@ -752,30 +752,39 @@ func (f *frameworkImpl) runFilterPlugin(ctx context.Context, pl framework.Filter } // RunPostFilterPlugins runs the set of configured PostFilter plugins until the first -// Success or Error is met, otherwise continues to execute all plugins. +// Success, Error or UnschedulableAndUnresolvable is met; otherwise continues to execute all plugins. func (f *frameworkImpl) RunPostFilterPlugins(ctx context.Context, state *framework.CycleState, pod *v1.Pod, filteredNodeStatusMap framework.NodeToStatusMap) (_ *framework.PostFilterResult, status *framework.Status) { startTime := time.Now() defer func() { metrics.FrameworkExtensionPointDuration.WithLabelValues(postFilter, status.Code().String(), f.profileName).Observe(metrics.SinceInSeconds(startTime)) }() - statuses := make(framework.PluginToStatus) // `result` records the last meaningful(non-noop) PostFilterResult. var result *framework.PostFilterResult + var reasons []string + var failedPlugin string for _, pl := range f.postFilterPlugins { r, s := f.runPostFilterPlugin(ctx, pl, state, pod, filteredNodeStatusMap) if s.IsSuccess() { return r, s + } else if s.Code() == framework.UnschedulableAndUnresolvable { + return r, s.WithFailedPlugin(pl.Name()) } else if !s.IsUnschedulable() { - // Any status other than Success or Unschedulable is Error. - return nil, framework.AsStatus(s.AsError()) + // Any status other than Success, Unschedulable or UnschedulableAndUnresolvable is Error. + return nil, framework.AsStatus(s.AsError()).WithFailedPlugin(pl.Name()) } else if r != nil && r.Mode() != framework.ModeNoop { result = r } - statuses[pl.Name()] = s + + reasons = append(reasons, s.Reasons()...) + // Record the first failed plugin unless we proved that + // the latter is more relevant. + if len(failedPlugin) == 0 { + failedPlugin = pl.Name() + } } - return result, statuses.Merge() + return result, framework.NewStatus(framework.Unschedulable, reasons...).WithFailedPlugin(failedPlugin) } func (f *frameworkImpl) runPostFilterPlugin(ctx context.Context, pl framework.PostFilterPlugin, state *framework.CycleState, pod *v1.Pod, filteredNodeStatusMap framework.NodeToStatusMap) (*framework.PostFilterResult, *framework.Status) { diff --git a/pkg/scheduler/framework/runtime/framework_test.go b/pkg/scheduler/framework/runtime/framework_test.go index 8d5b9103e46..d7974d8974d 100644 --- a/pkg/scheduler/framework/runtime/framework_test.go +++ b/pkg/scheduler/framework/runtime/framework_test.go @@ -1676,6 +1676,48 @@ func TestPostFilterPlugins(t *testing.T) { }, wantStatus: framework.NewStatus(framework.Success, injectReason), }, + { + name: "plugin1 failed to make a Pod schedulable, followed by plugin2 which makes the Pod schedulable", + plugins: []*TestPlugin{ + { + name: "TestPlugin1", + inj: injectedResult{PostFilterStatus: int(framework.Error)}, + }, + { + name: "TestPlugin2", + inj: injectedResult{PostFilterStatus: int(framework.Success)}, + }, + }, + wantStatus: framework.AsStatus(fmt.Errorf(injectReason)).WithFailedPlugin("TestPlugin1"), + }, + { + name: "plugin1 failed to make a Pod schedulable, followed by plugin2 which makes the Pod unresolvable", + plugins: []*TestPlugin{ + { + name: "TestPlugin1", + inj: injectedResult{PostFilterStatus: int(framework.Unschedulable)}, + }, + { + name: "TestPlugin2", + inj: injectedResult{PostFilterStatus: int(framework.UnschedulableAndUnresolvable)}, + }, + }, + wantStatus: framework.NewStatus(framework.UnschedulableAndUnresolvable, injectReason).WithFailedPlugin("TestPlugin2"), + }, + { + name: "both plugins failed to make a Pod schedulable", + plugins: []*TestPlugin{ + { + name: "TestPlugin1", + inj: injectedResult{PostFilterStatus: int(framework.Unschedulable)}, + }, + { + name: "TestPlugin2", + inj: injectedResult{PostFilterStatus: int(framework.Unschedulable)}, + }, + }, + wantStatus: framework.NewStatus(framework.Unschedulable, []string{injectReason, injectReason}...).WithFailedPlugin("TestPlugin1"), + }, } for _, tt := range tests { diff --git a/test/integration/scheduler/plugins/plugins_test.go b/test/integration/scheduler/plugins/plugins_test.go index 12a73a40d13..20c3bec480e 100644 --- a/test/integration/scheduler/plugins/plugins_test.go +++ b/test/integration/scheduler/plugins/plugins_test.go @@ -96,10 +96,12 @@ type FilterPlugin struct { } type PostFilterPlugin struct { + name string fh framework.Handle numPostFilterCalled int failPostFilter bool rejectPostFilter bool + breakPostFilter bool } type ReservePlugin struct { @@ -463,7 +465,7 @@ func (pp *PreFilterPlugin) reset() { // Name returns name of the plugin. func (pp *PostFilterPlugin) Name() string { - return postfilterPluginName + return pp.name } func (pp *PostFilterPlugin) PostFilter(ctx context.Context, state *framework.CycleState, pod *v1.Pod, _ framework.NodeToStatusMap) (*framework.PostFilterResult, *framework.Status) { @@ -486,8 +488,12 @@ func (pp *PostFilterPlugin) PostFilter(ctx context.Context, state *framework.Cyc return nil, framework.NewStatus(framework.Error, fmt.Sprintf("injecting failure for pod %v", pod.Name)) } if pp.rejectPostFilter { - return nil, framework.NewStatus(framework.Unschedulable, fmt.Sprintf("reject pod %v", pod.Name)) + return nil, framework.NewStatus(framework.Unschedulable, fmt.Sprintf("injecting unschedulable for pod %v", pod.Name)) } + if pp.breakPostFilter { + return nil, framework.NewStatus(framework.UnschedulableAndUnresolvable, fmt.Sprintf("injecting unresolvable for pod %v", pod.Name)) + } + return nil, framework.NewStatus(framework.Success, fmt.Sprintf("make room for pod %v to be schedulable", pod.Name)) } @@ -628,7 +634,7 @@ func TestPreFilterPlugin(t *testing.T) { } } -// TestPostFilterPlugin tests invocation of postfilter plugins. +// TestPostFilterPlugin tests invocation of postFilter plugins. func TestPostFilterPlugin(t *testing.T) { var numNodes int32 = 1 tests := []struct { @@ -637,6 +643,9 @@ func TestPostFilterPlugin(t *testing.T) { rejectFilter bool failScore bool rejectPostFilter bool + rejectPostFilter2 bool + breakPostFilter bool + breakPostFilter2 bool expectFilterNumCalled int32 expectScoreNumCalled int32 expectPostFilterNumCalled int @@ -669,7 +678,7 @@ func TestPostFilterPlugin(t *testing.T) { rejectPostFilter: true, expectFilterNumCalled: numNodes * 2, expectScoreNumCalled: 1, - expectPostFilterNumCalled: 1, + expectPostFilterNumCalled: 2, }, { name: "Score failed and PostFilter failed", @@ -679,25 +688,53 @@ func TestPostFilterPlugin(t *testing.T) { rejectPostFilter: true, expectFilterNumCalled: numNodes * 2, expectScoreNumCalled: 1, + expectPostFilterNumCalled: 2, + }, + { + name: "Filter failed and first PostFilter broken", + numNodes: numNodes, + rejectFilter: true, + breakPostFilter: true, + expectFilterNumCalled: numNodes * 2, + expectScoreNumCalled: 0, expectPostFilterNumCalled: 1, }, + { + name: "Filter failed and second PostFilter broken", + numNodes: numNodes, + rejectFilter: true, + rejectPostFilter: true, + rejectPostFilter2: true, + breakPostFilter2: true, + expectFilterNumCalled: numNodes * 2, + expectScoreNumCalled: 0, + expectPostFilterNumCalled: 2, + }, } + var postFilterPluginName2 = postfilterPluginName + "2" + for i, tt := range tests { t.Run(tt.name, func(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 = &FilterPlugin{} + scorePlugin = &ScorePlugin{} + postFilterPlugin = &PostFilterPlugin{name: postfilterPluginName} + postFilterPlugin2 = &PostFilterPlugin{name: postFilterPluginName2} ) filterPlugin.rejectFilter = tt.rejectFilter scorePlugin.failScore = tt.failScore postFilterPlugin.rejectPostFilter = tt.rejectPostFilter + postFilterPlugin2.rejectPostFilter = tt.rejectPostFilter2 + postFilterPlugin.breakPostFilter = tt.breakPostFilter + postFilterPlugin2.breakPostFilter = tt.breakPostFilter2 + registry := frameworkruntime.Registry{ - filterPluginName: newPlugin(filterPlugin), - scorePluginName: newPlugin(scorePlugin), - postfilterPluginName: newPlugin(postFilterPlugin), + filterPluginName: newPlugin(filterPlugin), + scorePluginName: newPlugin(scorePlugin), + postfilterPluginName: newPlugin(postFilterPlugin), + postFilterPluginName2: newPlugin(postFilterPlugin2), } // Setup plugins for testing. @@ -723,9 +760,10 @@ func TestPostFilterPlugin(t *testing.T) { PostFilter: configv1.PluginSet{ Enabled: []configv1.Plugin{ {Name: postfilterPluginName}, + {Name: postFilterPluginName2}, }, // Need to disable default in-tree PostFilter plugins, as they will - // call RunFilterPlugins and hence impact the "numFilterCalled". + // call RunPostFilterPlugins and hence impact the "numPostFilterCalled". Disabled: []configv1.Plugin{ {Name: "*"}, }, @@ -760,9 +798,6 @@ func TestPostFilterPlugin(t *testing.T) { 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) } - 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) - } } else { if err = testutils.WaitForPodToSchedule(testCtx.ClientSet, pod); err != nil { t.Errorf("Expected the pod to be scheduled. error: %v", err) @@ -773,9 +808,11 @@ func TestPostFilterPlugin(t *testing.T) { 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) - } + } + + numPostFilterCalled := postFilterPlugin.numPostFilterCalled + postFilterPlugin2.numPostFilterCalled + if numPostFilterCalled != tt.expectPostFilterNumCalled { + t.Errorf("Expected the postfilter plugin to be called %v times, but got %v.", tt.expectPostFilterNumCalled, numPostFilterCalled) } }) }