Refactor pods format to ObjRef slice

Refactor format.Pods to return a slice of ObjRef for structured logging.
Ref: https://github.com/kubernetes/kubernetes/pull/99029#discussion_r586785552

Signed-off-by: Qi Wang <qiwan@redhat.com>

pkg/kubelet/kubelet.go

@@ -1927,23 +1927,23 @@ func (kl *Kubelet) syncLoopIteration(configCh <-chan kubetypes.PodUpdate, handle
 
 		switch u.Op {
 		case kubetypes.ADD:
-			klog.V(2).Infof("SyncLoop (ADD, %q): %q", u.Source, format.Pods(u.Pods))
+			klog.V(2).InfoS("SyncLoop ADD", "source", u.Source, "pods", format.Pods(u.Pods))
 			// After restarting, kubelet will get all existing pods through
 			// ADD as if they are new pods. These pods will then go through the
 			// admission process and *may* be rejected. This can be resolved
 			// once we have checkpointing.
 			handler.HandlePodAdditions(u.Pods)
 		case kubetypes.UPDATE:
-			klog.V(2).Infof("SyncLoop (UPDATE, %q): %q", u.Source, format.PodsWithDeletionTimestamps(u.Pods))
+			klog.V(2).InfoS("SyncLoop UPDATE", "source", u.Source, "pods", format.Pods(u.Pods))
 			handler.HandlePodUpdates(u.Pods)
 		case kubetypes.REMOVE:
-			klog.V(2).Infof("SyncLoop (REMOVE, %q): %q", u.Source, format.Pods(u.Pods))
+			klog.V(2).InfoS("SyncLoop REMOVE", "source", u.Source, "pods", format.Pods(u.Pods))
 			handler.HandlePodRemoves(u.Pods)
 		case kubetypes.RECONCILE:
-			klog.V(4).Infof("SyncLoop (RECONCILE, %q): %q", u.Source, format.Pods(u.Pods))
+			klog.V(4).InfoS("SyncLoop RECONCILE", "source", u.Source, "pods", format.Pods(u.Pods))
 			handler.HandlePodReconcile(u.Pods)
 		case kubetypes.DELETE:
-			klog.V(2).Infof("SyncLoop (DELETE, %q): %q", u.Source, format.Pods(u.Pods))
+			klog.V(2).Infof("SyncLoop DELETE", "source", u.Source, "pods", format.Pods(u.Pods))
 			// DELETE is treated as a UPDATE because of graceful deletion.
 			handler.HandlePodUpdates(u.Pods)
 		case kubetypes.SET:
@@ -1984,7 +1984,7 @@ func (kl *Kubelet) syncLoopIteration(configCh <-chan kubetypes.PodUpdate, handle
 		if len(podsToSync) == 0 {
 			break
 		}
-		klog.V(4).Infof("SyncLoop (SYNC): %d pods; %s", len(podsToSync), format.Pods(podsToSync))
+		klog.V(4).InfoS("SyncLoop (SYNC) pods", "total", len(podsToSync), "pods", format.Pods(podsToSync))
 		handler.HandlePodSyncs(podsToSync)
 	case update := <-kl.livenessManager.Updates():
 		if update.Result == proberesults.Failure {

pkg/kubelet/util/format/pod.go

@@ -18,15 +18,13 @@ package format
 
 import (
 	"fmt"
-	"strings"
 	"time"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/klog/v2"
 )
 
-type podHandler func(*v1.Pod) string
-
 // Pod returns a string representing a pod in a consistent human readable format,
 // with pod UID as part of the string.
 func Pod(pod *v1.Pod) string {
@@ -57,22 +55,13 @@ func PodWithDeletionTimestamp(pod *v1.Pod) string {
 	return Pod(pod) + deletionTimestamp
 }
 
-// Pods returns a string representation a list of pods in a human
+// Pods returns a list of pods as ObjectRef
-// readable format.
+func Pods(pods []*v1.Pod) []klog.ObjectRef {
-func Pods(pods []*v1.Pod) string {
+	podKObjs := make([]klog.ObjectRef, 0, len(pods))
-	return aggregatePods(pods, Pod)
+	for _, p := range pods {
-}
+		if p != nil {
-
+			podKObjs = append(podKObjs, klog.KObj(p))
-// PodsWithDeletionTimestamps is the same as Pods. In addition, it prints the
+		}
-// deletion timestamps of the pods if they are not nil.
-func PodsWithDeletionTimestamps(pods []*v1.Pod) string {
-	return aggregatePods(pods, PodWithDeletionTimestamp)
-}
-
-func aggregatePods(pods []*v1.Pod, handler podHandler) string {
-	podStrings := make([]string, 0, len(pods))
-	for _, pod := range pods {
-		podStrings = append(podStrings, handler(pod))
 	}
-	return strings.Join(podStrings, ", ")
+	return podKObjs
 }

pkg/kubelet/util/format/pod_test.go

@@ -22,9 +22,10 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/klog/v2"
 )
 
 func fakeCreatePod(name, namespace string, uid types.UID) *v1.Pod {
@@ -117,16 +118,24 @@ func TestPods(t *testing.T) {
 	pod1 := fakeCreatePod("pod1", metav1.NamespaceDefault, "551f5a43-9f2f-11e7-a589-fa163e148d75")
 	pod2 := fakeCreatePod("pod2", metav1.NamespaceDefault, "e84a99bf-d1f9-43c2-9fa5-044ac85f794b")
 
+	pod1Obj := klog.ObjectRef{
+		Name:      "pod1",
+		Namespace: metav1.NamespaceDefault,
+	}
+	pod2Obj := klog.ObjectRef{
+		Name:      "pod2",
+		Namespace: metav1.NamespaceDefault,
+	}
 	testCases := []struct {
 		caseName      string
 		pods          []*v1.Pod
-		expectedValue string
+		expectedValue []klog.ObjectRef
 	}{
-		{"input_nil_case", nil, ""},
+		{"input_nil_case", nil, []klog.ObjectRef{}},
-		{"input_empty_case", []*v1.Pod{}, ""},
+		{"input_empty_case", []*v1.Pod{}, []klog.ObjectRef{}},
-		{"input_length_one_case", []*v1.Pod{pod1}, "pod1_default(551f5a43-9f2f-11e7-a589-fa163e148d75)"},
+		{"input_length_one_case", []*v1.Pod{pod1, nil}, []klog.ObjectRef{pod1Obj}},
-		{"input_length_more_than_one_case", []*v1.Pod{pod1, pod2}, "pod1_default(551f5a43-9f2f-11e7-a589-fa163e148d75), pod2_default(e84a99bf-d1f9-43c2-9fa5-044ac85f794b)"},
+		{"input_length_more_than_one_case", []*v1.Pod{pod1, pod2}, []klog.ObjectRef{pod1Obj, pod2Obj}},
-		{"input_include_nil_case", []*v1.Pod{pod1, nil}, "pod1_default(551f5a43-9f2f-11e7-a589-fa163e148d75), <nil>"},
+		{"input_include_nil_case", []*v1.Pod{pod1, nil}, []klog.ObjectRef{pod1Obj}},
 	}
 
 	for _, testCase := range testCases {
@@ -134,26 +143,3 @@ func TestPods(t *testing.T) {
 		assert.Equalf(t, testCase.expectedValue, realPods, "Failed to test: %s", testCase.caseName)
 	}
 }
-
-func TestPodsWithDeletionTimestamps(t *testing.T) {
-	normalDeletionTime := metav1.Date(2017, time.September, 26, 14, 37, 50, 00, time.UTC)
-	pod1 := fakeCreatePodWithDeletionTimestamp("pod1", metav1.NamespaceDefault, "551f5a43-9f2f-11e7-a589-fa163e148d75", &normalDeletionTime)
-	pod2 := fakeCreatePodWithDeletionTimestamp("pod2", metav1.NamespaceDefault, "e84a99bf-d1f9-43c2-9fa5-044ac85f794b", &normalDeletionTime)
-
-	testCases := []struct {
-		caseName      string
-		pods          []*v1.Pod
-		expectedValue string
-	}{
-		{"input_nil_case", nil, ""},
-		{"input_empty_case", []*v1.Pod{}, ""},
-		{"input_length_one_case", []*v1.Pod{pod1}, "pod1_default(551f5a43-9f2f-11e7-a589-fa163e148d75):DeletionTimestamp=2017-09-26T14:37:50Z"},
-		{"input_length_more_than_one_case", []*v1.Pod{pod1, pod2}, "pod1_default(551f5a43-9f2f-11e7-a589-fa163e148d75):DeletionTimestamp=2017-09-26T14:37:50Z, pod2_default(e84a99bf-d1f9-43c2-9fa5-044ac85f794b):DeletionTimestamp=2017-09-26T14:37:50Z"},
-		{"input_include_nil_case", []*v1.Pod{pod1, nil}, "pod1_default(551f5a43-9f2f-11e7-a589-fa163e148d75):DeletionTimestamp=2017-09-26T14:37:50Z, <nil>"},
-	}
-
-	for _, testCase := range testCases {
-		realPods := PodsWithDeletionTimestamps(testCase.pods)
-		assert.Equalf(t, testCase.expectedValue, realPods, "Failed to test: %s", testCase.caseName)
-	}
-}