Repository: knative/pkg
Pull request #1896 (2020-11-05 16:49:28)

Add GetObservers to tracker interface
This allows an arbitrary client to fetch all the observers of a given object. Ultimately, this enables an informer setup that can use this knowledge as a filter to decide whether or not the current object is actually watched by anything.

/assign @mattmoor @dprotaso 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1896#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)~~ [markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=h1) Report
> Merging [#1896](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=desc) into [master](https://codecov.io/gh/knative/pkg/commit/e668649c683aa4eef02c916ec2bf22d584b3a003?el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/1896/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1896      +/-   ##
==========================================
+ Coverage   69.42%   69.47%   +0.04%     
==========================================
  Files         207      207              
  Lines        8738     8739       +1     
==========================================
+ Hits         6066     6071       +5     
+ Misses       2402     2400       -2     
+ Partials      270      268       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tracker/interface.go](https://codecov.io/gh/knative/pkg/pull/1896/diff?src=pr&el=tree#diff-dHJhY2tlci9pbnRlcmZhY2UuZ28=) | `100.00% <ø> (ø)` | |
| [tracker/enqueue.go](https://codecov.io/gh/knative/pkg/pull/1896/diff?src=pr&el=tree#diff-dHJhY2tlci9lbnF1ZXVlLmdv) | `98.24% <100.00%> (+0.01%)` | :arrow_up: |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/1896/diff?src=pr&el=tree#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `92.47% <0.00%> (+2.15%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=footer). Last update [e668649...3428cf3](https://codecov.io/gh/knative/pkg/pull/1896?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/tracker/enqueue.go b/tracker/enqueue.go
index 2e001470c..70fbe0e46 100644
--- a/tracker/enqueue.go
+++ b/tracker/enqueue.go
@@ -216,9 +216,18 @@ func isExpired(expiry time.Time) bool {
 
 // OnChanged implements Interface.
 func (i *impl) OnChanged(obj interface{}) {
+	observers := i.GetObservers(obj)
+
+	for _, observer := range observers {
+		i.cb(observer)
+	}
+}
+
+// GetObservers implements Interface.
+func (i *impl) GetObservers(obj interface{}) []types.NamespacedName {
 	item, err := kmeta.DeletionHandlingAccessor(obj)
 	if err != nil {
-		return
+		return nil
 	}
 
 	or := kmeta.ObjectReference(item)
@@ -229,14 +238,9 @@ func (i *impl) OnChanged(obj interface{}) {
 		Name:       or.Name,
 	}
 
-	i.m.Lock()
-	// Call the callbacks without the lock held.
 	var keys []types.NamespacedName
-	defer func(cb func(types.NamespacedName)) {
-		for _, key := range keys {
-			cb(key)
-		}
-	}(i.cb) // read i.cb with the lock held
+
+	i.m.Lock()
 	defer i.m.Unlock()
 
 	// Handle exact matches.
@@ -274,6 +278,8 @@ func (i *impl) OnChanged(obj interface{}) {
 			delete(i.exact, ref)
 		}
 	}
+
+	return keys
 }
 
 // OnChanged implements Interface.
diff --git a/tracker/interface.go b/tracker/interface.go
index c8ab479fa..bfcda9cf2 100644
--- a/tracker/interface.go
+++ b/tracker/interface.go
@@ -23,6 +23,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation"
 
 	"knative.dev/pkg/apis"
@@ -71,6 +72,10 @@ type Interface interface {
 	// so that we are notified for appropriate object changes.
 	OnChanged(obj interface{})
 
+	// GetObservers returns the names of all observers for the given
+	// object.
+	GetObservers(obj interface{}) []types.NamespacedName
+
 	// OnDeletedObserver is a callback to register with the InformerFactory
 	// so that we are notified for deletions of a watching parent to
 	// remove the respective tracking.


Test patch:
diff --git a/reconciler/testing/tracker.go b/reconciler/testing/tracker.go
index 35a15f896..e586097d9 100644
--- a/reconciler/testing/tracker.go
+++ b/reconciler/testing/tracker.go
@@ -41,6 +41,31 @@ var _ tracker.Interface = (*FakeTracker)(nil)
 // OnChanged implements OnChanged.
 func (*FakeTracker) OnChanged(interface{}) {}
 
+// GetObservers implements GetObservers.
+func (n *FakeTracker) GetObservers(obj interface{}) []types.NamespacedName {
+	item, err := kmeta.DeletionHandlingAccessor(obj)
+	if err != nil {
+		return nil
+	}
+
+	or := kmeta.ObjectReference(item)
+	ref := tracker.Reference{
+		APIVersion: or.APIVersion,
+		Kind:       or.Kind,
+		Namespace:  or.Namespace,
+		Name:       or.Name,
+	}
+
+	n.Lock()
+	defer n.Unlock()
+
+	keys := make([]types.NamespacedName, 0, len(n.references[ref]))
+	for key := range n.references[ref] {
+		keys = append(keys, key)
+	}
+	return keys
+}
+
 // OnDeletedObserver implements OnDeletedObserver.
 func (n *FakeTracker) OnDeletedObserver(obj interface{}) {
 	item, err := kmeta.DeletionHandlingAccessor(obj)
diff --git a/reconciler/testing/tracker_test.go b/reconciler/testing/tracker_test.go
index 6f15ccd40..90a1e236f 100644
--- a/reconciler/testing/tracker_test.go
+++ b/reconciler/testing/tracker_test.go
@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	. "knative.dev/pkg/testing"
 	"knative.dev/pkg/tracker"
 )
@@ -30,24 +31,52 @@ func TestFakeTracker(t *testing.T) {
 			Name:      "foo",
 		},
 	}
+	t1Name := types.NamespacedName{
+		Namespace: t1.Namespace,
+		Name:      t1.Name,
+	}
 	t2 := &Resource{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "default",
 			Name:      "bar.baz.this-is-fine",
 		},
 	}
+	t2Name := types.NamespacedName{
+		Namespace: t2.Namespace,
+		Name:      t2.Name,
+	}
 
+	obj1 := &Resource{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "fakeapi",
+			Kind:       "Fake",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo",
+			Name:      "bar",
+		},
+	}
 	ref1 := tracker.Reference{
-		APIVersion: "fakeapi",
-		Kind:       "Fake",
-		Namespace:  "foo",
-		Name:       "bar",
+		APIVersion: obj1.APIVersion,
+		Kind:       obj1.Kind,
+		Namespace:  obj1.Namespace,
+		Name:       obj1.Name,
+	}
+	obj2 := &Resource{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "fakeapi2",
+			Kind:       "Fake2",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "foo2",
+			Name:      "bar2",
+		},
 	}
 	ref2 := tracker.Reference{
-		APIVersion: "fakeapi2",
-		Kind:       "Fake2",
-		Namespace:  "foo2",
-		Name:       "bar2",
+		APIVersion: obj2.APIVersion,
+		Kind:       obj2.Kind,
+		Namespace:  obj2.Namespace,
+		Name:       obj2.Name,
 	}
 
 	trk := &FakeTracker{}
@@ -57,10 +86,17 @@ func TestFakeTracker(t *testing.T) {
 	if !isTracking(trk, ref1) {
 		t.Fatal("Tracker is not tracking", ref1)
 	}
+	if !hasObserver(trk, obj1, t1Name) {
+		t.Fatalf("Object %v is not being observed by %v", obj1, t1Name)
+	}
+
 	trk.OnDeletedObserver(t1)
 	if isTracking(trk, ref1) {
 		t.Fatal("Tracker is still tracking", ref1)
 	}
+	if hasObserver(trk, obj1, t1Name) {
+		t.Fatalf("Object %v is still being observed by %v", obj1, t1Name)
+	}
 
 	// Adding t1, t2 to ref1 and t2 to ref2, then removing t2 results in ref2 stopping
 	// tracking.
@@ -73,17 +109,43 @@ func TestFakeTracker(t *testing.T) {
 	if !isTracking(trk, ref2) {
 		t.Fatal("Tracker is not tracking", ref2)
 	}
+	if !hasObserver(trk, obj1, t1Name) {
+		t.Fatalf("Object %v is not being observed by %v", obj1, t1Name)
+	}
+	if !hasObserver(trk, obj1, t2Name) {
+		t.Fatalf("Object %v is not being observed by %v", obj1, t2Name)
+	}
+	if hasObserver(trk, obj2, t1Name) {
+		t.Fatalf("Object %v wrongly being observed by %v", obj2, t1Name)
+	}
+	if !hasObserver(trk, obj2, t2Name) {
+		t.Fatalf("Object %v is not being observed by %v", obj2, t2Name)
+	}
+
 	trk.OnDeletedObserver(t2)
 	if !isTracking(trk, ref1) {
 		t.Fatal("Tracker is not tracking", ref1)
 	}
+	if !hasObserver(trk, obj1, t1Name) {
+		t.Fatalf("Object %v is not being observed by %v", obj1, t1Name)
+	}
 	if isTracking(trk, ref2) {
 		t.Fatal("Tracker is still tracking", ref2)
 	}
+	if hasObserver(trk, obj1, t2Name) {
+		t.Fatalf("Object %v is still being observed by %v", obj1, t2Name)
+	}
+	if hasObserver(trk, obj2, t2Name) {
+		t.Fatalf("Object %v is still being observed by %v", obj2, t2Name)
+	}
+
 	trk.OnDeletedObserver(t1)
 	if isTracking(trk, ref1) {
 		t.Fatal("Tracker is still tracking", ref1)
 	}
+	if hasObserver(trk, obj1, t1Name) {
+		t.Fatalf("Object %v is still being observed by %v", obj1, t1Name)
+	}
 }
 
 func isTracking(tracker *FakeTracker, ref1 tracker.Reference) bool {
@@ -94,3 +156,13 @@ func isTracking(tracker *FakeTracker, ref1 tracker.Reference) bool {
 	}
 	return false
 }
+
+func hasObserver(tracker *FakeTracker, obj *Resource, obs types.NamespacedName) bool {
+	observers := tracker.GetObservers(obj)
+	for _, observer := range observers {
+		if observer == obs {
+			return true
+		}
+	}
+	return false
+}
diff --git a/tracker/enqueue_test.go b/tracker/enqueue_test.go
index a5afbf5e3..18f7651ff 100644
--- a/tracker/enqueue_test.go
+++ b/tracker/enqueue_test.go
@@ -73,6 +73,10 @@ func TestHappyPathsExact(t *testing.T) {
 		if got, want := calls, 0; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Tracked gets called
@@ -89,6 +93,10 @@ func TestHappyPathsExact(t *testing.T) {
 		if got, want := calls, 2; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 1; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Still gets called
@@ -109,6 +117,10 @@ func TestHappyPathsExact(t *testing.T) {
 		if _, stillThere := trk.(*impl).exact[ref]; stillThere {
 			t.Fatal("Timeout passed, but exact for objectReference is still there")
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Starts getting called again
@@ -125,6 +137,10 @@ func TestHappyPathsExact(t *testing.T) {
 		if got, want := calls, 5; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 1; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// OnChanged non-accessor
@@ -171,6 +187,10 @@ func TestHappyPathsExact(t *testing.T) {
 		if got, want := calls, 6; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Track bad object
@@ -178,6 +198,10 @@ func TestHappyPathsExact(t *testing.T) {
 		if err := trk.Track(ref.ObjectReference(), struct{}{}); err == nil {
 			t.Fatal("Track() = nil, wanted error")
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 }
 
@@ -504,6 +528,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if got, want := calls, 0; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Tracked gets called
@@ -520,6 +548,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if got, want := calls, 2; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 1; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Still gets called
@@ -540,6 +572,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if _, stillThere := trk.(*impl).exact[ref]; stillThere {
 			t.Fatal("Timeout passed, but exact for objectReference is still there")
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Starts getting called again
@@ -556,6 +592,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if got, want := calls, 5; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 1; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// OnChanged non-accessor
@@ -604,6 +644,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if got, want := calls, 6; got != want {
 			t.Fatalf("Track() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 1; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 
 		thing1unlabeled := thing1.DeepCopy()
 		thing1unlabeled.Labels = nil
@@ -647,6 +691,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if got, want := calls, 7; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 
 	// Track bad object
@@ -654,6 +702,10 @@ func TestHappyPathsInexact(t *testing.T) {
 		if err := trk.TrackReference(ref, struct{}{}); err == nil {
 			t.Fatal("Track() = nil, wanted error")
 		}
+		obs := trk.GetObservers(thing1)
+		if got, want := len(obs), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 }
 
@@ -726,6 +778,9 @@ func TestHappyPathsByBoth(t *testing.T) {
 		if got, want := calls, 1; got != want {
 			t.Fatalf("Track() = %v, wanted %v", got, want)
 		}
+		if got, want := len(trk.GetObservers(thing1)), 1; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 
 		if err := trk.TrackReference(ref2, thing2); err != nil {
 			t.Fatal("Track() =", err)
@@ -734,6 +789,9 @@ func TestHappyPathsByBoth(t *testing.T) {
 		if got, want := calls, 2; got != want {
 			t.Fatalf("Track() = %v, wanted %v", got, want)
 		}
+		if got, want := len(trk.GetObservers(thing1)), 2; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 
 		// The callback should be called for each of the tracks (exact and inexact)
 		trk.OnChanged(thing1)
@@ -749,5 +807,8 @@ func TestHappyPathsByBoth(t *testing.T) {
 		if got, want := calls, 4; got != want {
 			t.Fatalf("OnChanged() = %v, wanted %v", got, want)
 		}
+		if got, want := len(trk.GetObservers(thing1)), 0; got != want {
+			t.Fatalf("len(GetObservers()) = %v, wanted %v", got, want)
+		}
 	}
 }