From 0c2d3ffe689b33a1974a5b94e85f55963223e0e6 Mon Sep 17 00:00:00 2001 From: Daniel Smith Date: Thu, 2 Apr 2015 11:11:00 -0700 Subject: [PATCH] Single-key matching behavior in generic.Matcher --- pkg/client/cache/store.go | 3 +- pkg/registry/generic/etcd/etcd.go | 39 +++++----- pkg/registry/generic/etcd/etcd_test.go | 102 ++++++++++++++++--------- pkg/registry/generic/registry.go | 59 +++++++++++++- pkg/registry/generic/registry_test.go | 50 +++++++++--- pkg/registry/minion/etcd/etcd.go | 10 +-- pkg/registry/minion/etcd/etcd_test.go | 53 ------------- pkg/registry/minion/rest.go | 23 +++--- pkg/registry/minion/rest_test.go | 45 +++++++++++ 9 files changed, 243 insertions(+), 141 deletions(-) create mode 100644 pkg/registry/minion/rest_test.go diff --git a/pkg/client/cache/store.go b/pkg/client/cache/store.go index 47d70971c0b..8f647cb3d7e 100644 --- a/pkg/client/cache/store.go +++ b/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: / +// The key uses the format / unless is empty, then +// it's just . func MetaNamespaceKeyFunc(obj interface{}) (string, error) { meta, err := meta.Accessor(obj) if err != nil { diff --git a/pkg/registry/generic/etcd/etcd.go b/pkg/registry/generic/etcd/etcd.go index 463b5acd2e0..736bdeed891 100644 --- a/pkg/registry/generic/etcd/etcd.go +++ b/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 { - 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) + return e.WatchPredicate(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) diff --git a/pkg/registry/generic/etcd/etcd_test.go b/pkg/registry/generic/etcd/etcd_test.go index 34a8ab5cb61..75f14a7515b 100644 --- a/pkg/registry/generic/etcd/etcd_test.go +++ b/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 -type EverythingMatcher struct{} +func (sm setMatcher) MatchesSingle() (string, bool) { + 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{ - ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"}, - Spec: api.PodSpec{Host: "machine"}, - } - respWithPodA := &etcd.Response{ - Node: &etcd.Node{ - Value: runtime.EncodeOrDie(testapi.Codec(), podA), - ModifiedIndex: 1, - CreatedIndex: 1, - }, - Action: "create", + table := map[string]generic.Matcher{ + "single": setMatcher{util.NewStringSet("foo")}, + "multi": setMatcher{util.NewStringSet("foo", "bar")}, } - fakeClient, registry := NewTestGenericEtcdRegistry(t) - wi, err := registry.WatchPredicate(api.NewContext(), EverythingMatcher{}, "1") - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - fakeClient.WaitForWatchCompletion() + for name, m := range table { + podA := &api.Pod{ + ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "1"}, + Spec: api.PodSpec{Host: "machine"}, + } + respWithPodA := &etcd.Response{ + Node: &etcd.Node{ + Value: runtime.EncodeOrDie(testapi.Codec(), podA), + ModifiedIndex: 1, + CreatedIndex: 1, + }, + Action: "create", + } - go func() { - fakeClient.WatchResponse <- respWithPodA - }() + fakeClient, registry := NewTestGenericEtcdRegistry(t) + 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() - if !open { - t.Fatalf("unexpected channel close") - } + go func() { + fakeClient.WatchResponse <- respWithPodA + }() - if e, a := podA, got.Object; !api.Semantic.DeepDerivative(e, a) { - t.Errorf("difference: %s", util.ObjectDiff(e, a)) + got, open := <-wi.ResultChan() + 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)) + } } } diff --git a/pkg/registry/generic/registry.go b/pkg/registry/generic/registry.go index 66de3060afc..f271806aa72 100644 --- a/pkg/registry/generic/registry.go +++ b/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 diff --git a/pkg/registry/generic/registry_test.go b/pkg/registry/generic/registry_test.go index dc1423b0f77..fd4aa81c85a 100644 --- a/pkg/registry/generic/registry_test.go +++ b/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, - MatcherFunc(func(obj runtime.Object) (bool, error) { - i, ok := obj.(*Ignored) - if !ok { - return false, errors.New("wrong type") - } - return i.ID[0] == 'b', nil - }), - nil, - ) + m := MatcherFunc(func(obj runtime.Object) (bool, error) { + i, ok := obj.(*Ignored) + if !ok { + return false, errors.New("wrong type") + } + return i.ID[0] == 'b', nil + }) + if _, matchesSingleObject := m.MatchesSingle(); matchesSingleObject { + 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) + } +} diff --git a/pkg/registry/minion/etcd/etcd.go b/pkg/registry/minion/etcd/etcd.go index f90510cb4b5..0c22589d933 100644 --- a/pkg/registry/minion/etcd/etcd.go +++ b/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 { - return minion.MatchNode(label, field) - }, - EndpointName: "minion", + PredicateFunc: minion.MatchNode, + EndpointName: "minion", CreateStrategy: minion.Strategy, UpdateStrategy: minion.Strategy, diff --git a/pkg/registry/minion/etcd/etcd_test.go b/pkg/registry/minion/etcd/etcd_test.go index 8f23d87a4e4..78248a28367 100644 --- a/pkg/registry/minion/etcd/etcd_test.go +++ b/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) diff --git a/pkg/registry/minion/rest.go b/pkg/registry/minion/rest.go index 0590e0a2bcd..78dc1a9076c 100644 --- a/pkg/registry/minion/rest.go +++ b/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 { - return labels.Set{ +func NodeToSelectableFields(node *api.Node) fields.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) { - nodeObj, ok := obj.(*api.Node) - if !ok { - return false, fmt.Errorf("not a node") - } - fields := NodeToSelectableFields(nodeObj) - return label.Matches(labels.Set(nodeObj.Labels)) && field.Matches(fields), nil - }) + return &generic.SelectionPredicate{ + Label: label, + Field: field, + GetAttrs: func(obj runtime.Object) (labels.Set, fields.Set, error) { + nodeObj, ok := obj.(*api.Node) + if !ok { + 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. diff --git a/pkg/registry/minion/rest_test.go b/pkg/registry/minion/rest_test.go new file mode 100644 index 00000000000..0fbae857c04 --- /dev/null +++ b/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) + } + } + } +}