nrt: logr: add support for logr

Move to the logr.Logger interface everywhere, instead of the global `klog` instance. This enable named logger, presetting values for simple and automatic consistency, enables pluggable loggers and comes for free since we already depend on the logr package and klog has a native logr integration. In addition, add minimal support to make it easy to replace the logr reference, to help integrators of this code. The default is still (and will still be) klog for backward compatibility and ecosystem integration. Signed-off-by: Francesco Romani <fromani@redhat.com>
kubernetes-sigs · Apr 19, 2024 · 045a43d · 045a43d
1 parent 8d9a4cd
commit 045a43d
Show file tree

Hide file tree

Showing 28 changed files with 412 additions and 306 deletions.
diff --git a/pkg/noderesourcetopology/cache/discardreserved.go b/pkg/noderesourcetopology/cache/discardreserved.go
@@ -20,13 +20,15 @@ import (
 	"context"
 	"sync"
 
+	"github.com/go-logr/logr"
 	topologyv1alpha2 "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
 
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/scheduler-plugins/pkg/noderesourcetopology/logging"
 )
 
 // DiscardReserved is intended to solve similiar problem as Overreserve Cache,
@@ -45,12 +47,14 @@ type DiscardReserved struct {
 	rMutex         sync.RWMutex
 	reservationMap map[string]map[types.UID]bool // Key is NodeName, value is Pod UID : reserved status
 	client         ctrlclient.Client
+	lh             logr.Logger
 }
 
-func NewDiscardReserved(client ctrlclient.Client) Interface {
+func NewDiscardReserved(lh logr.Logger, client ctrlclient.Client) Interface {
 	return &DiscardReserved{
 		client:         client,
 		reservationMap: make(map[string]map[types.UID]bool),
+		lh:             lh,
 	}
 }
 
@@ -74,7 +78,7 @@ func (pt *DiscardReserved) NodeMaybeOverReserved(nodeName string, pod *corev1.Po
 func (pt *DiscardReserved) NodeHasForeignPods(nodeName string, pod *corev1.Pod)    {}
 
 func (pt *DiscardReserved) ReserveNodeResources(nodeName string, pod *corev1.Pod) {
-	klog.V(5).InfoS("nrtcache NRT Reserve", "logID", klog.KObj(pod), "UID", pod.GetUID(), "node", nodeName)
+	pt.lh.V(5).Info("NRT Reserve", "logID", logging.PodLogID(pod), "podUID", pod.GetUID(), "node", nodeName)
 	pt.rMutex.Lock()
 	defer pt.rMutex.Unlock()
 
@@ -85,14 +89,14 @@ func (pt *DiscardReserved) ReserveNodeResources(nodeName string, pod *corev1.Pod
 }
 
 func (pt *DiscardReserved) UnreserveNodeResources(nodeName string, pod *corev1.Pod) {
-	klog.V(5).InfoS("nrtcache NRT Unreserve", "logID", klog.KObj(pod), "UID", pod.GetUID(), "node", nodeName)
+	pt.lh.V(5).Info("NRT Unreserve", "logID", klog.KObj(pod), "podUID", pod.GetUID(), "node", nodeName)
 
 	pt.removeReservationForNode(nodeName, pod)
 }
 
 // PostBind is invoked to cleanup reservationMap
 func (pt *DiscardReserved) PostBind(nodeName string, pod *corev1.Pod) {
-	klog.V(5).InfoS("nrtcache NRT PostBind", "logID", klog.KObj(pod), "UID", pod.GetUID(), "node", nodeName)
+	pt.lh.V(5).Info("NRT PostBind", "logID", klog.KObj(pod), "podUID", pod.GetUID(), "node", nodeName)
 
 	pt.removeReservationForNode(nodeName, pod)
 }

diff --git a/pkg/noderesourcetopology/cache/discardreserved_test.go b/pkg/noderesourcetopology/cache/discardreserved_test.go
@@ -26,6 +26,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	podlisterv1 "k8s.io/client-go/listers/core/v1"
+	"k8s.io/klog/v2"
 
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -50,7 +51,7 @@ func TestDiscardReservedNodesGetCachedNRTCopy(t *testing.T) {
 	checkGetCachedNRTCopy(
 		t,
 		func(client ctrlclient.Client, _ podlisterv1.PodLister) (Interface, error) {
-			return NewDiscardReserved(client), nil
+			return NewDiscardReserved(klog.Background(), client), nil
 		},
 		testCases...,
 	)

diff --git a/pkg/noderesourcetopology/cache/foreign_pods.go b/pkg/noderesourcetopology/cache/foreign_pods.go
@@ -17,11 +17,14 @@ limitations under the License.
 package cache
 
 import (
+	"fmt"
+
+	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	k8scache "k8s.io/client-go/tools/cache"
-	"k8s.io/klog/v2"
 
+	"sigs.k8s.io/scheduler-plugins/pkg/noderesourcetopology/logging"
 	"sigs.k8s.io/scheduler-plugins/pkg/noderesourcetopology/resourcerequests"
 )
 
@@ -35,19 +38,19 @@ var (
 	onlyExclusiveResources = false
 )
 
-func SetupForeignPodsDetector(schedProfileName string, podInformer k8scache.SharedInformer, cc Interface) {
+func SetupForeignPodsDetector(lh logr.Logger, schedProfileName string, podInformer k8scache.SharedInformer, cc Interface) {
 	foreignCache := func(obj interface{}) {
 		pod, ok := obj.(*corev1.Pod)
 		if !ok {
-			klog.V(3).InfoS("nrtcache: foreign: unsupported object %T", obj)
+			lh.V(3).Info("unsupported object", "kind", fmt.Sprintf("%T", obj))
 			return
 		}
 		if !IsForeignPod(pod) {
 			return
 		}
 
 		cc.NodeHasForeignPods(pod.Spec.NodeName, pod)
-		klog.V(6).InfoS("nrtcache: has foreign pods", "logID", klog.KObj(pod), "node", pod.Spec.NodeName, "podUID", pod.UID)
+		lh.V(6).Info("detected foreign pods", "logID", logging.PodLogID(pod), "podUID", pod.GetUID(), "node", pod.Spec.NodeName)
 	}
 
 	podInformer.AddEventHandler(k8scache.ResourceEventHandlerFuncs{
@@ -67,11 +70,11 @@ func TrackAllForeignPods() {
 	onlyExclusiveResources = false
 }
 
-func RegisterSchedulerProfileName(schedProfileName string) {
-	klog.InfoS("nrtcache: setting up foreign pod detection", "profile", schedProfileName)
+func RegisterSchedulerProfileName(lh logr.Logger, schedProfileName string) {
+	lh.Info("setting up detection", "profile", schedProfileName)
 	schedProfileNames.Insert(schedProfileName)
 
-	klog.V(5).InfoS("nrtcache: registered scheduler profiles", "names", schedProfileNames.List())
+	lh.V(5).Info("registered scheduler profiles", "names", schedProfileNames.List())
 }
 
 func IsForeignPod(pod *corev1.Pod) bool {

diff --git a/pkg/noderesourcetopology/cache/foreign_pods_test.go b/pkg/noderesourcetopology/cache/foreign_pods_test.go
@@ -22,6 +22,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/klog/v2"
 )
 
 func TestIsForeignPod(t *testing.T) {
@@ -195,7 +196,7 @@ func TestIsForeignPod(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			for _, profileName := range tt.profileNames {
-				RegisterSchedulerProfileName(profileName)
+				RegisterSchedulerProfileName(klog.Background(), profileName)
 			}
 			defer CleanRegisteredSchedulerProfileNames()