From 7f041297364f927cf1a6b91ecdc43e027b38ae90 Mon Sep 17 00:00:00 2001 From: Harry Zhang Date: Mon, 19 Mar 2018 12:15:24 -0700 Subject: [PATCH 1/2] Add Ignorable flag to extender Ignore extender in generic scheduler Add test to verify the ignorable flag Fix warning msg --- .../algorithm/scheduler_interface.go | 4 +++ pkg/scheduler/api/types.go | 3 ++ pkg/scheduler/api/v1/types.go | 3 ++ pkg/scheduler/core/extender.go | 13 +++++++-- pkg/scheduler/core/extender_test.go | 27 ++++++++++++++++++ pkg/scheduler/core/generic_scheduler.go | 28 +++++++++++++++---- pkg/scheduler/factory/factory_test.go | 5 ++++ 7 files changed, 76 insertions(+), 7 deletions(-) diff --git a/pkg/scheduler/algorithm/scheduler_interface.go b/pkg/scheduler/algorithm/scheduler_interface.go index 61c740c60a..e43077151f 100644 --- a/pkg/scheduler/algorithm/scheduler_interface.go +++ b/pkg/scheduler/algorithm/scheduler_interface.go @@ -64,6 +64,10 @@ type SchedulerExtender interface { // SupportsPreemption returns if the scheduler extender support preemption or not. SupportsPreemption() bool + + // IsIgnorable returns true indicates scheduling should not fail when this extender + // is unavailable. This gives scheduler ability to fail fast and tolerate non-critical extenders as well. + IsIgnorable() bool } // ScheduleAlgorithm is an interface implemented by things that know how to schedule pods diff --git a/pkg/scheduler/api/types.go b/pkg/scheduler/api/types.go index 7b31dfe7fe..8ce5d205bb 100644 --- a/pkg/scheduler/api/types.go +++ b/pkg/scheduler/api/types.go @@ -192,6 +192,9 @@ type ExtenderConfig struct { // will skip checking the resource in predicates. // +optional ManagedResources []ExtenderManagedResource + // Ignorable specifies if the extender is ignorable, i.e. scheduling should not + // fail when the extender returns an error or is not reachable. + Ignorable bool } // ExtenderPreemptionResult represents the result returned by preemption phase of extender. diff --git a/pkg/scheduler/api/v1/types.go b/pkg/scheduler/api/v1/types.go index 80ad839a03..21412c3ea1 100644 --- a/pkg/scheduler/api/v1/types.go +++ b/pkg/scheduler/api/v1/types.go @@ -174,6 +174,9 @@ type ExtenderConfig struct { // will skip checking the resource in predicates. // +optional ManagedResources []ExtenderManagedResource `json:"managedResources,omitempty"` + // Ignorable specifies if the extender is ignorable, i.e. scheduling should not + // fail when the extender returns an error or is not reachable. + Ignorable bool `json:"ignorable,omitempty"` } // ExtenderArgs represents the arguments needed by the extender to filter/prioritize diff --git a/pkg/scheduler/core/extender.go b/pkg/scheduler/core/extender.go index 8664ba8b52..a654905e28 100644 --- a/pkg/scheduler/core/extender.go +++ b/pkg/scheduler/core/extender.go @@ -49,6 +49,7 @@ type HTTPExtender struct { client *http.Client nodeCacheCapable bool managedResources sets.String + ignorable bool } func makeTransport(config *schedulerapi.ExtenderConfig) (http.RoundTripper, error) { @@ -102,9 +103,16 @@ func NewHTTPExtender(config *schedulerapi.ExtenderConfig) (algorithm.SchedulerEx client: client, nodeCacheCapable: config.NodeCacheCapable, managedResources: managedResources, + ignorable: config.Ignorable, }, nil } +// IsIgnorable returns true indicates scheduling should not fail when this extender +// is unavailable +func (h *HTTPExtender) IsIgnorable() bool { + return h.ignorable +} + // SupportsPreemption returns if a extender support preemption. // A extender should have preempt verb defined and enabled its own node cache. func (h *HTTPExtender) SupportsPreemption() bool { @@ -147,11 +155,12 @@ func (h *HTTPExtender) ProcessPreemption( // Extender will always return NodeNameToMetaVictims. // So let's convert it to NodeToVictims by using NodeNameToInfo. - nodeToVictims, err := h.convertToNodeToVictims(result.NodeNameToMetaVictims, nodeNameToInfo) + newNodeToVictims, err := h.convertToNodeToVictims(result.NodeNameToMetaVictims, nodeNameToInfo) if err != nil { return nil, err } - return nodeToVictims, nil + // Do not override nodeToVictims + return newNodeToVictims, nil } // convertToNodeToVictims converts "nodeNameToMetaVictims" from object identifiers, diff --git a/pkg/scheduler/core/extender_test.go b/pkg/scheduler/core/extender_test.go index 6064ba04e3..99855a4f28 100644 --- a/pkg/scheduler/core/extender_test.go +++ b/pkg/scheduler/core/extender_test.go @@ -113,12 +113,17 @@ type FakeExtender struct { nodeCacheCapable bool filteredNodes []*v1.Node unInterested bool + ignorable bool // Cached node information for fake extender cachedNodeNameToInfo map[string]*schedulercache.NodeInfo cachedPDBs []*policy.PodDisruptionBudget } +func (f *FakeExtender) IsIgnorable() bool { + return f.ignorable +} + func (f *FakeExtender) SupportsPreemption() bool { // Assume preempt verb is always defined. return true @@ -456,6 +461,28 @@ func TestGenericSchedulerWithExtenders(t *testing.T) { expectedHost: "machine2", // machine2 has higher score name: "test 8", }, + { + // Scheduling is expected to not fail in + // Filter/Prioritize phases if the extender is not available and ignorable. + // + // If scheduler did not ignore the extender, the test will fail + // because of the errors from errorPredicateExtender. + predicates: map[string]algorithm.FitPredicate{"true": truePredicate}, + prioritizers: []algorithm.PriorityConfig{{Map: EqualPriorityMap, Weight: 1}}, + extenders: []FakeExtender{ + { + predicates: []fitPredicate{errorPredicateExtender}, + ignorable: true, + }, + { + predicates: []fitPredicate{machine1PredicateExtender}, + }, + }, + nodes: []string{"machine1", "machine2"}, + expectsErr: false, + expectedHost: "machine1", + name: "test 9", + }, } for _, test := range tests { diff --git a/pkg/scheduler/core/generic_scheduler.go b/pkg/scheduler/core/generic_scheduler.go index 7571b2cf69..365fc79ffe 100644 --- a/pkg/scheduler/core/generic_scheduler.go +++ b/pkg/scheduler/core/generic_scheduler.go @@ -269,12 +269,24 @@ func (g *genericScheduler) processPreemptionWithExtenders( if len(nodeToVictims) > 0 { for _, extender := range g.extenders { if extender.SupportsPreemption() { - var err error - // Replace nodeToVictims with result after preemption from extender. - if nodeToVictims, err = extender.ProcessPreemption(pod, nodeToVictims, g.cachedNodeInfoMap); err != nil { + newNodeToVictims, err := extender.ProcessPreemption( + pod, + nodeToVictims, + g.cachedNodeInfoMap, + ) + if err != nil { + if extender.IsIgnorable() { + glog.Warningf("Skipping extender %v as it returned error %v and has ignorable flag set", + extender, err) + continue + } return nil, err } - // If node list is empty, no preemption will happen, skip other extenders. + // Replace nodeToVictims with new result after preemption. So the + // rest of extenders can continue use it as parameter. + nodeToVictims = newNodeToVictims + + // If node list becomes empty, no preemption can happen regardless of other extenders. if len(nodeToVictims) == 0 { break } @@ -384,7 +396,13 @@ func findNodesThatFit( } filteredList, failedMap, err := extender.Filter(pod, filtered, nodeNameToInfo) if err != nil { - return []*v1.Node{}, FailedPredicateMap{}, err + if extender.IsIgnorable() { + glog.Warningf("Skipping extender %v as it returned error %v and has ignorable flag set", + extender, err) + continue + } else { + return []*v1.Node{}, FailedPredicateMap{}, err + } } for failedNodeName, failedMsg := range failedMap { diff --git a/pkg/scheduler/factory/factory_test.go b/pkg/scheduler/factory/factory_test.go index 924e48bb4d..a49e2e0198 100644 --- a/pkg/scheduler/factory/factory_test.go +++ b/pkg/scheduler/factory/factory_test.go @@ -539,6 +539,11 @@ func newConfigFactory(client *clientset.Clientset, hardPodAffinitySymmetricWeigh type fakeExtender struct { isBinder bool interestedPodName string + ignorable bool +} + +func (f *fakeExtender) IsIgnorable() bool { + return f.ignorable } func (f *fakeExtender) ProcessPreemption( From 083684d771f224b61cb67938e65496a327dda41c Mon Sep 17 00:00:00 2001 From: Harry Zhang Date: Fri, 30 Mar 2018 14:01:09 -0700 Subject: [PATCH 2/2] Add test to verify preempt ignore --- pkg/scheduler/core/extender_test.go | 32 +++++++++++++------- pkg/scheduler/core/generic_scheduler.go | 1 + pkg/scheduler/core/generic_scheduler_test.go | 24 +++++++++++++++ 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/pkg/scheduler/core/extender_test.go b/pkg/scheduler/core/extender_test.go index 99855a4f28..f4ad3e00c0 100644 --- a/pkg/scheduler/core/extender_test.go +++ b/pkg/scheduler/core/extender_test.go @@ -146,7 +146,10 @@ func (f *FakeExtender) ProcessPreemption( for node, victims := range nodeToVictimsCopy { // Try to do preemption on extender side. - extenderVictimPods, extendernPDBViolations, fits := f.selectVictimsOnNodeByExtender(pod, node, nodeNameToInfo) + extenderVictimPods, extendernPDBViolations, fits, err := f.selectVictimsOnNodeByExtender(pod, node, nodeNameToInfo) + if err != nil { + return nil, err + } // If it's unfit after extender's preemption, this node is unresolvable by preemption overall, // let's remove it from potential preemption nodes. if !fits { @@ -169,15 +172,18 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender( pod *v1.Pod, node *v1.Node, nodeNameToInfo map[string]*schedulercache.NodeInfo, -) ([]*v1.Pod, int, bool) { - // TODO(harry): add more test in generic_scheduler_test.go to verify this logic. +) ([]*v1.Pod, int, bool, error) { // If a extender support preemption but have no cached node info, let's run filter to make sure // default scheduler's decision still stand with given pod and node. if !f.nodeCacheCapable { - if fits, _ := f.runPredicate(pod, node); !fits { - return nil, 0, false + fits, err := f.runPredicate(pod, node) + if err != nil { + return nil, 0, false, err } - return []*v1.Pod{}, 0, true + if !fits { + return nil, 0, false, nil + } + return []*v1.Pod{}, 0, true, nil } // Otherwise, as a extender support preemption and have cached node info, we will assume cachedNodeNameToInfo is available @@ -205,8 +211,12 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender( // If the new pod does not fit after removing all the lower priority pods, // we are almost done and this node is not suitable for preemption. - if fits, _ := f.runPredicate(pod, nodeInfoCopy.Node()); !fits { - return nil, 0, false + fits, err := f.runPredicate(pod, nodeInfoCopy.Node()) + if err != nil { + return nil, 0, false, err + } + if !fits { + return nil, 0, false, nil } var victims []*v1.Pod @@ -230,7 +240,7 @@ func (f *FakeExtender) selectVictimsOnNodeByExtender( reprievePod(p.(*v1.Pod)) } - return victims, numViolatingVictim, true + return victims, numViolatingVictim, true, nil } // runPredicate run predicates of extender one by one for given pod and node. @@ -444,7 +454,7 @@ func TestGenericSchedulerWithExtenders(t *testing.T) { // Filter/Prioritize phases if the extender is not interested in // the pod. // - // If scheduler sends the pod by mistake, the test will fail + // If scheduler sends the pod by mistake, the test would fail // because of the errors from errorPredicateExtender and/or // errorPrioritizerExtender. predicates: map[string]algorithm.FitPredicate{"true": truePredicate}, @@ -465,7 +475,7 @@ func TestGenericSchedulerWithExtenders(t *testing.T) { // Scheduling is expected to not fail in // Filter/Prioritize phases if the extender is not available and ignorable. // - // If scheduler did not ignore the extender, the test will fail + // If scheduler did not ignore the extender, the test would fail // because of the errors from errorPredicateExtender. predicates: map[string]algorithm.FitPredicate{"true": truePredicate}, prioritizers: []algorithm.PriorityConfig{{Map: EqualPriorityMap, Weight: 1}}, diff --git a/pkg/scheduler/core/generic_scheduler.go b/pkg/scheduler/core/generic_scheduler.go index 365fc79ffe..8f072d7648 100644 --- a/pkg/scheduler/core/generic_scheduler.go +++ b/pkg/scheduler/core/generic_scheduler.go @@ -282,6 +282,7 @@ func (g *genericScheduler) processPreemptionWithExtenders( } return nil, err } + // Replace nodeToVictims with new result after preemption. So the // rest of extenders can continue use it as parameter. nodeToVictims = newNodeToVictims diff --git a/pkg/scheduler/core/generic_scheduler_test.go b/pkg/scheduler/core/generic_scheduler_test.go index 05eb9c046f..696cdd096f 100644 --- a/pkg/scheduler/core/generic_scheduler_test.go +++ b/pkg/scheduler/core/generic_scheduler_test.go @@ -1274,6 +1274,30 @@ func TestPreempt(t *testing.T) { expectedNode: "", expectedPods: []string{}, }, + { + name: "One scheduler extender allows only machine1, the other returns error but ignorable. Only machine1 would be chosen", + pod: &v1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "pod1", UID: types.UID("pod1")}, Spec: v1.PodSpec{ + Containers: veryLargeContainers, + Priority: &highPriority}, + }, + pods: []*v1.Pod{ + {ObjectMeta: metav1.ObjectMeta{Name: "m1.1", UID: types.UID("m1.1")}, Spec: v1.PodSpec{Containers: smallContainers, Priority: &midPriority, NodeName: "machine1"}, Status: v1.PodStatus{Phase: v1.PodRunning}}, + {ObjectMeta: metav1.ObjectMeta{Name: "m1.2", UID: types.UID("m1.2")}, Spec: v1.PodSpec{Containers: smallContainers, Priority: &lowPriority, NodeName: "machine1"}, Status: v1.PodStatus{Phase: v1.PodRunning}}, + + {ObjectMeta: metav1.ObjectMeta{Name: "m2.1", UID: types.UID("m2.1")}, Spec: v1.PodSpec{Containers: largeContainers, Priority: &midPriority, NodeName: "machine2"}, Status: v1.PodStatus{Phase: v1.PodRunning}}, + }, + extenders: []*FakeExtender{ + { + predicates: []fitPredicate{errorPredicateExtender}, + ignorable: true, + }, + { + predicates: []fitPredicate{machine1PredicateExtender}, + }, + }, + expectedNode: "machine1", + expectedPods: []string{"m1.1", "m1.2"}, + }, } for _, test := range tests {