Repository: kubernetes/klog
Pull request #226 (2021-03-12 13:51:29)

Fix cases where KMetadata interface has nil object
DO NOT MERGE, this is one of the alternatives considered to fix https://github.com/kubernetes/kubernetes/issues/100155

**What this PR does / why we need it**:

Fixes passing `nil` object to `klog.KObj`

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes https://github.com/kubernetes/kubernetes/issues/100155

**Special notes for your reviewer**:

Added benchmark run them with `go test -bench=BenchmarkKObj -count=100 -benchmem -run=^$` and got results analysed by [benchstat](https://github.com/golang/perf/tree/master/cmd/benchstat):
```
name    old time/op    new time/op    delta
KObj-8    7.04ns ± 6%    7.50ns ± 8%  +6.57%  (p=0.000 n=89+93)

name    old alloc/op   new alloc/op   delta
KObj-8     0.00B          0.00B         ~     (all equal)

name    old allocs/op  new allocs/op  delta
KObj-8      0.00           0.00         ~     (all equal)
```

We see around 19% worse performance by introducing reflections, still looks acceptable.

**Release note**:
```release-note
NONE
```

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/klog/pull/226#" title="Author self-approved">serathius</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **lavalamp** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @lavalamp` 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=kubernetes%2Fklog).

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

- **[OWNERS](https://github.com/kubernetes/klog/blob/master/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":["lavalamp"]} -->
serathius: Removed second call to `reflect.ValueOf`, results look much better:
```
name    old time/op    new time/op    delta
KObj-8    42.2ns ± 8%    43.4ns ± 8%  +2.90%  (p=0.000 n=98+98)

name    old alloc/op   new alloc/op   delta
KObj-8     32.0B ± 0%     32.0B ± 0%    ~     (all equal)

name    old allocs/op  new allocs/op  delta
KObj-8      1.00 ± 0%      1.00 ± 0%    ~     (all equal)
```
Looks like the overhead is only 3%!!!
serathius: Removing WIP as benchmark results look reasonable  
serathius: Fixed how benchmark is implemented
dims: LGTM, i'd like a second opinion/review from @liggitt or @DirectXMan12 
serathius: Updated benchmark results, looks better around 7% difference.
dims: /approve

leaving `/lgtm` for @liggitt or @ehashman 

i can cut a release once this is merged
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/klog/pull/226#issuecomment-797662760" title="Approved">dims</a>*, *<a href="https://github.com/kubernetes/klog/pull/226#" title="Author self-approved">serathius</a>*

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

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/kubernetes/klog/blob/master/OWNERS)~~ [dims]

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/klog.go b/klog.go
index 4892da6..25483fa 100644
--- a/klog.go
+++ b/klog.go
@@ -81,6 +81,7 @@ import (
 	"math"
 	"os"
 	"path/filepath"
+	"reflect"
 	"runtime"
 	"strconv"
 	"strings"
@@ -1580,6 +1581,9 @@ func KObj(obj KMetadata) ObjectRef {
 	if obj == nil {
 		return ObjectRef{}
 	}
+	if val := reflect.ValueOf(obj); val.Kind() == reflect.Ptr && val.IsNil() {
+		return ObjectRef{}
+	}
 
 	return ObjectRef{
 		Name:      obj.GetName(),


Test patch:
diff --git a/klog_test.go b/klog_test.go
index 671e4c1..1e368cb 100644
--- a/klog_test.go
+++ b/klog_test.go
@@ -621,6 +621,26 @@ func BenchmarkHeaderWithDir(b *testing.B) {
 	}
 }
 
+// Ensure that benchmarks have side effects to avoid compiler optimization
+var result ObjectRef
+
+func BenchmarkKRef(b *testing.B) {
+	var r ObjectRef
+	for i := 0; i < b.N; i++ {
+		r = KRef("namespace", "name")
+	}
+	result = r
+}
+
+func BenchmarkKObj(b *testing.B) {
+	a := kMetadataMock{name: "a", ns: "a"}
+	var r ObjectRef
+	for i := 0; i < b.N; i++ {
+		r = KObj(&a)
+	}
+	result = r
+}
+
 func BenchmarkLogs(b *testing.B) {
 	setFlags()
 	defer logging.swap(logging.newBuffers())
@@ -734,6 +754,11 @@ func TestInfoObjectRef(t *testing.T) {
 			},
 			want: "test-name",
 		},
+		{
+			name: "empty",
+			ref:  ObjectRef{},
+			want: "",
+		},
 	}
 
 	for _, tt := range tests {
@@ -746,14 +771,25 @@ func TestInfoObjectRef(t *testing.T) {
 	}
 }
 
-type mockKmeta struct {
+type kMetadataMock struct {
+	name, ns string
+}
+
+func (m kMetadataMock) GetName() string {
+	return m.name
+}
+func (m kMetadataMock) GetNamespace() string {
+	return m.ns
+}
+
+type ptrKMetadataMock struct {
 	name, ns string
 }
 
-func (m mockKmeta) GetName() string {
+func (m *ptrKMetadataMock) GetName() string {
 	return m.name
 }
-func (m mockKmeta) GetNamespace() string {
+func (m *ptrKMetadataMock) GetNamespace() string {
 	return m.ns
 }
 
@@ -763,6 +799,21 @@ func TestKObj(t *testing.T) {
 		obj  KMetadata
 		want ObjectRef
 	}{
+		{
+			name: "nil passed as pointer KMetadata implementation",
+			obj:  (*ptrKMetadataMock)(nil),
+			want: ObjectRef{},
+		},
+		{
+			name: "empty struct passed as non-pointer KMetadata implementation",
+			obj:  kMetadataMock{},
+			want: ObjectRef{},
+		},
+		{
+			name: "nil pointer passed to non-pointer KMetadata implementation",
+			obj:  (*kMetadataMock)(nil),
+			want: ObjectRef{},
+		},
 		{
 			name: "nil",
 			obj:  nil,
@@ -770,7 +821,7 @@ func TestKObj(t *testing.T) {
 		},
 		{
 			name: "with ns",
-			obj:  mockKmeta{"test-name", "test-ns"},
+			obj:  &kMetadataMock{"test-name", "test-ns"},
 			want: ObjectRef{
 				Name:      "test-name",
 				Namespace: "test-ns",
@@ -778,7 +829,7 @@ func TestKObj(t *testing.T) {
 		},
 		{
 			name: "without ns",
-			obj:  mockKmeta{"test-name", ""},
+			obj:  &kMetadataMock{"test-name", ""},
 			want: ObjectRef{
 				Name: "test-name",
 			},
@@ -1029,13 +1080,25 @@ func TestKvListFormat(t *testing.T) {
 			want:       " pod=\"kubedns\" status=\"ready\"",
 		},
 		{
-			keysValues: []interface{}{"pod", KObj(mockKmeta{"test-name", "test-ns"}), "status", "ready"},
+			keysValues: []interface{}{"pod", KObj(kMetadataMock{"test-name", "test-ns"}), "status", "ready"},
 			want:       " pod=\"test-ns/test-name\" status=\"ready\"",
 		},
 		{
-			keysValues: []interface{}{"pod", KObj(mockKmeta{"test-name", ""}), "status", "ready"},
+			keysValues: []interface{}{"pod", KObj(kMetadataMock{"test-name", ""}), "status", "ready"},
 			want:       " pod=\"test-name\" status=\"ready\"",
 		},
+		{
+			keysValues: []interface{}{"pod", KObj(nil), "status", "ready"},
+			want:       " pod=\"\" status=\"ready\"",
+		},
+		{
+			keysValues: []interface{}{"pod", KObj((*ptrKMetadataMock)(nil)), "status", "ready"},
+			want:       " pod=\"\" status=\"ready\"",
+		},
+		{
+			keysValues: []interface{}{"pod", KObj((*kMetadataMock)(nil)), "status", "ready"},
+			want:       " pod=\"\" status=\"ready\"",
+		},
 	}
 
 	for _, d := range testKVList {