Repository: knative/pkg
Pull request #1349 (2020-05-25 09:09:52)

Make onAfterStore synchronous to the update itself.
Generally, the `onAfterStore` function is used to kick off a global resync, which should be a very quick operation. Having `onAfterStore` being called asynchronously on a goroutine makes it very hard or straight out impossible to control it, especially in tests.

If there's a costly operation being done `onAfterStore`, it should be up to the caller to decide to make that asynchronous and thus put it into a goroutine.

This contributes to a lot of test flakes, at least in Knative Serving.

/assign @vagababov 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1349#" title="Author self-approved">markusthoemmes</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mdemirhan**
You can assign the PR to them by writing `/assign @mdemirhan` in a comment when ready.

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

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

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

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":["mdemirhan"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[configmap/store.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1349/pull-knative-pkg-go-coverage/1264846229255229440/artifacts/line-cov.html#file0) | 96.7% | 96.6% | -0.1

dprotaso: /lgtm

vagababov: Since it's not gonna make to release, we have lots of time to see if it is not a good idea (I.e. we're timing out because of this).
So some "It took %v to update all CM watchers" would be nice.
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1349#" title="Author self-approved">markusthoemmes</a>*, *<a href="https://github.com/knative/pkg/pull/1349#pullrequestreview-417823300" title="Approved">vagababov</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:

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

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":[]} -->

Patch:
diff --git a/configmap/store.go b/configmap/store.go
index 825803c25..a994601ab 100644
--- a/configmap/store.go
+++ b/configmap/store.go
@@ -157,9 +157,7 @@ func (s *UntypedStore) OnConfigChanged(c *corev1.ConfigMap) {
 	s.logger.Infof("%s config %q config was added or updated: %#v", s.name, name, result)
 	storage.Store(result)
 
-	go func() {
-		for _, f := range s.onAfterStore {
-			f(name, result)
-		}
-	}()
+	for _, f := range s.onAfterStore {
+		f(name, result)
+	}
 }


Test patch:
diff --git a/configmap/store_test.go b/configmap/store_test.go
index f5ba2c002..2c2527d2d 100644
--- a/configmap/store_test.go
+++ b/configmap/store_test.go
@@ -32,6 +32,15 @@ import (
 	. "knative.dev/pkg/logging/testing"
 )
 
+const (
+	config1 = "config-name-1"
+	config2 = "config-name-2"
+)
+
+var constructor = func(c *corev1.ConfigMap) (interface{}, error) {
+	return c.Name, nil
+}
+
 func TestStoreBadConstructors(t *testing.T) {
 	tests := []struct {
 		name        string
@@ -69,17 +78,13 @@ func TestStoreBadConstructors(t *testing.T) {
 }
 
 func TestStoreWatchConfigs(t *testing.T) {
-	constructor := func(c *corev1.ConfigMap) (interface{}, error) {
-		return c.Name, nil
-	}
-
 	defer ClearAll()
 	store := NewUntypedStore(
 		"name",
 		TestLogger(t),
 		Constructors{
-			"config-name-1": constructor,
-			"config-name-2": constructor,
+			config1: constructor,
+			config2: constructor,
 		},
 	)
 
@@ -87,8 +92,8 @@ func TestStoreWatchConfigs(t *testing.T) {
 	store.WatchConfigs(watcher)
 
 	want := []string{
-		"config-name-1",
-		"config-name-2",
+		config1,
+		config2,
 	}
 
 	got := watcher.watches
@@ -98,112 +103,64 @@ func TestStoreWatchConfigs(t *testing.T) {
 	}
 }
 
-func appendTo(evidence *[]string) func(int) func(string, interface{}) {
-	return func(i int) func(string, interface{}) {
-		return func(name string, value interface{}) {
-			*evidence = append(*evidence, fmt.Sprintf("%s:%s:%v", name, value, i))
-		}
-	}
-}
-
-func listOf(f func(int) func(string, interface{}), count int, tail ...func(string, interface{})) []func(string, interface{}) {
-	result := make([]func(string, interface{}), 0, count)
-	for i := 0; i < count; i++ {
-		result = append(result, f(i))
-	}
-	return append(result, tail...)
-}
-
-func expectedEvidence(name string, count int) []string {
-	var result []string
-	for i := 0; i < count; i++ {
-		result = append(result, fmt.Sprintf("%s:%s:%v", name, name, i))
-	}
-	return result
-}
-
-func signalComplete(done chan<- string) func(string, interface{}) {
-	return func(name string, value interface{}) {
-		done <- fmt.Sprintf("%s:%s", name, value)
-	}
-}
-
 func TestOnAfterStore(t *testing.T) {
-	constructor := func(c *corev1.ConfigMap) (interface{}, error) {
-		return c.Name, nil
-	}
-
-	var evidence []string
-	completeCh := make(chan string)
-
+	var calledFor string
 	store := NewUntypedStore(
 		"name",
 		TestLogger(t),
 		Constructors{
-			"config-name-1": constructor,
-			"config-name-2": constructor,
+			config1: constructor,
+			config2: constructor,
+		},
+		func(name string, value interface{}) {
+			calledFor = name
 		},
-		listOf(appendTo(&evidence), 100, signalComplete(completeCh))...,
 	)
 
 	store.OnConfigChanged(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-name-1",
+			Name: config1,
 		},
 	})
 
-	if diff := cmp.Diff("config-name-1:config-name-1", <-completeCh); diff != "" {
-		t.Fatalf("Expected value from completeCh diff: %s", diff)
+	if calledFor != config1 {
+		t.Fatalf("calledFor = %s, want %s", calledFor, config1)
 	}
 
-	if diff := cmp.Diff(expectedEvidence("config-name-1", 100), evidence); diff != "" {
-		t.Fatalf("Expected evidence diff: %s", diff)
-	}
-
-	evidence = nil
-
 	store.OnConfigChanged(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-name-2",
+			Name: config2,
 		},
 	})
 
-	if diff := cmp.Diff("config-name-2:config-name-2", <-completeCh); diff != "" {
-		t.Fatalf("Expected value from completeCh diff: %s", diff)
-	}
-
-	if diff := cmp.Diff(expectedEvidence("config-name-2", 100), evidence); diff != "" {
-		t.Fatalf("Expected evidence diff: %s", diff)
+	if calledFor != config2 {
+		t.Fatalf("calledFor = %s, want %s", calledFor, config2)
 	}
 }
 
 func TestStoreConfigChange(t *testing.T) {
-	constructor := func(c *corev1.ConfigMap) (interface{}, error) {
-		return c.Name, nil
-	}
-
 	store := NewUntypedStore(
 		"name",
 		TestLogger(t),
 		Constructors{
-			"config-name-1": constructor,
-			"config-name-2": constructor,
+			config1: constructor,
+			config2: constructor,
 		},
 	)
 
 	store.OnConfigChanged(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-name-1",
+			Name: config1,
 		},
 	})
 
-	result := store.UntypedLoad("config-name-1")
+	result := store.UntypedLoad(config1)
 
-	if diff := cmp.Diff(result, "config-name-1"); diff != "" {
+	if diff := cmp.Diff(result, config1); diff != "" {
 		t.Errorf("Expected loaded value diff: %s", diff)
 	}
 
-	result = store.UntypedLoad("config-name-2")
+	result = store.UntypedLoad(config2)
 
 	if diff := cmp.Diff(result, nil); diff != "" {
 		t.Errorf("Unexpected loaded value diff: %s", diff)
@@ -211,13 +168,13 @@ func TestStoreConfigChange(t *testing.T) {
 
 	store.OnConfigChanged(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-name-2",
+			Name: config2,
 		},
 	})
 
-	result = store.UntypedLoad("config-name-2")
+	result = store.UntypedLoad(config2)
 
-	if diff := cmp.Diff(result, "config-name-2"); diff != "" {
+	if diff := cmp.Diff(result, config2); diff != "" {
 		t.Errorf("Expected loaded value diff: %s", diff)
 	}
 }
@@ -229,12 +186,12 @@ func TestStoreFailedFirstConversionCrashes(t *testing.T) {
 		}
 
 		store := NewUntypedStore("name", TestLogger(t),
-			Constructors{"config-name-1": constructor},
+			Constructors{config1: constructor},
 		)
 
 		store.OnConfigChanged(&corev1.ConfigMap{
 			ObjectMeta: metav1.ObjectMeta{
-				Name: "config-name-1",
+				Name: config1,
 			},
 		})
 		return
@@ -261,25 +218,25 @@ func TestStoreFailedUpdate(t *testing.T) {
 	}
 
 	store := NewUntypedStore("name", TestLogger(t),
-		Constructors{"config-name-1": constructor},
+		Constructors{config1: constructor},
 	)
 
 	store.OnConfigChanged(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-name-1",
+			Name: config1,
 		},
 	})
 
-	firstLoad := store.UntypedLoad("config-name-1")
+	firstLoad := store.UntypedLoad(config1)
 
 	induceConstructorFailure = true
 	store.OnConfigChanged(&corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-name-1",
+			Name: config1,
 		},
 	})
 
-	secondLoad := store.UntypedLoad("config-name-1")
+	secondLoad := store.UntypedLoad(config1)
 
 	if diff := cmp.Diff(firstLoad, secondLoad); diff != "" {
 		t.Errorf("Expected loaded value to remain the same dff: %s", diff)