Revert "Make scheduler optimistic about its bindings"

plugin/pkg/scheduler/scheduler.go

@@ -104,21 +104,20 @@ func (s *Scheduler) Run() {
 	go util.Until(s.scheduleOne, 0, s.config.StopEverything)
 }
 
-func (s *Scheduler) schedule() (executeBinding func()) {
+func (s *Scheduler) scheduleOne() {
 	pod := s.config.NextPod()
 	glog.V(3).Infof("Attempting to schedule: %v", pod)
 	start := time.Now()
-	recordTime := func() {
+	defer func() {
 		metrics.E2eSchedulingLatency.Observe(metrics.SinceInMicroseconds(start))
-	}
+	}()
 	dest, err := s.config.Algorithm.Schedule(pod, s.config.MinionLister)
 	metrics.SchedulingAlgorithmLatency.Observe(metrics.SinceInMicroseconds(start))
 	if err != nil {
 		glog.V(1).Infof("Failed to schedule: %v", pod)
 		s.config.Recorder.Eventf(pod, "failedScheduling", "Error scheduling: %v", err)
 		s.config.Error(pod, err)
-		recordTime()
+		return
-		return func() {}
 	}
 	b := &api.Binding{
 		ObjectMeta: api.ObjectMeta{Namespace: pod.Namespace, Name: pod.Name},
@@ -128,32 +127,22 @@ func (s *Scheduler) schedule() (executeBinding func()) {
 		},
 	}
 
-	// Actually do the binding asynchronously with respect to the scheduling queue.
+	// We want to add the pod to the model iff the bind succeeds, but we don't want to race
-	return func() {
+	// with any deletions, which happen asyncronously.
-		defer recordTime()
+	s.config.Modeler.LockedAction(func() {
-		defer util.HandleCrash()
-
-		// Make an object representing our assumtion that the bind will succeed.
-		assumed := *pod
-		assumed.Spec.Host = dest
-		s.config.Modeler.AssumePod(&assumed)
-
 		bindingStart := time.Now()
 		err := s.config.Binder.Bind(b)
 		metrics.BindingLatency.Observe(metrics.SinceInMicroseconds(bindingStart))
 		if err != nil {
-			// Remove our (now invalid) assumption
-			s.config.Modeler.ForgetPod(&assumed)
 			glog.V(1).Infof("Failed to bind pod: %v", err)
 			s.config.Recorder.Eventf(pod, "failedScheduling", "Binding rejected: %v", err)
 			s.config.Error(pod, err)
 			return
 		}
 		s.config.Recorder.Eventf(pod, "scheduled", "Successfully assigned %v to %v", pod.Name, dest)
-	}
+		// tell the model to assume that this binding took effect.
-}
+		assumed := *pod
-
+		assumed.Spec.Host = dest
-func (s *Scheduler) scheduleOne() {
+		s.config.Modeler.AssumePod(&assumed)
-	bind := s.schedule()
+	})
-	go bind()
 }

plugin/pkg/scheduler/scheduler_test.go

@@ -113,11 +113,6 @@ func TestScheduler(t *testing.T) {
 				AssumePodFunc: func(pod *api.Pod) {
 					gotAssumedPod = pod
 				},
-				ForgetPodFunc: func(pod *api.Pod) {
-					if gotAssumedPod != nil && gotAssumedPod.Name == pod.Name && gotAssumedPod.Namespace == pod.Namespace {
-						gotAssumedPod = nil
-					}
-				},
 			},
 			MinionLister: algorithm.FakeMinionLister(
 				api.NodeList{Items: []api.Node{{ObjectMeta: api.ObjectMeta{Name: "machine1"}}}},
@@ -144,7 +139,7 @@ func TestScheduler(t *testing.T) {
 			}
 			close(called)
 		})
-		s.schedule()()
+		s.scheduleOne()
 		if e, a := item.expectAssumedPod, gotAssumedPod; !reflect.DeepEqual(e, a) {
 			t.Errorf("%v: assumed pod: wanted %v, got %v", i, e, a)
 		}
@@ -234,7 +229,7 @@ func TestSchedulerForgetAssumedPodAfterDelete(t *testing.T) {
 	// scheduledPodStore: []
 	// assumedPods: []
 
-	s.schedule()()
+	s.scheduleOne()
 	// queuedPodStore: []
 	// scheduledPodStore: [foo:8080]
 	// assumedPods: [foo:8080]
@@ -288,7 +283,7 @@ func TestSchedulerForgetAssumedPodAfterDelete(t *testing.T) {
 		close(called)
 	})
 
-	s.schedule()()
+	s.scheduleOne()
 
 	expectBind = &api.Binding{
 		ObjectMeta: api.ObjectMeta{Name: "bar"},