Refactor the code to make it more readable.

pkg/scheduler/generic_scheduler.go

@@ -33,47 +33,26 @@ type genericScheduler struct {
 	randomLock  sync.Mutex
 }
 
-type listMinionLister struct {
-	nodes []string
-}
-
-func (l *listMinionLister) List() ([]string, error) {
-	return l.nodes, nil
-}
-
 func (g *genericScheduler) Schedule(pod api.Pod, minionLister MinionLister) (string, error) {
 	minions, err := minionLister.List()
 	if err != nil {
 		return "", err
 	}
-	filtered := []string{}
+	filteredNodes, err := findNodesThatFit(pod, g.pods, g.predicates, minions)
-	machineToPods, err := MapPodsToMachines(g.pods)
 	if err != nil {
 		return "", err
 	}
-	for _, minion := range minions {
+	priorityList, err := g.prioritizer(pod, g.pods, FakeMinionLister(filteredNodes))
-		fits := true
-		for _, predicate := range g.predicates {
-			fit, err := predicate(pod, machineToPods[minion], minion)
-			if err != nil {
-				return "", err
-			}
-			if !fit {
-				fits = false
-				break
-			}
-		}
-		if fits {
-			filtered = append(filtered, minion)
-		}
-	}
-	priorityList, err := g.prioritizer(pod, g.pods, &listMinionLister{filtered})
 	if err != nil {
 		return "", err
 	}
 	if len(priorityList) == 0 {
 		return "", fmt.Errorf("failed to find a fit for pod: %v", pod)
 	}
+	return g.selectHost(priorityList)
+}
+
+func (g *genericScheduler) selectHost(priorityList HostPriorityList) (string, error) {
 	sort.Sort(priorityList)
 
 	hosts := getMinHosts(priorityList)
@@ -84,11 +63,38 @@ func (g *genericScheduler) Schedule(pod api.Pod, minionLister MinionLister) (str
 	return hosts[ix], nil
 }
 
+func findNodesThatFit(pod api.Pod, podLister PodLister, predicates []FitPredicate, nodes []string) ([]string, error) {
+	filtered := []string{}
+	machineToPods, err := MapPodsToMachines(podLister)
+	if err != nil {
+		return nil, err
+	}
+	for _, node := range nodes {
+		fits := true
+		for _, predicate := range predicates {
+			fit, err := predicate(pod, machineToPods[node], node)
+			if err != nil {
+				return nil, err
+			}
+			if !fit {
+				fits = false
+				break
+			}
+		}
+		if fits {
+			filtered = append(filtered, node)
+		}
+	}
+	return filtered, nil
+}
+
 func getMinHosts(list HostPriorityList) []string {
 	result := []string{}
 	for _, hostEntry := range list {
 		if hostEntry.score == list[0].score {
 			result = append(result, hostEntry.host)
+		} else {
+			break
 		}
 	}
 	return result

pkg/scheduler/generic_scheduler_test.go

@@ -25,14 +25,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
-func falsePredicate(pod api.Pod, existingPods []api.Pod, node string) (bool, error) {
-	return false, nil
-}
-
-func truePredicate(pod api.Pod, existingPods []api.Pod, node string) (bool, error) {
-	return true, nil
-}
-
 func matchesPredicate(pod api.Pod, existingPods []api.Pod, node string) (bool, error) {
 	return pod.ID == node, nil
 }
@@ -80,72 +72,56 @@ func TestGenericScheduler(t *testing.T) {
 		predicates   []FitPredicate
 		prioritizer  PriorityFunction
 		nodes        []string
-		existingPods []api.Pod
 		pod          api.Pod
 		expectedHost string
 		expectsErr   bool
 	}{
 		{
-			[]FitPredicate{falsePredicate},
+			predicates:  []FitPredicate{falsePredicate},
-			evenPriority,
+			prioritizer: evenPriority,
-			[]string{"machine1", "machine2"},
+			nodes:       []string{"machine1", "machine2"},
-			[]api.Pod{},
+			expectsErr:  true,
-			api.Pod{},
-			"",
-			true,
 		},
 		{
-			[]FitPredicate{truePredicate},
+			predicates:  []FitPredicate{truePredicate},
-			evenPriority,
+			prioritizer: evenPriority,
-			[]string{"machine1", "machine2"},
+			nodes:       []string{"machine1", "machine2"},
-			[]api.Pod{},
-			api.Pod{},
 			// Random choice between both, the rand seeded above with zero, chooses "machine2"
-			"machine2",
+			expectedHost: "machine2",
-			false,
 		},
 		{
-			[]FitPredicate{matchesPredicate},
+			// Fits on a machine where the pod ID matches the machine name
-			evenPriority,
+			predicates:   []FitPredicate{matchesPredicate},
-			[]string{"machine1", "machine2"},
+			prioritizer:  evenPriority,
-			[]api.Pod{},
+			nodes:        []string{"machine1", "machine2"},
-			api.Pod{JSONBase: api.JSONBase{ID: "machine2"}},
+			pod:          api.Pod{JSONBase: api.JSONBase{ID: "machine2"}},
-			"machine2",
+			expectedHost: "machine2",
-			false,
 		},
 		{
-			[]FitPredicate{truePredicate},
+			predicates:   []FitPredicate{truePredicate},
-			numericPriority,
+			prioritizer:  numericPriority,
-			[]string{"3", "2", "1"},
+			nodes:        []string{"3", "2", "1"},
-			[]api.Pod{},
+			expectedHost: "1",
-			api.Pod{},
-			"1",
-			false,
 		},
 		{
-			[]FitPredicate{matchesPredicate},
+			predicates:   []FitPredicate{matchesPredicate},
-			numericPriority,
+			prioritizer:  numericPriority,
-			[]string{"3", "2", "1"},
+			nodes:        []string{"3", "2", "1"},
-			[]api.Pod{},
+			pod:          api.Pod{JSONBase: api.JSONBase{ID: "2"}},
-			api.Pod{JSONBase: api.JSONBase{ID: "2"}},
+			expectedHost: "2",
-			"2",
-			false,
 		},
 		{
-			[]FitPredicate{truePredicate, falsePredicate},
+			predicates:  []FitPredicate{truePredicate, falsePredicate},
-			numericPriority,
+			prioritizer: numericPriority,
-			[]string{"3", "2", "1"},
+			nodes:       []string{"3", "2", "1"},
-			[]api.Pod{},
+			expectsErr:  true,
-			api.Pod{},
-			"",
-			true,
 		},
 	}
 
 	for _, test := range tests {
 		random := rand.New(rand.NewSource(0))
-		scheduler := NewGenericScheduler(test.predicates, test.prioritizer, FakePodLister(test.existingPods), random)
+		scheduler := NewGenericScheduler(test.predicates, test.prioritizer, FakePodLister([]api.Pod{}), random)
-		machine, err := scheduler.Schedule(test.pod, &listMinionLister{nodes: test.nodes})
+		machine, err := scheduler.Schedule(test.pod, FakeMinionLister(test.nodes))
 		if test.expectsErr {
 			if err == nil {
 				t.Error("Unexpected non-error")

pkg/scheduler/randomfit.go

@@ -25,9 +25,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
 )
 
-// FitPredicate is a function that indicates if a pod fits into an existing node.
-type FitPredicate func(pod api.Pod, existingPods []api.Pod, node string) (bool, error)
-
 // RandomFitScheduler is a Scheduler which schedules a Pod on a random machine which matches its requirement.
 type RandomFitScheduler struct {
 	podLister  PodLister
@@ -78,6 +75,8 @@ func containsPort(pod api.Pod, port api.Port) bool {
 	return false
 }
 
+// MapPodsToMachines obtains a list of pods and pivots that list into a map where the keys are host names
+// and the values are the list of pods running on that host.
 func MapPodsToMachines(lister PodLister) (map[string][]api.Pod, error) {
 	machineToPods := map[string][]api.Pod{}
 	// TODO: perform more targeted query...

pkg/scheduler/spreading_scheduler_test.go

@@ -42,28 +42,70 @@ func TestSpreadPriority(t *testing.T) {
 		pod          api.Pod
 		pods         []api.Pod
 		nodes        []string
-		expectErr    bool
 		expectedList HostPriorityList
 		test         string
 	}{
-		{api.Pod{}, []api.Pod{}, []string{"machine1", "machine2"}, false, []HostPriority{{"machine1", 0}, {"machine2", 0}}, "nothing scheduled"},
+		{
-		{api.Pod{Labels: labels1}, []api.Pod{{CurrentState: machine1State}}, []string{"machine1", "machine2"}, false, []HostPriority{{"machine1", 0}, {"machine2", 0}}, "no labels"},
+			nodes:        []string{"machine1", "machine2"},
-		{api.Pod{Labels: labels1}, []api.Pod{{CurrentState: machine1State, Labels: labels2}}, []string{"machine1", "machine2"}, false, []HostPriority{{"machine1", 0}, {"machine2", 0}}, "different labels"},
+			expectedList: []HostPriority{{"machine1", 0}, {"machine2", 0}},
-		{api.Pod{Labels: labels1}, []api.Pod{{CurrentState: machine1State, Labels: labels2}, {CurrentState: machine2State, Labels: labels1}}, []string{"machine1", "machine2"}, false, []HostPriority{{"machine1", 0}, {"machine2", 1}}, "one label match"},
+			test:         "nothing scheduled",
-		{api.Pod{Labels: labels1}, []api.Pod{{CurrentState: machine1State, Labels: labels2}, {CurrentState: machine1State, Labels: labels1}, {CurrentState: machine2State, Labels: labels1}}, []string{"machine1", "machine2"}, false, []HostPriority{{"machine1", 1}, {"machine2", 1}}, "two label matches on different machines"},
+		},
-		{api.Pod{Labels: labels1}, []api.Pod{{CurrentState: machine1State, Labels: labels2}, {CurrentState: machine1State, Labels: labels1}, {CurrentState: machine2State, Labels: labels1}, {CurrentState: machine2State, Labels: labels1}}, []string{"machine1", "machine2"}, false, []HostPriority{{"machine1", 1}, {"machine2", 2}}, "three label matches"},
+		{
+			pod:          api.Pod{Labels: labels1},
+			pods:         []api.Pod{{CurrentState: machine1State}},
+			nodes:        []string{"machine1", "machine2"},
+			expectedList: []HostPriority{{"machine1", 0}, {"machine2", 0}},
+			test:         "no labels",
+		},
+		{
+			pod:          api.Pod{Labels: labels1},
+			pods:         []api.Pod{{CurrentState: machine1State, Labels: labels2}},
+			nodes:        []string{"machine1", "machine2"},
+			expectedList: []HostPriority{{"machine1", 0}, {"machine2", 0}},
+			test:         "different labels",
+		},
+		{
+			pod: api.Pod{Labels: labels1},
+			pods: []api.Pod{
+				{CurrentState: machine1State, Labels: labels2},
+				{CurrentState: machine2State, Labels: labels1},
+			},
+			nodes:        []string{"machine1", "machine2"},
+			expectedList: []HostPriority{{"machine1", 0}, {"machine2", 1}},
+			test:         "one label match",
+		},
+		{
+			pod: api.Pod{Labels: labels1},
+			pods: []api.Pod{
+				{CurrentState: machine1State, Labels: labels2},
+				{CurrentState: machine1State, Labels: labels1},
+				{CurrentState: machine2State, Labels: labels1},
+			},
+			nodes:        []string{"machine1", "machine2"},
+			expectedList: []HostPriority{{"machine1", 1}, {"machine2", 1}},
+			test:         "two label matches on different machines",
+		},
+		{
+			pod: api.Pod{Labels: labels1},
+			pods: []api.Pod{
+				{CurrentState: machine1State, Labels: labels2},
+				{CurrentState: machine1State, Labels: labels1},
+				{CurrentState: machine2State, Labels: labels1},
+				{CurrentState: machine2State, Labels: labels1},
+			},
+			nodes:        []string{"machine1", "machine2"},
+			expectedList: []HostPriority{{"machine1", 1}, {"machine2", 2}},
+			test:         "three label matches",
+		},
 	}
 
 	for _, test := range tests {
-		list, err := CalculateSpreadPriority(test.pod, FakePodLister(test.pods), &listMinionLister{test.nodes})
+		list, err := CalculateSpreadPriority(test.pod, FakePodLister(test.pods), FakeMinionLister(test.nodes))
-		if test.expectErr {
+		if err != nil {
-			if err == nil {
+			t.Errorf("unexpected error: %v", err)
-				t.Errorf("%s: unexpected non-error", test.test)
+		}
-			}
+		if !reflect.DeepEqual(test.expectedList, list) {
-		} else {
+			t.Errorf("%s: expected %#v, got %#v", test.test, test.expectedList, list)
-			if !reflect.DeepEqual(test.expectedList, list) {
-				t.Errorf("%s: expected %#v, got %#v", test.test, test.expectedList, list)
-			}
 		}
 	}
 }