Single-key matching behavior in generic.Matcher

pkg/client/cache/store.go

@@ -63,7 +63,8 @@ func (k KeyError) Error() string {
 
 // MetaNamespaceKeyFunc is a convenient default KeyFunc which knows how to make
 // keys for API objects which implement meta.Interface.
-// The key uses the format: <namespace>/<name>
+// The key uses the format <namespace>/<name> unless <namespace> is empty, then
+// it's just <name>.
 func MetaNamespaceKeyFunc(obj interface{}) (string, error) {
 	meta, err := meta.Accessor(obj)
 	if err != nil {

pkg/registry/generic/etcd/etcd.go

@@ -62,13 +62,10 @@ type Etcd struct {
 	// Used for listing/watching; should not include trailing "/"
 	KeyRootFunc func(ctx api.Context) string
 
-	// Called for Create/Update/Get/Delete
+	// Called for Create/Update/Get/Delete. Note that 'namespace' can be
+	// gotten from ctx.
 	KeyFunc func(ctx api.Context, name string) (string, error)
 
-	// If field.Selector of Watch contains a label with such name, this will be
-	// translated to watching a single object (not all objects of that type).
-	WatchSingleFieldName string
-
 	// Called to get the name of an object
 	ObjectNameFunc func(obj runtime.Object) (string, error)
 
@@ -407,30 +404,32 @@ func (e *Etcd) finalizeDelete(obj runtime.Object, runHooks bool) (runtime.Object
 	return &api.Status{Status: api.StatusSuccess}, nil
 }
 
-// WatchPredicate starts a watch for the items that m matches.
+// Watch makes a matcher for the given label and field, and calls
+// WatchPredicate. If possible, you should customize PredicateFunc to produre a
+// matcher that matches by key.
 func (e *Etcd) Watch(ctx api.Context, label labels.Selector, field fields.Selector, resourceVersion string) (watch.Interface, error) {
-	if value, found := field.RequiresExactMatch(e.WatchSingleFieldName); found && len(e.WatchSingleFieldName) > 0 {
+	return e.WatchPredicate(ctx, e.PredicateFunc(label, field), resourceVersion)
-		key, err := e.KeyFunc(ctx, value)
-		if err != nil {
-			return nil, err
-		}
-		return e.watchPredicate(key, e.PredicateFunc(label, field), resourceVersion)
-	}
-	return e.watchPredicate(e.KeyRootFunc(ctx), e.PredicateFunc(label, field), resourceVersion)
 }
 
 // WatchPredicate starts a watch for the items that m matches.
-// TODO: Detect if m references a single object instead of a list.
 func (e *Etcd) WatchPredicate(ctx api.Context, m generic.Matcher, resourceVersion string) (watch.Interface, error) {
-	return e.watchPredicate(e.KeyRootFunc(ctx), m, resourceVersion)
-}
-
-func (e *Etcd) watchPredicate(key string, m generic.Matcher, resourceVersion string) (watch.Interface, error) {
 	version, err := tools.ParseWatchResourceVersion(resourceVersion, e.EndpointName)
 	if err != nil {
 		return nil, err
 	}
-	return e.Helper.WatchList(key, version, func(obj runtime.Object) bool {
+
+	var watchKey string
+	if name, ok := m.MatchesSingle(); ok {
+		key, err := e.KeyFunc(ctx, name)
+		if err != nil {
+			return nil, err
+		}
+		watchKey = key
+	} else {
+		watchKey = e.KeyRootFunc(ctx)
+	}
+
+	return e.Helper.WatchList(watchKey, version, func(obj runtime.Object) bool {
 		matches, err := m.Matches(obj)
 		if err != nil {
 			glog.Errorf("unable to match watch: %v", err)

pkg/registry/generic/etcd/etcd_test.go

@@ -83,13 +83,13 @@ func NewTestGenericEtcdRegistry(t *testing.T) (*tools.FakeEtcdClient, *Etcd) {
 	}
 }
 
-// SetMatcher is a matcher that matches any pod with id in the set.
+// setMatcher is a matcher that matches any pod with id in the set.
 // Makes testing simpler.
-type SetMatcher struct {
+type setMatcher struct {
 	util.StringSet
 }
 
-func (sm SetMatcher) Matches(obj runtime.Object) (bool, error) {
+func (sm setMatcher) Matches(obj runtime.Object) (bool, error) {
 	pod, ok := obj.(*api.Pod)
 	if !ok {
 		return false, fmt.Errorf("wrong object")
@@ -97,13 +97,25 @@ func (sm SetMatcher) Matches(obj runtime.Object) (bool, error) {
 	return sm.Has(pod.Name), nil
 }
 
-// EverythingMatcher matches everything
+func (sm setMatcher) MatchesSingle() (string, bool) {
-type EverythingMatcher struct{}
+	if sm.Len() == 1 {
+		// Since pod name is its key, we can optimize this case.
+		return sm.List()[0], true
+	}
+	return "", false
+}
 
-func (EverythingMatcher) Matches(obj runtime.Object) (bool, error) {
+// everythingMatcher matches everything
+type everythingMatcher struct{}
+
+func (everythingMatcher) Matches(obj runtime.Object) (bool, error) {
 	return true, nil
 }
 
+func (everythingMatcher) MatchesSingle() (string, bool) {
+	return "", false
+}
+
 func TestEtcdList(t *testing.T) {
 	podA := &api.Pod{
 		ObjectMeta: api.ObjectMeta{Name: "foo"},
@@ -138,7 +150,7 @@ func TestEtcdList(t *testing.T) {
 				},
 				E: nil,
 			},
-			m:       EverythingMatcher{},
+			m:       everythingMatcher{},
 			out:     &api.PodList{Items: []api.Pod{}},
 			succeed: true,
 		},
@@ -147,7 +159,7 @@ func TestEtcdList(t *testing.T) {
 				R: &etcd.Response{},
 				E: tools.EtcdErrorNotFound,
 			},
-			m:       EverythingMatcher{},
+			m:       everythingMatcher{},
 			out:     &api.PodList{Items: []api.Pod{}},
 			succeed: true,
 		},
@@ -156,7 +168,7 @@ func TestEtcdList(t *testing.T) {
 				R: normalListResp,
 				E: nil,
 			},
-			m:       EverythingMatcher{},
+			m:       everythingMatcher{},
 			out:     &api.PodList{Items: []api.Pod{*podA, *podB}},
 			succeed: true,
 		},
@@ -165,7 +177,16 @@ func TestEtcdList(t *testing.T) {
 				R: normalListResp,
 				E: nil,
 			},
-			m:       SetMatcher{util.NewStringSet("foo")},
+			m:       setMatcher{util.NewStringSet("foo")},
+			out:     &api.PodList{Items: []api.Pod{*podA}},
+			succeed: true,
+		},
+		"normalFilteredMatchMultiple": {
+			in: tools.EtcdResponseWithError{
+				R: normalListResp,
+				E: nil,
+			},
+			m:       setMatcher{util.NewStringSet("foo", "makeMatchSingleReturnFalse")},
 			out:     &api.PodList{Items: []api.Pod{*podA}},
 			succeed: true,
 		},
@@ -648,36 +669,45 @@ func TestEtcdDelete(t *testing.T) {
 }
 
 func TestEtcdWatch(t *testing.T) {
-	podA := &api.Pod{
+	table := map[string]generic.Matcher{
-		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
+		"single": setMatcher{util.NewStringSet("foo")},
-		Spec:       api.PodSpec{Host: "machine"},
+		"multi":  setMatcher{util.NewStringSet("foo", "bar")},
-	}
-	respWithPodA := &etcd.Response{
-		Node: &etcd.Node{
-			Value:         runtime.EncodeOrDie(testapi.Codec(), podA),
-			ModifiedIndex: 1,
-			CreatedIndex:  1,
-		},
-		Action: "create",
 	}
 
-	fakeClient, registry := NewTestGenericEtcdRegistry(t)
+	for name, m := range table {
-	wi, err := registry.WatchPredicate(api.NewContext(), EverythingMatcher{}, "1")
+		podA := &api.Pod{
-	if err != nil {
+			ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"},
-		t.Fatalf("unexpected error: %v", err)
+			Spec:       api.PodSpec{Host: "machine"},
-	}
+		}
-	fakeClient.WaitForWatchCompletion()
+		respWithPodA := &etcd.Response{
+			Node: &etcd.Node{
+				Value:         runtime.EncodeOrDie(testapi.Codec(), podA),
+				ModifiedIndex: 1,
+				CreatedIndex:  1,
+			},
+			Action: "create",
+		}
 
-	go func() {
+		fakeClient, registry := NewTestGenericEtcdRegistry(t)
-		fakeClient.WatchResponse <- respWithPodA
+		wi, err := registry.WatchPredicate(api.NewContext(), m, "1")
-	}()
+		if err != nil {
+			t.Errorf("%v: unexpected error: %v", name, err)
+			continue
+		}
+		fakeClient.WaitForWatchCompletion()
 
-	got, open := <-wi.ResultChan()
+		go func() {
-	if !open {
+			fakeClient.WatchResponse <- respWithPodA
-		t.Fatalf("unexpected channel close")
+		}()
-	}
 
-	if e, a := podA, got.Object; !api.Semantic.DeepDerivative(e, a) {
+		got, open := <-wi.ResultChan()
-		t.Errorf("difference: %s", util.ObjectDiff(e, a))
+		if !open {
+			t.Errorf("%v: unexpected channel close", name)
+			continue
+		}
+
+		if e, a := podA, got.Object; !api.Semantic.DeepDerivative(e, a) {
+			t.Errorf("%v: difference: %s", name, util.ObjectDiff(e, a))
+		}
 	}
 }

pkg/registry/generic/registry.go

@@ -49,14 +49,37 @@ func (s *SelectionPredicate) Matches(obj runtime.Object) (bool, error) {
 	return s.Label.Matches(labels) && s.Field.Matches(fields), nil
 }
 
+// MatchesSingle will return (name, true) iff s.Field matches on the object's
+// name.
+func (s *SelectionPredicate) MatchesSingle() (string, bool) {
+	// TODO: should be namespace.name
+	if name, ok := s.Field.RequiresExactMatch("name"); ok {
+		return name, true
+	}
+	return "", false
+}
+
 // Matcher can return true if an object matches the Matcher's selection
-// criteria.
+// criteria. If it is known that the matcher will match only a single object
+// then MatchesSingle should return the key of that object and true. This is an
+// optimization only--Matches() should continue to work.
 type Matcher interface {
-	Matches(obj runtime.Object) (bool, error)
+	// Matches should return true if obj matches this matcher's requirements.
+	Matches(obj runtime.Object) (matchesThisObject bool, err error)
+
+	// If this matcher matches a single object, return the key for that
+	// object and true here. This will greatly increase efficiency. You
+	// must still implement Matches(). Note that key does NOT need to
+	// include the object's namespace.
+	MatchesSingle() (key string, matchesSingleObject bool)
+
+	// TODO: when we start indexing objects, add something like the below:
+	//         MatchesIndices() (indexName []string, indexValue []string)
+	//       where indexName/indexValue are the same length.
 }
 
 // MatcherFunc makes a matcher from the provided function. For easy definition
-// of matchers for testing.
+// of matchers for testing. Note: use SelectionPredicate above for real code!
 func MatcherFunc(f func(obj runtime.Object) (bool, error)) Matcher {
 	return matcherFunc(f)
 }
@@ -68,6 +91,36 @@ func (m matcherFunc) Matches(obj runtime.Object) (bool, error) {
 	return m(obj)
 }
 
+// MatchesSingle always returns "", false-- because this is a predicate
+// implementation of Matcher.
+func (m matcherFunc) MatchesSingle() (string, bool) {
+	return "", false
+}
+
+// MatchOnKey returns a matcher that will send only the object matching key
+// through the matching function f. For testing!
+// Note: use SelectionPredicate above for real code!
+func MatchOnKey(key string, f func(obj runtime.Object) (bool, error)) Matcher {
+	return matchKey{key, f}
+}
+
+type matchKey struct {
+	key string
+	matcherFunc
+}
+
+// MatchesSingle always returns its key, true.
+func (m matchKey) MatchesSingle() (string, bool) {
+	return m.key, true
+}
+
+var (
+	// Assert implementations match the interface.
+	_ = Matcher(matchKey{})
+	_ = Matcher(&SelectionPredicate{})
+	_ = Matcher(matcherFunc(nil))
+)
+
 // DecoratorFunc can mutate the provided object prior to being returned.
 type DecoratorFunc func(obj runtime.Object) error
 

pkg/registry/generic/registry_test.go

@@ -44,6 +44,7 @@ func TestSelectionPredicate(t *testing.T) {
 		fields                       fields.Set
 		err                          error
 		shouldMatch                  bool
+		matchSingleKey               string
 	}{
 		"A": {
 			labelSelector: "name=foo",
@@ -66,6 +67,13 @@ func TestSelectionPredicate(t *testing.T) {
 			fields:        fields.Set{"uid": "12345"},
 			shouldMatch:   false,
 		},
+		"D": {
+			fieldSelector:  "name=12345",
+			labels:         labels.Set{},
+			fields:         fields.Set{"name": "12345"},
+			shouldMatch:    true,
+			matchSingleKey: "12345",
+		},
 		"error": {
 			labelSelector: "name=foo",
 			fieldSelector: "uid=12345",
@@ -98,6 +106,15 @@ func TestSelectionPredicate(t *testing.T) {
 		if e, a := item.shouldMatch, got; e != a {
 			t.Errorf("%v: expected %v, got %v", name, e, a)
 		}
+		if key := item.matchSingleKey; key != "" {
+			got, ok := sp.MatchesSingle()
+			if !ok {
+				t.Errorf("%v: expected single match", name)
+			}
+			if e, a := key, got; e != a {
+				t.Errorf("%v: expected %v, got %v", name, e, a)
+			}
+		}
 	}
 }
 
@@ -118,16 +135,18 @@ func TestFilterList(t *testing.T) {
 		},
 	}
 
-	got, err := FilterList(try,
+	m := MatcherFunc(func(obj runtime.Object) (bool, error) {
-		MatcherFunc(func(obj runtime.Object) (bool, error) {
+		i, ok := obj.(*Ignored)
-			i, ok := obj.(*Ignored)
+		if !ok {
-			if !ok {
+			return false, errors.New("wrong type")
-				return false, errors.New("wrong type")
+		}
-			}
+		return i.ID[0] == 'b', nil
-			return i.ID[0] == 'b', nil
+	})
-		}),
+	if _, matchesSingleObject := m.MatchesSingle(); matchesSingleObject {
-		nil,
+		t.Errorf("matcher unexpectedly matches only a single object.")
-	)
+	}
+
+	got, err := FilterList(try, m, nil)
 	if err != nil {
 		t.Fatalf("Unexpected error %v", err)
 	}
@@ -136,3 +155,14 @@ func TestFilterList(t *testing.T) {
 		t.Errorf("Expected %#v, got %#v", e, a)
 	}
 }
+
+func TestSingleMatch(t *testing.T) {
+	m := MatchOnKey("pod-name-here", func(obj runtime.Object) (bool, error) { return true, nil })
+	got, ok := m.MatchesSingle()
+	if !ok {
+		t.Errorf("Expected MatchesSingle to return true")
+	}
+	if e, a := "pod-name-here", got; e != a {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+}

pkg/registry/minion/etcd/etcd.go

@@ -23,9 +23,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/rest"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic"
 	etcdgeneric "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic/etcd"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/registry/minion"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/runtime"
@@ -49,14 +46,11 @@ func NewStorage(h tools.EtcdHelper, connection client.ConnectionInfoGetter) *RES
 		KeyFunc: func(ctx api.Context, name string) (string, error) {
 			return prefix + "/" + name, nil
 		},
-		WatchSingleFieldName: "name",
 		ObjectNameFunc: func(obj runtime.Object) (string, error) {
 			return obj.(*api.Node).Name, nil
 		},
-		PredicateFunc: func(label labels.Selector, field fields.Selector) generic.Matcher {
+		PredicateFunc: minion.MatchNode,
-			return minion.MatchNode(label, field)
+		EndpointName:  "minion",
-		},
-		EndpointName: "minion",
 
 		CreateStrategy: minion.Strategy,
 		UpdateStrategy: minion.Strategy,

pkg/registry/minion/etcd/etcd_test.go

@@ -347,59 +347,6 @@ func TestEtcdWatchNodesMatch(t *testing.T) {
 	watching.Stop()
 }
 
-func TestEtcdWatchNodesFields(t *testing.T) {
-	ctx := api.NewDefaultContext()
-	storage, fakeClient := newStorage(t)
-	node := validNewNode()
-	nodeBytes, _ := latest.Codec.Encode(node)
-
-	testFieldMap := map[int][]fields.Set{
-		PASS: {
-			{"name": "foo"},
-		},
-		FAIL: {
-			{"name": "bar"},
-		},
-	}
-
-	for _, singleWatchField := range []string{"", "name"} {
-		storage.WatchSingleFieldName = singleWatchField
-		for expectedResult, fieldSet := range testFieldMap {
-			for _, field := range fieldSet {
-				watching, err := storage.Watch(ctx,
-					labels.Everything(),
-					field.AsSelector(),
-					"1",
-				)
-				if err != nil {
-					t.Fatalf("unexpected error: %v", err)
-				}
-				fakeClient.WaitForWatchCompletion()
-				fakeClient.WatchResponse <- &etcd.Response{
-					Action: "create",
-					Node: &etcd.Node{
-						Value: string(nodeBytes),
-					},
-				}
-				select {
-				case r, ok := <-watching.ResultChan():
-					if expectedResult == FAIL {
-						t.Errorf("unexpected result from channel %#v", r)
-					}
-					if !ok {
-						t.Errorf("watching channel should be open")
-					}
-				case <-time.After(time.Millisecond * 100):
-					if expectedResult == PASS {
-						t.Errorf("unexpected timeout from result channel")
-					}
-				}
-				watching.Stop()
-			}
-		}
-	}
-}
-
 func TestEtcdWatchNodesNotMatch(t *testing.T) {
 	ctx := api.NewDefaultContext()
 	storage, fakeClient := newStorage(t)

pkg/registry/minion/rest.go

@@ -83,22 +83,25 @@ type ResourceGetter interface {
 }
 
 // NodeToSelectableFields returns a label set that represents the object.
-func NodeToSelectableFields(node *api.Node) labels.Set {
+func NodeToSelectableFields(node *api.Node) fields.Set {
-	return labels.Set{
+	return fields.Set{
 		"name": node.Name,
 	}
 }
 
 // MatchNode returns a generic matcher for a given label and field selector.
 func MatchNode(label labels.Selector, field fields.Selector) generic.Matcher {
-	return generic.MatcherFunc(func(obj runtime.Object) (bool, error) {
+	return &generic.SelectionPredicate{
-		nodeObj, ok := obj.(*api.Node)
+		Label: label,
-		if !ok {
+		Field: field,
-			return false, fmt.Errorf("not a node")
+		GetAttrs: func(obj runtime.Object) (labels.Set, fields.Set, error) {
-		}
+			nodeObj, ok := obj.(*api.Node)
-		fields := NodeToSelectableFields(nodeObj)
+			if !ok {
-		return label.Matches(labels.Set(nodeObj.Labels)) && field.Matches(fields), nil
+				return nil, nil, fmt.Errorf("not a node")
-	})
+			}
+			return labels.Set(nodeObj.ObjectMeta.Labels), NodeToSelectableFields(nodeObj), nil
+		},
+	}
 }
 
 // ResourceLocation returns a URL to which one can send traffic for the specified node.

pkg/registry/minion/rest_test.go

@@ -0,0 +1,45 @@
+/*
+Copyright 2015 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package minion
+
+import (
+	"testing"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/fields"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/labels"
+)
+
+func TestMatchNode(t *testing.T) {
+	testFieldMap := map[bool][]fields.Set{
+		true: {
+			{"name": "foo"},
+		},
+		false: {
+			{"foo": "bar"},
+		},
+	}
+
+	for expectedResult, fieldSet := range testFieldMap {
+		for _, field := range fieldSet {
+			m := MatchNode(labels.Everything(), field.AsSelector())
+			_, matchesSingle := m.MatchesSingle()
+			if e, a := expectedResult, matchesSingle; e != a {
+				t.Errorf("%+v: expected %v, got %v", e, a)
+			}
+		}
+	}
+}