Merge pull request #5473 from lavalamp/fix6

pkg/client/cache/delta_fifo_test.go

+/*
+Copyright 2014 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 cache
+
+import (
+	"reflect"
+	"testing"
+	"time"
+)
+
+// helper function to reduce stuttering
+func testPop(f *DeltaFIFO) testFifoObject {
+	return f.Pop().(Deltas).Newest().Object.(testFifoObject)
+}
+
+func TestDeltaFIFO_basic(t *testing.T) {
+	f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)
+	const amount = 500
+	go func() {
+		for i := 0; i < amount; i++ {
+			f.Add(mkFifoObj(string([]rune{'a', rune(i)}), i+1))
+		}
+	}()
+	go func() {
+		for u := uint64(0); u < amount; u++ {
+			f.Add(mkFifoObj(string([]rune{'b', rune(u)}), u+1))
+		}
+	}()
+
+	lastInt := int(0)
+	lastUint := uint64(0)
+	for i := 0; i < amount*2; i++ {
+		switch obj := testPop(f).val.(type) {
+		case int:
+			if obj <= lastInt {
+				t.Errorf("got %v (int) out of order, last was %v", obj, lastInt)
+			}
+			lastInt = obj
+		case uint64:
+			if obj <= lastUint {
+				t.Errorf("got %v (uint) out of order, last was %v", obj, lastUint)
+			} else {
+				lastUint = obj
+			}
+		default:
+			t.Fatalf("unexpected type %#v", obj)
+		}
+	}
+}
+
+func TestDeltaFIFO_compressorWorks(t *testing.T) {
+	oldestTypes := []DeltaType{}
+	f := NewDeltaFIFO(
+		testFifoObjectKeyFunc,
+		// This function just keeps the most recent delta
+		// and puts deleted ones in the list.
+		DeltaCompressorFunc(func(d Deltas) Deltas {
+			if n := len(d); n > 1 {
+				oldestTypes = append(oldestTypes, d[0].Type)
+				d = d[1:]
+			}
+			return d
+		}),
+		nil,
+	)
+	f.Add(mkFifoObj("foo", 10))
+	f.Update(mkFifoObj("foo", 12))
+	f.Replace([]interface{}{mkFifoObj("foo", 20)})
+	f.Delete(mkFifoObj("foo", 15))
+	f.Delete(mkFifoObj("foo", 18)) // flush the last one out
+	expect := []DeltaType{Added, Updated, Sync, Deleted}
+	if e, a := expect, oldestTypes; !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %#v, got %#v", e, a)
+	}
+	if e, a := (Deltas{{Deleted, mkFifoObj("foo", 18)}}), f.Pop().(Deltas); !reflect.DeepEqual(e, a) {
+		t.Fatalf("Expected %#v, got %#v", e, a)
+	}
+
+}
+
+func TestDeltaFIFO_addUpdate(t *testing.T) {
+	f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)
+	f.Add(mkFifoObj("foo", 10))
+	f.Update(mkFifoObj("foo", 12))
+	f.Delete(mkFifoObj("foo", 15))
+
+	if e, a := []interface{}{mkFifoObj("foo", 15)}, f.List(); !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %+v, got %+v", e, a)
+	}
+	if e, a := []string{"foo"}, f.ListKeys(); !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %+v, got %+v", e, a)
+	}
+
+	got := make(chan testFifoObject, 2)
+	go func() {
+		for {
+			got <- testPop(f)
+		}
+	}()
+
+	first := <-got
+	if e, a := 15, first.val; e != a {
+		t.Errorf("Didn't get updated value (%v), got %v", e, a)
+	}
+	select {
+	case unexpected := <-got:
+		t.Errorf("Got second value %v", unexpected.val)
+	case <-time.After(50 * time.Millisecond):
+	}
+	_, exists, _ := f.Get(mkFifoObj("foo", ""))
+	if exists {
+		t.Errorf("item did not get removed")
+	}
+}
+
+func TestDeltaFIFO_enqueueing(t *testing.T) {
+	f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)
+	f.Add(mkFifoObj("foo", 10))
+	f.Update(mkFifoObj("bar", 15))
+	f.Delete(mkFifoObj("baz", 20))
+
+	expectList := []int{10, 15, 20}
+	for _, expect := range expectList {
+		if e, a := expect, testPop(f).val; e != a {
+			t.Errorf("Didn't get updated value (%v), got %v", e, a)
+		}
+	}
+	if e, a := 0, len(f.items); e != a {
+		t.Errorf("queue unexpectedly not empty: %v != %v", e, a)
+	}
+}
+
+func TestDeltaFIFO_addReplace(t *testing.T) {
+	f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)
+	f.Add(mkFifoObj("foo", 10))
+	f.Replace([]interface{}{mkFifoObj("foo", 15)})
+	got := make(chan testFifoObject, 2)
+	go func() {
+		for {
+			got <- testPop(f)
+		}
+	}()
+
+	first := <-got
+	if e, a := 15, first.val; e != a {
+		t.Errorf("Didn't get updated value (%v), got %v", e, a)
+	}
+	select {
+	case unexpected := <-got:
+		t.Errorf("Got second value %v", unexpected.val)
+	case <-time.After(50 * time.Millisecond):
+	}
+	_, exists, _ := f.Get(mkFifoObj("foo", ""))
+	if exists {
+		t.Errorf("item did not get removed")
+	}
+}
+
+func TestDeltaFIFO_ReplaceMakesDeletions(t *testing.T) {
+	f := NewDeltaFIFO(
+		testFifoObjectKeyFunc,
+		nil,
+		KeyListerFunc(func() []string {
+			return []string{"foo", "bar", "baz"}
+		}),
+	)
+	f.Delete(mkFifoObj("baz", 10))
+	f.Replace([]interface{}{mkFifoObj("foo", 5)})
+
+	expectedList := []Deltas{
+		{{Deleted, mkFifoObj("baz", 10)}},
+		{{Sync, mkFifoObj("foo", 5)}},
+		{{Deleted, DeletedFinalStateUnknown{Key: "bar"}}},
+	}
+
+	for _, expected := range expectedList {
+		cur := f.Pop().(Deltas)
+		if e, a := expected, cur; !reflect.DeepEqual(e, a) {
+			t.Errorf("Expected %#v, got %#v", e, a)
+		}
+	}
+}
+
+func TestDeltaFIFO_detectLineJumpers(t *testing.T) {
+	f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)
+
+	f.Add(mkFifoObj("foo", 10))
+	f.Add(mkFifoObj("bar", 1))
+	f.Add(mkFifoObj("foo", 11))
+	f.Add(mkFifoObj("foo", 13))
+	f.Add(mkFifoObj("zab", 30))
+
+	if e, a := 13, testPop(f).val; a != e {
+		t.Fatalf("expected %d, got %d", e, a)
+	}
+
+	f.Add(mkFifoObj("foo", 14)) // ensure foo doesn't jump back in line
+
+	if e, a := 1, testPop(f).val; a != e {
+		t.Fatalf("expected %d, got %d", e, a)
+	}
+
+	if e, a := 30, testPop(f).val; a != e {
+		t.Fatalf("expected %d, got %d", e, a)
+	}
+
+	if e, a := 14, testPop(f).val; a != e {
+		t.Fatalf("expected %d, got %d", e, a)
+	}
+}
+
+func TestDeltaFIFO_addIfNotPresent(t *testing.T) {
+	f := NewDeltaFIFO(testFifoObjectKeyFunc, nil, nil)
+
+	f.Add(mkFifoObj("b", 3))
+	b3 := f.Pop()
+	f.Add(mkFifoObj("c", 4))
+	c4 := f.Pop()
+	if e, a := 0, len(f.items); e != a {
+		t.Fatalf("Expected %v, got %v items in queue", e, a)
+	}
+
+	f.Add(mkFifoObj("a", 1))
+	f.Add(mkFifoObj("b", 2))
+	f.AddIfNotPresent(b3)
+	f.AddIfNotPresent(c4)
+
+	if e, a := 3, len(f.items); a != e {
+		t.Fatalf("expected queue length %d, got %d", e, a)
+	}
+
+	expectedValues := []int{1, 2, 4}
+	for _, expected := range expectedValues {
+		if actual := testPop(f).val; actual != expected {
+			t.Fatalf("expected value %d, got %d", expected, actual)
+		}
+	}
+}
+
+func TestDeltaFIFO_KeyOf(t *testing.T) {
+	f := DeltaFIFO{keyFunc: testFifoObjectKeyFunc}
+
+	table := []struct {
+		obj interface{}
+		key string
+	}{
+		{obj: testFifoObject{name: "A"}, key: "A"},
+		{obj: DeletedFinalStateUnknown{Key: "B"}, key: "B"},
+		{obj: Deltas{{Object: testFifoObject{name: "C"}}}, key: "C"},
+		{obj: Deltas{{Object: DeletedFinalStateUnknown{Key: "D"}}}, key: "D"},
+	}
+
+	for _, item := range table {
+		got, err := f.KeyOf(item.obj)
+		if err != nil {
+			t.Errorf("Unexpected error for %q: %v", item.obj, err)
+			continue
+		}
+		if e, a := item.key, got; e != a {
+			t.Errorf("Expected %v, got %v", e, a)
+		}
+	}
+}

pkg/client/cache/fifo.go

@@ -17,15 +17,35 @@ limitations under the License.
 package cache
 
 import (
-	"fmt"
 	"sync"
 )
 
+// Queue is exactly like a Store, but has a Pop() method too.
+type Queue interface {
+	Store
+
+	// Pop blocks until it has something to return.
+	Pop() interface{}
+
+	// AddIfNotPresent adds a value previously
+	// returned by Pop back into the queue as long
+	// as nothing else (presumably more recent)
+	// has since been added.
+	AddIfNotPresent(interface{}) error
+}
+
 // FIFO receives adds and updates from a Reflector, and puts them in a queue for
 // FIFO order processing. If multiple adds/updates of a single item happen while
 // an item is in the queue before it has been processed, it will only be
 // processed once, and when it is processed, the most recent version will be
 // processed. This can't be done with a channel.
+//
+// FIFO solves this use case:
+//  * You want to process every object (exactly) once.
+//  * You want to process the most recent version of the object when you process it.
+//  * You do not want to process deleted objects, they should be removed from the queue.
+//  * You do not want to periodically reprocess objects.
+// Compare with DeltaFIFO for other use cases.
 type FIFO struct {
 	lock sync.RWMutex
 	cond sync.Cond
@@ -37,12 +57,16 @@ type FIFO struct {
 	keyFunc KeyFunc
 }
 
+var (
+	_ = Queue(&FIFO{}) // FIFO is a Queue
+)
+
 // Add inserts an item, and puts it in the queue. The item is only enqueued
 // if it doesn't already exist in the set.
 func (f *FIFO) Add(obj interface{}) error {
 	id, err := f.keyFunc(obj)
 	if err != nil {
-		return fmt.Errorf("couldn't create key for object: %v", err)
+		return KeyError{obj, err}
 	}
 	f.lock.Lock()
 	defer f.lock.Unlock()
@@ -63,7 +87,7 @@ func (f *FIFO) Add(obj interface{}) error {
 func (f *FIFO) AddIfNotPresent(obj interface{}) error {
 	id, err := f.keyFunc(obj)
 	if err != nil {
-		return fmt.Errorf("couldn't create key for object: %v", err)
+		return KeyError{obj, err}
 	}
 	f.lock.Lock()
 	defer f.lock.Unlock()
@@ -88,7 +112,7 @@ func (f *FIFO) Update(obj interface{}) error {
 func (f *FIFO) Delete(obj interface{}) error {
 	id, err := f.keyFunc(obj)
 	if err != nil {
-		return fmt.Errorf("couldn't create key for object: %v", err)
+		return KeyError{obj, err}
 	}
 	f.lock.Lock()
 	defer f.lock.Unlock()
@@ -107,11 +131,23 @@ func (f *FIFO) List() []interface{} {
 	return list
 }
 
+// ListKeys returns a list of all the keys of the objects currently
+// in the FIFO.
+func (f *FIFO) ListKeys() []string {
+	f.lock.RLock()
+	defer f.lock.RUnlock()
+	list := make([]string, 0, len(f.items))
+	for key := range f.items {
+		list = append(list, key)
+	}
+	return list
+}
+
 // Get returns the requested item, or sets exists=false.
 func (f *FIFO) Get(obj interface{}) (item interface{}, exists bool, err error) {
 	key, err := f.keyFunc(obj)
 	if err != nil {
-		return nil, false, fmt.Errorf("couldn't create key for object: %v", err)
+		return nil, false, KeyError{obj, err}
 	}
 	return f.GetByKey(key)
 }
@@ -127,7 +163,8 @@ func (f *FIFO) GetByKey(key string) (item interface{}, exists bool, err error) {
 // Pop waits until an item is ready and returns it. If multiple items are
 // ready, they are returned in the order in which they were added/updated.
 // The item is removed from the queue (and the store) before it is returned,
-// so if you don't succesfully process it, you need to add it back with Add().
+// so if you don't succesfully process it, you need to add it back with
+// AddIfNotPresent().
 func (f *FIFO) Pop() interface{} {
 	f.lock.Lock()
 	defer f.lock.Unlock()
@@ -156,7 +193,7 @@ func (f *FIFO) Replace(list []interface{}) error {
 	for _, item := range list {
 		key, err := f.keyFunc(item)
 		if err != nil {
-			return fmt.Errorf("couldn't create key for object: %v", err)
+			return KeyError{item, err}
 		}
 		items[key] = item
 	}

pkg/client/cache/fifo_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cache
 
 import (
+	"reflect"
 	"testing"
 	"time"
 )
@@ -30,21 +31,21 @@ type testFifoObject struct {
 	val  interface{}
 }
 
-func TestFIFO_basic(t *testing.T) {
-	mkObj := func(name string, val interface{}) testFifoObject {
-		return testFifoObject{name: name, val: val}
-	}
+func mkFifoObj(name string, val interface{}) testFifoObject {
+	return testFifoObject{name: name, val: val}
+}
 
+func TestFIFO_basic(t *testing.T) {
 	f := NewFIFO(testFifoObjectKeyFunc)
 	const amount = 500
 	go func() {
 		for i := 0; i < amount; i++ {
-			f.Add(mkObj(string([]rune{'a', rune(i)}), i+1))
+			f.Add(mkFifoObj(string([]rune{'a', rune(i)}), i+1))
 		}
 	}()
 	go func() {
 		for u := uint64(0); u < amount; u++ {
-			f.Add(mkObj(string([]rune{'b', rune(u)}), u+1))
+			f.Add(mkFifoObj(string([]rune{'b', rune(u)}), u+1))
 		}
 	}()
 
@@ -70,13 +71,17 @@ func TestFIFO_basic(t *testing.T) {
 }
 
 func TestFIFO_addUpdate(t *testing.T) {
-	mkObj := func(name string, val interface{}) testFifoObject {
-		return testFifoObject{name: name, val: val}
+	f := NewFIFO(testFifoObjectKeyFunc)
+	f.Add(mkFifoObj("foo", 10))
+	f.Update(mkFifoObj("foo", 15))
+
+	if e, a := []interface{}{mkFifoObj("foo", 15)}, f.List(); !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %+v, got %+v", e, a)
+	}
+	if e, a := []string{"foo"}, f.ListKeys(); !reflect.DeepEqual(e, a) {
+		t.Errorf("Expected %+v, got %+v", e, a)
 	}
 
-	f := NewFIFO(testFifoObjectKeyFunc)
-	f.Add(mkObj("foo", 10))
-	f.Update(mkObj("foo", 15))
 	got := make(chan testFifoObject, 2)
 	go func() {
 		for {
@@ -93,20 +98,16 @@ func TestFIFO_addUpdate(t *testing.T) {
 		t.Errorf("Got second value %v", unexpected.val)
 	case <-time.After(50 * time.Millisecond):
 	}
-	_, exists, _ := f.Get(mkObj("foo", ""))
+	_, exists, _ := f.Get(mkFifoObj("foo", ""))
 	if exists {
 		t.Errorf("item did not get removed")
 	}
 }
 
 func TestFIFO_addReplace(t *testing.T) {
-	mkObj := func(name string, val interface{}) testFifoObject {
-		return testFifoObject{name: name, val: val}
-	}
-
 	f := NewFIFO(testFifoObjectKeyFunc)
-	f.Add(mkObj("foo", 10))
-	f.Replace([]interface{}{mkObj("foo", 15)})
+	f.Add(mkFifoObj("foo", 10))
+	f.Replace([]interface{}{mkFifoObj("foo", 15)})
 	got := make(chan testFifoObject, 2)
 	go func() {
 		for {
@@ -123,30 +124,26 @@ func TestFIFO_addReplace(t *testing.T) {
 		t.Errorf("Got second value %v", unexpected.val)
 	case <-time.After(50 * time.Millisecond):
 	}
-	_, exists, _ := f.Get(mkObj("foo", ""))
+	_, exists, _ := f.Get(mkFifoObj("foo", ""))
 	if exists {
 		t.Errorf("item did not get removed")
 	}
 }
 
 func TestFIFO_detectLineJumpers(t *testing.T) {
-	mkObj := func(name string, val interface{}) testFifoObject {
-		return testFifoObject{name: name, val: val}
-	}
-
 	f := NewFIFO(testFifoObjectKeyFunc)
 
-	f.Add(mkObj("foo", 10))
-	f.Add(mkObj("bar", 1))
-	f.Add(mkObj("foo", 11))
-	f.Add(mkObj("foo", 13))
-	f.Add(mkObj("zab", 30))
+	f.Add(mkFifoObj("foo", 10))
+	f.Add(mkFifoObj("bar", 1))
+	f.Add(mkFifoObj("foo", 11))
+	f.Add(mkFifoObj("foo", 13))
+	f.Add(mkFifoObj("zab", 30))
 
 	if e, a := 13, f.Pop().(testFifoObject).val; a != e {
 		t.Fatalf("expected %d, got %d", e, a)
 	}
 
-	f.Add(mkObj("foo", 14)) // ensure foo doesn't jump back in line
+	f.Add(mkFifoObj("foo", 14)) // ensure foo doesn't jump back in line
 
 	if e, a := 1, f.Pop().(testFifoObject).val; a != e {
 		t.Fatalf("expected %d, got %d", e, a)
@@ -162,16 +159,12 @@ func TestFIFO_detectLineJumpers(t *testing.T) {
 }
 
 func TestFIFO_addIfNotPresent(t *testing.T) {
-	mkObj := func(name string, val interface{}) testFifoObject {
-		return testFifoObject{name: name, val: val}
-	}
-
 	f := NewFIFO(testFifoObjectKeyFunc)
 
-	f.Add(mkObj("a", 1))
-	f.Add(mkObj("b", 2))
-	f.AddIfNotPresent(mkObj("b", 3))
-	f.AddIfNotPresent(mkObj("c", 4))
+	f.Add(mkFifoObj("a", 1))
+	f.Add(mkFifoObj("b", 2))
+	f.AddIfNotPresent(mkFifoObj("b", 3))
+	f.AddIfNotPresent(mkFifoObj("c", 4))
 
 	if e, a := 3, len(f.items); a != e {
 		t.Fatalf("expected queue length %d, got %d", e, a)

pkg/client/cache/store.go

@@ -49,6 +49,18 @@ type Store interface {
 // KeyFunc knows how to make a key from an object. Implementations should be deterministic.
 type KeyFunc func(obj interface{}) (string, error)
 
+// KeyError will be returned any time a KeyFunc gives an error; it includes the object
+// at fault.
+type KeyError struct {
+	Obj interface{}
+	Err error
+}
+
+// Error gives a human-readable description of the error.
+func (k KeyError) Error() string {
+	return fmt.Sprintf("couldn't create key for object %+v: %v", k.Obj, k.Err)
+}
+
 // 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>
@@ -79,7 +91,7 @@ type cache struct {
 func (c *cache) Add(obj interface{}) error {
 	key, err := c.keyFunc(obj)
 	if err != nil {
-		return fmt.Errorf("couldn't create key for object: %v", err)
+		return KeyError{obj, err}
 	}
 	// keep a pointer to whatever could have been there previously
 	c.lock.Lock()
@@ -148,7 +160,7 @@ func (c *cache) deleteFromIndices(obj interface{}) error {
 func (c *cache) Update(obj interface{}) error {
 	key, err := c.keyFunc(obj)
 	if err != nil {
-		return fmt.Errorf("couldn't create key for object: %v", err)
+		return KeyError{obj, err}
 	}
 	c.lock.Lock()
 	defer c.lock.Unlock()
@@ -162,7 +174,7 @@ func (c *cache) Update(obj interface{}) error {
 func (c *cache) Delete(obj interface{}) error {
 	key, err := c.keyFunc(obj)
 	if err != nil {
-		return fmt.Errorf("couldn't create key for object: %v", err)
+		return KeyError{obj, err}
 	}
 	c.lock.Lock()
 	defer c.lock.Unlock()
@@ -212,7 +224,7 @@ func (c *cache) Index(indexName string, obj interface{}) ([]interface{}, error)
 func (c *cache) Get(obj interface{}) (item interface{}, exists bool, err error) {
 	key, _ := c.keyFunc(obj)
 	if err != nil {
-		return nil, false, fmt.Errorf("couldn't create key for object: %v", err)
+		return nil, false, KeyError{obj, err}
 	}
 	return c.GetByKey(key)
 }
@@ -234,7 +246,7 @@ func (c *cache) Replace(list []interface{}) error {
 	for _, item := range list {
 		key, err := c.keyFunc(item)
 		if err != nil {
-			return fmt.Errorf("couldn't create key for object: %v", err)
+			return KeyError{item, err}
 		}
 		items[key] = item
 	}