From 5fbe58b2c88bc951d7fc90878f3415f79f4c2ee8 Mon Sep 17 00:00:00 2001
From: Wojciech Tyczynski <wojtekt@google.com>
Date: Tue, 19 May 2015 14:27:28 -0700
Subject: [PATCH] Revert "Make scheduler optimistic about its bindings"

---
 plugin/pkg/scheduler/scheduler.go      | 35 +++++++++-----------------
 plugin/pkg/scheduler/scheduler_test.go | 11 +++-----
 2 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/plugin/pkg/scheduler/scheduler.go b/plugin/pkg/scheduler/scheduler.go
index 6b51cd4e790..7d3830fcac2 100644
--- a/plugin/pkg/scheduler/scheduler.go
+++ b/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 func() {}
+		return
 	}
 	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.
-	return func() {
-		defer recordTime()
-		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)
-
+	// We want to add the pod to the model iff the bind succeeds, but we don't want to race
+	// with any deletions, which happen asyncronously.
+	s.config.Modeler.LockedAction(func() {
 		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)
-	}
-}
-
-func (s *Scheduler) scheduleOne() {
-	bind := s.schedule()
-	go bind()
+		// tell the model to assume that this binding took effect.
+		assumed := *pod
+		assumed.Spec.Host = dest
+		s.config.Modeler.AssumePod(&assumed)
+	})
 }
diff --git a/plugin/pkg/scheduler/scheduler_test.go b/plugin/pkg/scheduler/scheduler_test.go
index 9ec4c7029fb..39093a4c2e8 100644
--- a/plugin/pkg/scheduler/scheduler_test.go
+++ b/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"},