Refactor *Namespacer.List().

Refactor storePodsNamespacer.List() and
storeReplicationContollersNamespacer.List().  They are the same
function, just with different signatures.

This fixes a bug where, when we fell back on a brute force approach, we
were still returning an error.

Also change to explicit return without named return values.

pkg/client/cache/listers.go

@@ -76,38 +76,39 @@ type storePodsNamespacer struct {
 // Please note that selector is filtering among the pods that have gotten into
 // the store; there may have been some filtering that already happened before
 // that.
-func (s storePodsNamespacer) List(selector labels.Selector) (pods api.PodList, err error) {
+func (s storePodsNamespacer) List(selector labels.Selector) (api.PodList, error) {
-	list := api.PodList{}
+	pods := api.PodList{}
+
 	if s.namespace == api.NamespaceAll {
 		for _, m := range s.indexer.List() {
 			pod := m.(*api.Pod)
 			if selector.Matches(labels.Set(pod.Labels)) {
-				list.Items = append(list.Items, *pod)
+				pods.Items = append(pods.Items, *pod)
 			}
 		}
-		return list, nil
+		return pods, nil
 	}
 
 	key := &api.Pod{ObjectMeta: api.ObjectMeta{Namespace: s.namespace}}
 	items, err := s.indexer.Index(NamespaceIndex, key)
 	if err != nil {
+		// Ignore error; do slow search without index.
 		glog.Warningf("can not retrieve list of objects using index : %v", err)
 		for _, m := range s.indexer.List() {
 			pod := m.(*api.Pod)
 			if s.namespace == pod.Namespace && selector.Matches(labels.Set(pod.Labels)) {
-				list.Items = append(list.Items, *pod)
+				pods.Items = append(pods.Items, *pod)
 			}
 		}
-		return list, err
+		return pods, nil
 	}
-
 	for _, m := range items {
 		pod := m.(*api.Pod)
 		if selector.Matches(labels.Set(pod.Labels)) {
-			list.Items = append(list.Items, *pod)
+			pods.Items = append(pods.Items, *pod)
 		}
 	}
-	return list, nil
+	return pods, nil
 }
 
 // Exists returns true if a pod matching the namespace/name of the given pod exists in the store.
@@ -194,7 +195,9 @@ type storeReplicationControllersNamespacer struct {
 	namespace string
 }
 
-func (s storeReplicationControllersNamespacer) List(selector labels.Selector) (controllers []api.ReplicationController, err error) {
+func (s storeReplicationControllersNamespacer) List(selector labels.Selector) ([]api.ReplicationController, error) {
+	controllers := []api.ReplicationController{}
+
 	if s.namespace == api.NamespaceAll {
 		for _, m := range s.indexer.List() {
 			rc := *(m.(*api.ReplicationController))
@@ -202,12 +205,13 @@ func (s storeReplicationControllersNamespacer) List(selector labels.Selector) (c
 				controllers = append(controllers, rc)
 			}
 		}
-		return
+		return controllers, nil
 	}
 
 	key := &api.ReplicationController{ObjectMeta: api.ObjectMeta{Namespace: s.namespace}}
 	items, err := s.indexer.Index(NamespaceIndex, key)
 	if err != nil {
+		// Ignore error; do slow search without index.
 		glog.Warningf("can not retrieve list of objects using index : %v", err)
 		for _, m := range s.indexer.List() {
 			rc := *(m.(*api.ReplicationController))
@@ -215,7 +219,7 @@ func (s storeReplicationControllersNamespacer) List(selector labels.Selector) (c
 				controllers = append(controllers, rc)
 			}
 		}
-		return
+		return controllers, nil
 	}
 	for _, m := range items {
 		rc := *(m.(*api.ReplicationController))
@@ -223,7 +227,7 @@ func (s storeReplicationControllersNamespacer) List(selector labels.Selector) (c
 			controllers = append(controllers, rc)
 		}
 	}
-	return
+	return controllers, nil
 }
 
 // GetPodControllers returns a list of replication controllers managing a pod. Returns an error only if no matching controllers are found.

pkg/client/cache/listers_test.go

@@ -124,26 +124,56 @@ func TestStoreToNodeConditionLister(t *testing.T) {
 }
 
 func TestStoreToReplicationControllerLister(t *testing.T) {
-	store := NewIndexer(MetaNamespaceKeyFunc, Indexers{NamespaceIndex: MetaNamespaceIndexFunc})
-	lister := StoreToReplicationControllerLister{store}
 	testCases := []struct {
-		inRCs      []*api.ReplicationController
+		description              string
-		list       func() ([]api.ReplicationController, error)
+		inRCs                    []*api.ReplicationController
-		outRCNames sets.String
+		list                     func(StoreToReplicationControllerLister) ([]api.ReplicationController, error)
-		expectErr  bool
+		outRCNames               sets.String
+		expectErr                bool
+		onlyIfIndexedByNamespace bool
 	}{
-		// Basic listing with all labels and no selectors
 		{
+			description: "Verify we can search all namespaces",
+			inRCs: []*api.ReplicationController{
+				{
+					ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "bar"},
+				},
+				{
+					ObjectMeta: api.ObjectMeta{Name: "hmm", Namespace: "hmm"},
+				},
+			},
+			list: func(lister StoreToReplicationControllerLister) ([]api.ReplicationController, error) {
+				return lister.ReplicationControllers(api.NamespaceAll).List(labels.Set{}.AsSelector())
+			},
+			outRCNames: sets.NewString("hmm", "foo"),
+		},
+		{
+			description: "Verify we can search a specific namespace",
+			inRCs: []*api.ReplicationController{
+				{
+					ObjectMeta: api.ObjectMeta{Name: "foo", Namespace: "bar"},
+				},
+				{
+					ObjectMeta: api.ObjectMeta{Name: "hmm", Namespace: "hmm"},
+				},
+			},
+			list: func(lister StoreToReplicationControllerLister) ([]api.ReplicationController, error) {
+				return lister.ReplicationControllers("hmm").List(labels.Set{}.AsSelector())
+			},
+			outRCNames: sets.NewString("hmm"),
+		},
+		{
+			description: "Basic listing with all labels and no selectors",
 			inRCs: []*api.ReplicationController{
 				{ObjectMeta: api.ObjectMeta{Name: "basic"}},
 			},
-			list: func() ([]api.ReplicationController, error) {
+			list: func(lister StoreToReplicationControllerLister) ([]api.ReplicationController, error) {
 				return lister.List()
 			},
 			outRCNames: sets.NewString("basic"),
 		},
-		// No pod labels
 		{
+			description: "No pod labels",
 			inRCs: []*api.ReplicationController{
 				{
 					ObjectMeta: api.ObjectMeta{Name: "basic", Namespace: "ns"},
@@ -152,7 +182,7 @@ func TestStoreToReplicationControllerLister(t *testing.T) {
 					},
 				},
 			},
-			list: func() ([]api.ReplicationController, error) {
+			list: func(lister StoreToReplicationControllerLister) ([]api.ReplicationController, error) {
 				pod := &api.Pod{
 					ObjectMeta: api.ObjectMeta{Name: "pod1", Namespace: "ns"},
 				}
@@ -161,14 +191,14 @@ func TestStoreToReplicationControllerLister(t *testing.T) {
 			outRCNames: sets.NewString(),
 			expectErr:  true,
 		},
-		// No RC selectors
 		{
+			description: "No RC selectors",
 			inRCs: []*api.ReplicationController{
 				{
 					ObjectMeta: api.ObjectMeta{Name: "basic", Namespace: "ns"},
 				},
 			},
-			list: func() ([]api.ReplicationController, error) {
+			list: func(lister StoreToReplicationControllerLister) ([]api.ReplicationController, error) {
 				pod := &api.Pod{
 					ObjectMeta: api.ObjectMeta{
 						Name:      "pod1",
@@ -181,8 +211,8 @@ func TestStoreToReplicationControllerLister(t *testing.T) {
 			outRCNames: sets.NewString(),
 			expectErr:  true,
 		},
-		// Matching labels to selectors and namespace
 		{
+			description: "Matching labels to selectors and namespace",
 			inRCs: []*api.ReplicationController{
 				{
 					ObjectMeta: api.ObjectMeta{Name: "foo"},
@@ -197,7 +227,7 @@ func TestStoreToReplicationControllerLister(t *testing.T) {
 					},
 				},
 			},
-			list: func() ([]api.ReplicationController, error) {
+			list: func(lister StoreToReplicationControllerLister) ([]api.ReplicationController, error) {
 				pod := &api.Pod{
 					ObjectMeta: api.ObjectMeta{
 						Name:      "pod1",
@@ -207,30 +237,43 @@ func TestStoreToReplicationControllerLister(t *testing.T) {
 				}
 				return lister.GetPodControllers(pod)
 			},
-			outRCNames: sets.NewString("bar"),
+			outRCNames:               sets.NewString("bar"),
+			onlyIfIndexedByNamespace: true,
 		},
 	}
 	for _, c := range testCases {
-		for _, r := range c.inRCs {
+		for _, withIndex := range []bool{true, false} {
-			store.Add(r)
+			if c.onlyIfIndexedByNamespace && !withIndex {
-		}
+				continue
+			}
+			var store Indexer
+			if withIndex {
+				store = NewIndexer(MetaNamespaceKeyFunc, Indexers{NamespaceIndex: MetaNamespaceIndexFunc})
+			} else {
+				store = NewIndexer(MetaNamespaceKeyFunc, Indexers{})
+			}
 
-		gotControllers, err := c.list()
+			for _, r := range c.inRCs {
-		if err != nil && c.expectErr {
+				store.Add(r)
-			continue
+			}
-		} else if c.expectErr {
+
-			t.Error("Expected error, got none")
+			gotControllers, err := c.list(StoreToReplicationControllerLister{store})
-			continue
+			if err != nil && c.expectErr {
-		} else if err != nil {
+				continue
-			t.Errorf("Unexpected error %#v", err)
+			} else if c.expectErr {
-			continue
+				t.Errorf("(%q, withIndex=%v) Expected error, got none", c.description, withIndex)
-		}
+				continue
-		gotNames := make([]string, len(gotControllers))
+			} else if err != nil {
-		for ix := range gotControllers {
+				t.Errorf("(%q, withIndex=%v) Unexpected error %#v", c.description, withIndex, err)
-			gotNames[ix] = gotControllers[ix].Name
+				continue
-		}
+			}
-		if !c.outRCNames.HasAll(gotNames...) || len(gotNames) != len(c.outRCNames) {
+			gotNames := make([]string, len(gotControllers))
-			t.Errorf("Unexpected got controllers %+v expected %+v", gotNames, c.outRCNames)
+			for ix := range gotControllers {
+				gotNames[ix] = gotControllers[ix].Name
+			}
+			if !c.outRCNames.HasAll(gotNames...) || len(gotNames) != len(c.outRCNames) {
+				t.Errorf("(%q, withIndex=%v) Unexpected got controllers %+v expected %+v", c.description, withIndex, gotNames, c.outRCNames)
+			}
 		}
 	}
 }
@@ -645,49 +688,74 @@ func TestStoreToJobLister(t *testing.T) {
 }
 
 func TestStoreToPodLister(t *testing.T) {
-	store := NewIndexer(MetaNamespaceKeyFunc, Indexers{NamespaceIndex: MetaNamespaceIndexFunc})
+	// We test with and without a namespace index, because StoreToPodLister has
-	ids := []string{"foo", "bar", "baz"}
+	// special logic to work on namespaces even when no namespace index is
-	for _, id := range ids {
+	// present.
+	stores := []Indexer{
+		NewIndexer(MetaNamespaceKeyFunc, Indexers{NamespaceIndex: MetaNamespaceIndexFunc}),
+		NewIndexer(MetaNamespaceKeyFunc, Indexers{}),
+	}
+	for _, store := range stores {
+		ids := []string{"foo", "bar", "baz"}
+		for _, id := range ids {
+			store.Add(&api.Pod{
+				ObjectMeta: api.ObjectMeta{
+					Name:   id,
+					Labels: map[string]string{"name": id},
+				},
+			})
+		}
 		store.Add(&api.Pod{
 			ObjectMeta: api.ObjectMeta{
-				Name:   id,
+				Name:      "quux",
-				Labels: map[string]string{"name": id},
+				Namespace: api.NamespaceDefault,
+				Labels:    map[string]string{"name": "quux"},
 			},
 		})
-	}
+		spl := StoreToPodLister{store}
-	spl := StoreToPodLister{store}
 
-	for _, id := range ids {
+		// Verify that we can always look up by Namespace.
-		got, err := spl.List(labels.Set{"name": id}.AsSelector())
+		defaultPods, err := spl.Pods(api.NamespaceDefault).List(labels.Set{}.AsSelector())
 		if err != nil {
 			t.Errorf("Unexpected error: %v", err)
-			continue
+		} else if e, a := 1, len(defaultPods.Items); e != a {
-		}
-		if e, a := 1, len(got); e != a {
 			t.Errorf("Expected %v, got %v", e, a)
-			continue
+		} else if e, a := "quux", defaultPods.Items[0].Name; e != a {
-		}
-		if e, a := id, got[0].Name; e != a {
 			t.Errorf("Expected %v, got %v", e, a)
-			continue
 		}
 
-		exists, err := spl.Exists(&api.Pod{ObjectMeta: api.ObjectMeta{Name: id}})
+		for _, id := range ids {
+			got, err := spl.List(labels.Set{"name": id}.AsSelector())
+			if err != nil {
+				t.Errorf("Unexpected error: %v", err)
+				continue
+			}
+			if e, a := 1, len(got); e != a {
+				t.Errorf("Expected %v, got %v", e, a)
+				continue
+			}
+			if e, a := id, got[0].Name; e != a {
+				t.Errorf("Expected %v, got %v", e, a)
+				continue
+			}
+
+			exists, err := spl.Exists(&api.Pod{ObjectMeta: api.ObjectMeta{Name: id}})
+			if err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+			if !exists {
+				t.Errorf("exists returned false for %v", id)
+			}
+		}
+
+		exists, err := spl.Exists(&api.Pod{ObjectMeta: api.ObjectMeta{Name: "qux"}})
 		if err != nil {
 			t.Errorf("unexpected error: %v", err)
 		}
-		if !exists {
+		if exists {
-			t.Errorf("exists returned false for %v", id)
+			t.Error("Unexpected pod exists")
 		}
 	}
-
-	exists, err := spl.Exists(&api.Pod{ObjectMeta: api.ObjectMeta{Name: "qux"}})
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-	}
-	if exists {
-		t.Error("Unexpected pod exists")
-	}
 }
 
 func TestStoreToServiceLister(t *testing.T) {