From 54ac023a53d1bcfb1677631794f7d93008205c8e Mon Sep 17 00:00:00 2001 From: Abdullah Gharaibeh Date: Wed, 11 Sep 2019 14:03:18 -0400 Subject: [PATCH] Scheduler PreBind plugins are currently allowed to return Unschedulable status, which should not according to the KEP and comments. --- pkg/scheduler/framework/v1alpha1/framework.go | 5 -- pkg/scheduler/scheduler.go | 9 +- test/integration/scheduler/framework_test.go | 85 ++++--------------- 3 files changed, 20 insertions(+), 79 deletions(-) diff --git a/pkg/scheduler/framework/v1alpha1/framework.go b/pkg/scheduler/framework/v1alpha1/framework.go index cb2aca2c18b..57c14b5141a 100644 --- a/pkg/scheduler/framework/v1alpha1/framework.go +++ b/pkg/scheduler/framework/v1alpha1/framework.go @@ -433,11 +433,6 @@ func (f *framework) RunPreBindPlugins( for _, pl := range f.preBindPlugins { status := pl.PreBind(pc, pod, nodeName) if !status.IsSuccess() { - if status.IsUnschedulable() { - msg := fmt.Sprintf("rejected by %q at prebind: %v", pl.Name(), status.Message()) - klog.V(4).Infof(msg) - return NewStatus(status.Code(), msg) - } msg := fmt.Sprintf("error while running %q prebind plugin for pod %q: %v", pl.Name(), pod.Name, status.Message()) klog.Error(msg) return NewStatus(Error, msg) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 5847b6bebea..67b2ac47b3f 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -635,13 +635,8 @@ func (sched *Scheduler) scheduleOne() { preBindStatus := fwk.RunPreBindPlugins(pluginContext, assumedPod, scheduleResult.SuggestedHost) if !preBindStatus.IsSuccess() { var reason string - if preBindStatus.IsUnschedulable() { - metrics.PodScheduleFailures.Inc() - reason = v1.PodReasonUnschedulable - } else { - metrics.PodScheduleErrors.Inc() - reason = SchedulerError - } + metrics.PodScheduleErrors.Inc() + reason = SchedulerError if forgetErr := sched.Cache().ForgetPod(assumedPod); forgetErr != nil { klog.Errorf("scheduler cache ForgetPod failed: %v", forgetErr) } diff --git a/test/integration/scheduler/framework_test.go b/test/integration/scheduler/framework_test.go index 22d0020137e..61729fbf580 100644 --- a/test/integration/scheduler/framework_test.go +++ b/test/integration/scheduler/framework_test.go @@ -732,20 +732,12 @@ func TestPrebindPlugin(t *testing.T) { t.Errorf("Error while creating a test pod: %v", err) } - if test.fail { + if test.fail || test.reject { if err = wait.Poll(10*time.Millisecond, 30*time.Second, podSchedulingError(cs, pod.Namespace, pod.Name)); err != nil { t.Errorf("test #%v: Expected a scheduling error, but didn't get it. error: %v", i, err) } - } else { - if test.reject { - if err = waitForPodUnschedulable(cs, pod); err != nil { - t.Errorf("test #%v: Didn't expected the pod to be scheduled. error: %v", i, err) - } - } else { - if err = waitForPodToSchedule(cs, pod); err != nil { - t.Errorf("test #%v: Expected the pod to be scheduled. error: %v", i, err) - } - } + } else if err = waitForPodToSchedule(cs, pod); err != nil { + t.Errorf("test #%v: Expected the pod to be scheduled. error: %v", i, err) } if preBindPlugin.numPreBindCalled == 0 { @@ -810,30 +802,18 @@ func TestUnreservePlugin(t *testing.T) { } tests := []struct { - preBindFail bool - preBindReject bool + preBindFail bool }{ { - preBindFail: false, - preBindReject: false, + preBindFail: false, }, { - preBindFail: true, - preBindReject: false, - }, - { - preBindFail: false, - preBindReject: true, - }, - { - preBindFail: true, - preBindReject: true, + preBindFail: true, }, } for i, test := range tests { preBindPlugin.failPreBind = test.preBindFail - preBindPlugin.rejectPreBind = test.preBindReject // Create a best effort pod. pod, err := createPausePod(cs, @@ -850,20 +830,11 @@ func TestUnreservePlugin(t *testing.T) { t.Errorf("test #%v: Expected the unreserve plugin to be called %d times, was called %d times.", i, preBindPlugin.numPreBindCalled, unreservePlugin.numUnreserveCalled) } } else { - if test.preBindReject { - if err = waitForPodUnschedulable(cs, pod); err != nil { - t.Errorf("test #%v: Didn't expected the pod to be scheduled. error: %v", i, err) - } - if unreservePlugin.numUnreserveCalled == 0 { - t.Errorf("test #%v: Expected the unreserve plugin to be called %d times, was called %d times.", i, preBindPlugin.numPreBindCalled, unreservePlugin.numUnreserveCalled) - } - } else { - if err = waitForPodToSchedule(cs, pod); err != nil { - t.Errorf("test #%v: Expected the pod to be scheduled. error: %v", i, err) - } - if unreservePlugin.numUnreserveCalled > 0 { - t.Errorf("test #%v: Didn't expected the unreserve plugin to be called, was called %d times.", i, unreservePlugin.numUnreserveCalled) - } + if err = waitForPodToSchedule(cs, pod); err != nil { + t.Errorf("test #%v: Expected the pod to be scheduled. error: %v", i, err) + } + if unreservePlugin.numUnreserveCalled > 0 { + t.Errorf("test #%v: Didn't expected the unreserve plugin to be called, was called %d times.", i, unreservePlugin.numUnreserveCalled) } } @@ -1122,26 +1093,15 @@ func TestPostBindPlugin(t *testing.T) { preBindReject bool }{ { - preBindFail: false, - preBindReject: false, + preBindFail: false, }, { - preBindFail: true, - preBindReject: false, - }, - { - preBindFail: false, - preBindReject: true, - }, - { - preBindFail: true, - preBindReject: true, + preBindFail: true, }, } for i, test := range tests { preBindPlugin.failPreBind = test.preBindFail - preBindPlugin.rejectPreBind = test.preBindReject // Create a best effort pod. pod, err := createPausePod(cs, @@ -1158,20 +1118,11 @@ func TestPostBindPlugin(t *testing.T) { t.Errorf("test #%v: Didn't expected the postbind plugin to be called %d times.", i, postBindPlugin.numPostBindCalled) } } else { - if test.preBindReject { - if err = waitForPodUnschedulable(cs, pod); err != nil { - t.Errorf("test #%v: Didn't expected the pod to be scheduled. error: %v", i, err) - } - if postBindPlugin.numPostBindCalled > 0 { - t.Errorf("test #%v: Didn't expected the postbind plugin to be called %d times.", i, postBindPlugin.numPostBindCalled) - } - } else { - if err = waitForPodToSchedule(cs, pod); err != nil { - t.Errorf("test #%v: Expected the pod to be scheduled. error: %v", i, err) - } - if postBindPlugin.numPostBindCalled == 0 { - t.Errorf("test #%v: Expected the postbind plugin to be called, was called %d times.", i, postBindPlugin.numPostBindCalled) - } + if err = waitForPodToSchedule(cs, pod); err != nil { + t.Errorf("test #%v: Expected the pod to be scheduled. error: %v", i, err) + } + if postBindPlugin.numPostBindCalled == 0 { + t.Errorf("test #%v: Expected the postbind plugin to be called, was called %d times.", i, postBindPlugin.numPostBindCalled) } }