From cf28762aeff98c569d73bc724971a4bb1506a595 Mon Sep 17 00:00:00 2001 From: Yuan Chen Date: Thu, 30 Jun 2022 14:52:57 -0700 Subject: [PATCH] Add PreFilter messages to Diagnosis Address Wei Huang's comments Define a separatorTemplate Add test for scheduler FitError.Error() --- pkg/scheduler/framework/types.go | 29 +++++++-- pkg/scheduler/framework/types_test.go | 85 +++++++++++++++++++++++++++ pkg/scheduler/schedule_one.go | 8 +-- pkg/scheduler/schedule_one_test.go | 30 ++++------ 4 files changed, 121 insertions(+), 31 deletions(-) diff --git a/pkg/scheduler/framework/types.go b/pkg/scheduler/framework/types.go index 247c124a7a5..6537007ae43 100644 --- a/pkg/scheduler/framework/types.go +++ b/pkg/scheduler/framework/types.go @@ -213,7 +213,9 @@ type WeightedAffinityTerm struct { type Diagnosis struct { NodeToStatusMap NodeToStatusMap UnschedulablePlugins sets.String - // PostFilterMsg records the messages returned from PostFilterPlugins. + // PreFilterMsg records the messages returned from PreFilter plugins. + PreFilterMsg string + // PostFilterMsg records the messages returned from PostFilter plugins. PostFilterMsg string } @@ -224,10 +226,15 @@ type FitError struct { Diagnosis Diagnosis } -// NoNodeAvailableMsg is used to format message when no nodes available. -const NoNodeAvailableMsg = "0/%v nodes are available" +const ( + // NoNodeAvailableMsg is used to format message when no nodes available. + NoNodeAvailableMsg = "0/%v nodes are available" + // SeparatorFormat is used to separate PreFilterMsg, FilterMsg and PostFilterMsg. + SeparatorFormat = " %v." +) -// Error returns detailed information of why the pod failed to fit on each node +// Error returns detailed information of why the pod failed to fit on each node. +// A message format is "0/X nodes are available: . . ." func (f *FitError) Error() string { reasons := make(map[string]int) for _, status := range f.Diagnosis.NodeToStatusMap { @@ -236,6 +243,12 @@ func (f *FitError) Error() string { } } + reasonMsg := fmt.Sprintf(NoNodeAvailableMsg+":", f.NumAllNodes) + // Add the messages from PreFilter plugins to reasonMsg. + preFilterMsg := f.Diagnosis.PreFilterMsg + if preFilterMsg != "" { + reasonMsg += fmt.Sprintf(SeparatorFormat, preFilterMsg) + } sortReasonsHistogram := func() []string { var reasonStrings []string for k, v := range reasons { @@ -244,10 +257,14 @@ func (f *FitError) Error() string { sort.Strings(reasonStrings) return reasonStrings } - reasonMsg := fmt.Sprintf(NoNodeAvailableMsg+": %v.", f.NumAllNodes, strings.Join(sortReasonsHistogram(), ", ")) + sortedFilterMsg := sortReasonsHistogram() + if len(sortedFilterMsg) != 0 { + reasonMsg += fmt.Sprintf(SeparatorFormat, strings.Join(sortedFilterMsg, ", ")) + } + // Add the messages from PostFilter plugins to reasonMsg. postFilterMsg := f.Diagnosis.PostFilterMsg if postFilterMsg != "" { - reasonMsg += " " + postFilterMsg + reasonMsg += fmt.Sprintf(SeparatorFormat, postFilterMsg) } return reasonMsg } diff --git a/pkg/scheduler/framework/types_test.go b/pkg/scheduler/framework/types_test.go index 7b1a0b42090..de8cffc369f 100644 --- a/pkg/scheduler/framework/types_test.go +++ b/pkg/scheduler/framework/types_test.go @@ -1373,3 +1373,88 @@ func TestGetNamespacesFromPodAffinityTerm(t *testing.T) { }) } } + +func TestFitError_Error(t *testing.T) { + tests := []struct { + name string + pod *v1.Pod + numAllNodes int + diagnosis Diagnosis + wantReasonMsg string + }{ + { + name: "nodes failed Prefilter plugin", + numAllNodes: 3, + diagnosis: Diagnosis{ + PreFilterMsg: "Node(s) failed PreFilter plugin FalsePreFilter", + }, + wantReasonMsg: "0/3 nodes are available: Node(s) failed PreFilter plugin FalsePreFilter.", + }, + { + name: "nodes failed one Filter plugin with an empty PostFilterMsg", + numAllNodes: 3, + diagnosis: Diagnosis{ + PreFilterMsg: "", + NodeToStatusMap: NodeToStatusMap{ + "node1": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node2": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node3": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + }, + }, + wantReasonMsg: "0/3 nodes are available: 3 Node(s) failed Filter plugin FalseFilter-1.", + }, + { + name: "nodes failed one Filter plugin with a non-empty PostFilterMsg", + numAllNodes: 3, + diagnosis: Diagnosis{ + PreFilterMsg: "", + NodeToStatusMap: NodeToStatusMap{ + "node1": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node2": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node3": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + }, + PostFilterMsg: "Error running PostFilter plugin FailedPostFilter", + }, + wantReasonMsg: "0/3 nodes are available: 3 Node(s) failed Filter plugin FalseFilter-1. Error running PostFilter plugin FailedPostFilter.", + }, + { + name: "nodes failed two Filter plugins with an empty PostFilterMsg", + numAllNodes: 3, + diagnosis: Diagnosis{ + PreFilterMsg: "", + NodeToStatusMap: NodeToStatusMap{ + "node1": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node2": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node3": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-2"), + }, + }, + wantReasonMsg: "0/3 nodes are available: 1 Node(s) failed Filter plugin FalseFilter-2, 2 Node(s) failed Filter plugin FalseFilter-1.", + }, + { + name: "nodes failed two Filter plugins with a non-empty PostFilterMsg", + numAllNodes: 3, + diagnosis: Diagnosis{ + PreFilterMsg: "", + NodeToStatusMap: NodeToStatusMap{ + "node1": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node2": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-1"), + "node3": NewStatus(Unschedulable, "Node(s) failed Filter plugin FalseFilter-2"), + }, + PostFilterMsg: "Error running PostFilter plugin FailedPostFilter", + }, + wantReasonMsg: "0/3 nodes are available: 1 Node(s) failed Filter plugin FalseFilter-2, 2 Node(s) failed Filter plugin FalseFilter-1. Error running PostFilter plugin FailedPostFilter.", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &FitError{ + Pod: tt.pod, + NumAllNodes: tt.numAllNodes, + Diagnosis: tt.diagnosis, + } + if gotReasonMsg := f.Error(); gotReasonMsg != tt.wantReasonMsg { + t.Errorf("Error() = Got: %v Want: %v", gotReasonMsg, tt.wantReasonMsg) + } + }) + } +} diff --git a/pkg/scheduler/schedule_one.go b/pkg/scheduler/schedule_one.go index 52ce257bc6a..febe269762e 100644 --- a/pkg/scheduler/schedule_one.go +++ b/pkg/scheduler/schedule_one.go @@ -400,11 +400,9 @@ func (sched *Scheduler) findNodesThatFitPod(ctx context.Context, fwk framework.F if !s.IsUnschedulable() { return nil, diagnosis, s.AsError() } - // All nodes will have the same status. Some non trivial refactoring is - // needed to avoid this copy. - for _, n := range allNodes { - diagnosis.NodeToStatusMap[n.Node().Name] = s - } + // Record the messages from PreFilter in Diagnosis.PreFilterMsg. + diagnosis.PreFilterMsg = s.Message() + klog.V(5).InfoS("Status after running PreFilter plugins for pod", "pod", klog.KObj(pod), "status", s) // Status satisfying IsUnschedulable() gets injected into diagnosis.UnschedulablePlugins. if s.FailedPlugin() != "" { diagnosis.UnschedulablePlugins.Insert(s.FailedPlugin()) diff --git a/pkg/scheduler/schedule_one_test.go b/pkg/scheduler/schedule_one_test.go index a782af5f7c5..4af56210492 100644 --- a/pkg/scheduler/schedule_one_test.go +++ b/pkg/scheduler/schedule_one_test.go @@ -1694,10 +1694,8 @@ func TestSchedulerSchedulePod(t *testing.T) { Pod: st.MakePod().Name("ignore").UID("ignore").PVC("unknownPVC").Obj(), NumAllNodes: 2, Diagnosis: framework.Diagnosis{ - NodeToStatusMap: framework.NodeToStatusMap{ - "node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, `persistentvolumeclaim "unknownPVC" not found`).WithFailedPlugin(volumebinding.Name), - "node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, `persistentvolumeclaim "unknownPVC" not found`).WithFailedPlugin(volumebinding.Name), - }, + NodeToStatusMap: framework.NodeToStatusMap{}, + PreFilterMsg: `persistentvolumeclaim "unknownPVC" not found`, UnschedulablePlugins: sets.NewString(volumebinding.Name), }, }, @@ -1718,10 +1716,8 @@ func TestSchedulerSchedulePod(t *testing.T) { Pod: st.MakePod().Name("ignore").UID("ignore").Namespace(v1.NamespaceDefault).PVC("existingPVC").Obj(), NumAllNodes: 2, Diagnosis: framework.Diagnosis{ - NodeToStatusMap: framework.NodeToStatusMap{ - "node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, `persistentvolumeclaim "existingPVC" is being deleted`).WithFailedPlugin(volumebinding.Name), - "node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, `persistentvolumeclaim "existingPVC" is being deleted`).WithFailedPlugin(volumebinding.Name), - }, + NodeToStatusMap: framework.NodeToStatusMap{}, + PreFilterMsg: `persistentvolumeclaim "existingPVC" is being deleted`, UnschedulablePlugins: sets.NewString(volumebinding.Name), }, }, @@ -1878,10 +1874,8 @@ func TestSchedulerSchedulePod(t *testing.T) { Pod: st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(), NumAllNodes: 2, Diagnosis: framework.Diagnosis{ - NodeToStatusMap: framework.NodeToStatusMap{ - "1": framework.NewStatus(framework.UnschedulableAndUnresolvable, "injected unschedulable status").WithFailedPlugin("FakePreFilter"), - "2": framework.NewStatus(framework.UnschedulableAndUnresolvable, "injected unschedulable status").WithFailedPlugin("FakePreFilter"), - }, + NodeToStatusMap: framework.NodeToStatusMap{}, + PreFilterMsg: "injected unschedulable status", UnschedulablePlugins: sets.NewString("FakePreFilter"), }, }, @@ -1948,12 +1942,9 @@ func TestSchedulerSchedulePod(t *testing.T) { Pod: st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(), NumAllNodes: 3, Diagnosis: framework.Diagnosis{ - NodeToStatusMap: framework.NodeToStatusMap{ - "node1": framework.NewStatus(framework.Unschedulable, "node(s) didn't satisfy plugin(s) [FakePreFilter2 FakePreFilter3] simultaneously"), - "node2": framework.NewStatus(framework.Unschedulable, "node(s) didn't satisfy plugin(s) [FakePreFilter2 FakePreFilter3] simultaneously"), - "node3": framework.NewStatus(framework.Unschedulable, "node(s) didn't satisfy plugin(s) [FakePreFilter2 FakePreFilter3] simultaneously"), - }, + NodeToStatusMap: framework.NodeToStatusMap{}, UnschedulablePlugins: sets.String{}, + PreFilterMsg: "node(s) didn't satisfy plugin(s) [FakePreFilter2 FakePreFilter3] simultaneously", }, }, }, @@ -1977,10 +1968,9 @@ func TestSchedulerSchedulePod(t *testing.T) { Pod: st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(), NumAllNodes: 1, Diagnosis: framework.Diagnosis{ - NodeToStatusMap: framework.NodeToStatusMap{ - "node1": framework.NewStatus(framework.Unschedulable, "node(s) didn't satisfy plugin FakePreFilter2"), - }, + NodeToStatusMap: framework.NodeToStatusMap{}, UnschedulablePlugins: sets.String{}, + PreFilterMsg: "node(s) didn't satisfy plugin FakePreFilter2", }, }, },