Repository: knative/pkg
Pull request #1348 (2020-05-22 00:26:38)

Add the key of the object to the log context
We don't log _what_ we convert, but only _what type_ it is.
And it's not very useful
So log all the stuff

/assign @dprotaso @mattmoor 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1348#" title="Author self-approved">vagababov</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` 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:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/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":["mattmoor"]} -->
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
---- |:------------:|:------------:|:-----:
[webhook/resourcesemantics/conversion/conversion.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1348/pull-knative-pkg-go-coverage/1263627448298246145/artifacts/line-cov.html#file0) | 98.7% | 97.6% | -1.2


Patch:
diff --git a/apis/duck/v1/status_types.go b/apis/duck/v1/status_types.go
index 9186e961f..365cc5edf 100644
--- a/apis/duck/v1/status_types.go
+++ b/apis/duck/v1/status_types.go
@@ -45,6 +45,12 @@ type Status struct {
 	// +patchMergeKey=type
 	// +patchStrategy=merge
 	Conditions Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
+
+	// Annotations is additional Status fields for the Resource to save some
+	// additional State as well as convey more information to the user. This is
+	// roughly akin to Annotations on any k8s resource, just the reconciler conveying
+	// richer information outwards.
+	Annotations map[string]string `json:"annotations,omitempty"`
 }
 
 var _ apis.ConditionsAccessor = (*Status)(nil)
diff --git a/apis/duck/v1/zz_generated.deepcopy.go b/apis/duck/v1/zz_generated.deepcopy.go
index 3397713be..190d773e7 100644
--- a/apis/duck/v1/zz_generated.deepcopy.go
+++ b/apis/duck/v1/zz_generated.deepcopy.go
@@ -428,6 +428,13 @@ func (in *Status) DeepCopyInto(out *Status) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	if in.Annotations != nil {
+		in, out := &in.Annotations, &out.Annotations
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	return
 }
 
diff --git a/apis/duck/v1beta1/status_types.go b/apis/duck/v1beta1/status_types.go
index b2095fb58..a046dbedf 100644
--- a/apis/duck/v1beta1/status_types.go
+++ b/apis/duck/v1beta1/status_types.go
@@ -63,6 +63,12 @@ type Status struct {
 	// +patchMergeKey=type
 	// +patchStrategy=merge
 	Conditions Conditions `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
+
+	// Annotations is additional Status fields for the Resource to save some
+	// additional State as well as convey more information to the user. This is
+	// roughly akin to Annotations on any k8s resource, just the reconciler conveying
+	// richer information outwards.
+	Annotations map[string]string `json:"annotations,omitempty"`
 }
 
 var _ apis.ConditionsAccessor = (*Status)(nil)
diff --git a/apis/duck/v1beta1/zz_generated.deepcopy.go b/apis/duck/v1beta1/zz_generated.deepcopy.go
index 032dccb3c..152bb8828 100644
--- a/apis/duck/v1beta1/zz_generated.deepcopy.go
+++ b/apis/duck/v1beta1/zz_generated.deepcopy.go
@@ -374,6 +374,13 @@ func (in *Status) DeepCopyInto(out *Status) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
+	if in.Annotations != nil {
+		in, out := &in.Annotations, &out.Annotations
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	return
 }
 
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)
+	}
 }
diff --git a/webhook/resourcesemantics/conversion/conversion.go b/webhook/resourcesemantics/conversion/conversion.go
index e0d495b9f..3fab95bad 100644
--- a/webhook/resourcesemantics/conversion/conversion.go
+++ b/webhook/resourcesemantics/conversion/conversion.go
@@ -22,12 +22,16 @@ import (
 	"fmt"
 
 	"go.uber.org/zap"
+
 	apixv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+
 	"knative.dev/pkg/apis"
+	"knative.dev/pkg/kmeta"
 	"knative.dev/pkg/logging"
+	"knative.dev/pkg/logging/logkey"
 )
 
 // Convert implements webhook.ConversionController
@@ -78,8 +82,6 @@ func (r *reconciler) convert(
 		return ret, err
 	}
 
-	logger.Infof("Converting %s to version %s", formatGVK(inGVK), targetVersion)
-
 	inGK := inGVK.GroupKind()
 	conv, ok := r.kinds[inGK]
 	if !ok {
@@ -109,17 +111,28 @@ func (r *reconciler) convert(
 	out := outZygote.DeepCopyObject().(ConvertibleObject)
 
 	hubGVK := inGVK.GroupKind().WithVersion(conv.HubVersion)
-	ctx = logging.WithLogger(ctx, logger.With(
+
+	logger = logger.With(
 		zap.String("inputType", formatGVK(inGVK)),
 		zap.String("outputType", formatGVK(outGVK)),
 		zap.String("hubType", formatGVK(hubGVK)),
-	))
+	)
 
 	// TODO(dprotaso) - potentially error on unknown fields
 	if err = json.Unmarshal(inRaw.Raw, &in); err != nil {
 		return ret, fmt.Errorf("unable to unmarshal input: %w", err)
 	}
 
+	if acc, err := kmeta.DeletionHandlingAccessor(in); err == nil {
+		// TODO: right now we don't convert any non-namespaced objects. If we ever do that
+		// this needs to updated to deal with it.
+		logger = logger.With(zap.String(logkey.Key, acc.GetNamespace()+"/"+acc.GetName()))
+	} else {
+		logger.Infof("Could not get Accessor for %s: %v", formatGK(inGVK.GroupKind()), err)
+	}
+	logger.Infof("Converting %s to version %s", formatGVK(inGVK), targetVersion)
+	ctx = logging.WithLogger(ctx, logger)
+
 	if inGVK.Version == conv.HubVersion {
 		hub = in
 	} else if err = hub.ConvertFrom(ctx, in); err != nil {


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)