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) } }) }