Repository: kubernetes-sigs/node-feature-discovery
Pull request #1567 (2024-01-25 15:24:47)

topology-updater: ditch apihelper
Stop using pkg/apihelper for accessing the Kubernetes API. Modify unit tests to use the fake kubernetes client instead of mocked apihelper interface.

Part of #1561 

marquiz: /assign @PiotrProkop 
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/node-feature-discovery/pull/1567#" title="Author self-approved">marquiz</a>*

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

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-sigs/node-feature-discovery/blob/master/OWNERS)~~ [marquiz]

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":[]} -->
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) Report
Attention: `5 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`5d30be1`)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/commit/5d30be101386f748edd12a00c52d1bc0a8361a65?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) 31.07% compared to head [(`92f8407`)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) 31.03%.

<details><summary>Additional details and impacted files</summary>


[![Impacted file tree graph](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567/graphs/tree.svg?width=650&height=150&src=pr&token=mEZgtJQ1vg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

```diff
@@            Coverage Diff             @@
##           master    #1567      +/-   ##
==========================================
- Coverage   31.07%   31.03%   -0.04%     
==========================================
  Files          61       61              
  Lines        7785     7784       -1     
==========================================
- Hits         2419     2416       -3     
- Misses       5120     5124       +4     
+ Partials      246      244       -2     
```


| [Files](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs) | Coverage Δ | |
|---|---|---|
| [pkg/resourcemonitor/podresourcesscanner.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL3Jlc291cmNlbW9uaXRvci9wb2RyZXNvdXJjZXNzY2FubmVyLmdv) | `82.71% <100.00%> (+1.39%)` | :arrow_up: |
| [pkg/nfd-topology-updater/nfd-topology-updater.go](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs#diff-cGtnL25mZC10b3BvbG9neS11cGRhdGVyL25mZC10b3BvbG9neS11cGRhdGVyLmdv) | `3.05% <0.00%> (-0.05%)` | :arrow_down: |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/kubernetes-sigs/node-feature-discovery/pull/1567/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubernetes-sigs)

</details>

Patch:
diff --git a/pkg/nfd-topology-updater/nfd-topology-updater.go b/pkg/nfd-topology-updater/nfd-topology-updater.go
index aac194d8f..6889f8eb8 100644
--- a/pkg/nfd-topology-updater/nfd-topology-updater.go
+++ b/pkg/nfd-topology-updater/nfd-topology-updater.go
@@ -26,12 +26,12 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	k8sclient "k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 	kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1"
 
 	"github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2"
 	topologyclientset "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/generated/clientset/versioned"
-	"sigs.k8s.io/node-feature-discovery/pkg/apihelper"
 	"sigs.k8s.io/node-feature-discovery/pkg/nfd-topology-updater/kubeletnotifier"
 	"sigs.k8s.io/node-feature-discovery/pkg/podres"
 	"sigs.k8s.io/node-feature-discovery/pkg/resourcemonitor"
@@ -74,7 +74,6 @@ type NfdTopologyUpdater interface {
 type nfdTopologyUpdater struct {
 	nodeName            string
 	args                Args
-	apihelper           apihelper.APIHelpers
 	topoClient          topologyclientset.Interface
 	resourcemonitorArgs resourcemonitor.Args
 	stop                chan struct{} // channel for signaling stop
@@ -137,13 +136,17 @@ func (w *nfdTopologyUpdater) Run() error {
 	if err != nil {
 		return err
 	}
-	w.apihelper = apihelper.K8sHelpers{Kubeconfig: kubeconfig}
 	topoClient, err := topologyclientset.NewForConfig(kubeconfig)
 	if err != nil {
 		return nil
 	}
 	w.topoClient = topoClient
 
+	k8sClient, err := k8sclient.NewForConfig(kubeconfig)
+	if err != nil {
+		return err
+	}
+
 	if err := w.configure(); err != nil {
 		return fmt.Errorf("faild to configure Node Feature Discovery Topology Updater: %w", err)
 	}
@@ -160,7 +163,7 @@ func (w *nfdTopologyUpdater) Run() error {
 
 	var resScan resourcemonitor.ResourcesScanner
 
-	resScan, err = resourcemonitor.NewPodResourcesScanner(w.resourcemonitorArgs.Namespace, podResClient, w.apihelper, w.resourcemonitorArgs.PodSetFingerprint)
+	resScan, err = resourcemonitor.NewPodResourcesScanner(w.resourcemonitorArgs.Namespace, podResClient, k8sClient, w.resourcemonitorArgs.PodSetFingerprint)
 	if err != nil {
 		return fmt.Errorf("failed to initialize ResourceMonitor instance: %w", err)
 	}
diff --git a/pkg/resourcemonitor/podresourcesscanner.go b/pkg/resourcemonitor/podresourcesscanner.go
index 37ffb1dd7..05d730640 100644
--- a/pkg/resourcemonitor/podresourcesscanner.go
+++ b/pkg/resourcemonitor/podresourcesscanner.go
@@ -23,11 +23,11 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	client "k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 	podresourcesapi "k8s.io/kubelet/pkg/apis/podresources/v1"
 
-	"sigs.k8s.io/node-feature-discovery/pkg/apihelper"
-
 	"github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2"
 	"github.com/k8stopologyawareschedwg/podfingerprint"
 )
@@ -35,16 +35,16 @@ import (
 type PodResourcesScanner struct {
 	namespace         string
 	podResourceClient podresourcesapi.PodResourcesListerClient
-	apihelper         apihelper.APIHelpers
+	k8sClient         client.Interface
 	podFingerprint    bool
 }
 
 // NewPodResourcesScanner creates a new ResourcesScanner instance
-func NewPodResourcesScanner(namespace string, podResourceClient podresourcesapi.PodResourcesListerClient, kubeApihelper apihelper.APIHelpers, podFingerprint bool) (ResourcesScanner, error) {
+func NewPodResourcesScanner(namespace string, podResourceClient podresourcesapi.PodResourcesListerClient, k8sClient client.Interface, podFingerprint bool) (ResourcesScanner, error) {
 	resourcemonitorInstance := &PodResourcesScanner{
 		namespace:         namespace,
 		podResourceClient: podResourceClient,
-		apihelper:         kubeApihelper,
+		k8sClient:         k8sClient,
 		podFingerprint:    podFingerprint,
 	}
 	if resourcemonitorInstance.namespace != "*" {
@@ -58,11 +58,7 @@ func NewPodResourcesScanner(namespace string, podResourceClient podresourcesapi.
 
 // isWatchable tells if the the given namespace should be watched.
 func (resMon *PodResourcesScanner) isWatchable(podNamespace string, podName string, hasDevice bool) (bool, bool, error) {
-	cli, err := resMon.apihelper.GetClient()
-	if err != nil {
-		return false, false, err
-	}
-	pod, err := resMon.apihelper.GetPod(cli, podNamespace, podName)
+	pod, err := resMon.k8sClient.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{})
 	if err != nil {
 		return false, false, err
 	}


Test patch:
diff --git a/pkg/resourcemonitor/podresourcesscanner_test.go b/pkg/resourcemonitor/podresourcesscanner_test.go
index 7bb473613..ee9dfd84e 100644
--- a/pkg/resourcemonitor/podresourcesscanner_test.go
+++ b/pkg/resourcemonitor/podresourcesscanner_test.go
@@ -25,22 +25,16 @@ import (
 	"github.com/k8stopologyawareschedwg/podfingerprint"
 	. "github.com/smartystreets/goconvey/convey"
 	"github.com/stretchr/testify/mock"
-
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	k8sclient "k8s.io/client-go/kubernetes"
+	fakeclient "k8s.io/client-go/kubernetes/fake"
 	v1 "k8s.io/kubelet/pkg/apis/podresources/v1"
 
-	"sigs.k8s.io/node-feature-discovery/pkg/apihelper"
-	mockv1 "sigs.k8s.io/node-feature-discovery/pkg/podres/mocks"
+	mockpodres "sigs.k8s.io/node-feature-discovery/pkg/podres/mocks"
 )
 
 func TestPodScanner(t *testing.T) {
-
-	var resScan ResourcesScanner
-	var err error
-
 	// PodFingerprint only depends on Name/Namespace of the pods running on a Node
 	// so we can precalculate the expected value
 	expectedFingerprintCompute := func(pods []*corev1.Pod) (string, error) {
@@ -54,11 +48,11 @@ func TestPodScanner(t *testing.T) {
 	}
 
 	Convey("When I scan for pod resources using fake client and no namespace", t, func() {
-		mockPodResClient := new(mockv1.PodResourcesListerClient)
-		mockAPIHelper := new(apihelper.MockAPIHelpers)
-		mockClient := &k8sclient.Clientset{}
+		mockPodResClient := new(mockpodres.PodResourcesListerClient)
+
+		fakeCli := fakeclient.NewSimpleClientset()
 		computePodFingerprint := true
-		resScan, err = NewPodResourcesScanner("*", mockPodResClient, mockAPIHelper, computePodFingerprint)
+		resScan, err := NewPodResourcesScanner("*", mockPodResClient, fakeCli, computePodFingerprint)
 
 		Convey("Creating a Resources Scanner using a mock client", func() {
 			So(err, ShouldBeNil)
@@ -172,8 +166,9 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+
+			fakeCli := fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -286,8 +281,9 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -373,8 +369,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -463,8 +459,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -541,8 +537,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -633,8 +629,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -676,11 +672,10 @@ func TestPodScanner(t *testing.T) {
 	})
 
 	Convey("When I scan for pod resources using fake client and given namespace", t, func() {
-		mockPodResClient := new(mockv1.PodResourcesListerClient)
-		mockAPIHelper := new(apihelper.MockAPIHelpers)
-		mockClient := &k8sclient.Clientset{}
+		mockPodResClient := new(mockpodres.PodResourcesListerClient)
+		fakeCli := fakeclient.NewSimpleClientset()
 		computePodFingerprint := false
-		resScan, err = NewPodResourcesScanner("pod-res-test", mockPodResClient, mockAPIHelper, computePodFingerprint)
+		resScan, err := NewPodResourcesScanner("pod-res-test", mockPodResClient, fakeCli, computePodFingerprint)
 
 		Convey("Creating a Resources Scanner using a mock client", func() {
 			So(err, ShouldBeNil)
@@ -752,12 +747,12 @@ func TestPodScanner(t *testing.T) {
 							Name: "test-cnt-0",
 							Resources: corev1.ResourceRequirements{
 								Requests: corev1.ResourceList{
-									corev1.ResourceCPU:                      *resource.NewQuantity(2, resource.DecimalSI),
+									corev1.ResourceCPU:                      *resource.NewQuantity(1, resource.DecimalSI),
 									corev1.ResourceName("fake.io/resource"): *resource.NewQuantity(1, resource.DecimalSI),
 									corev1.ResourceMemory:                   *resource.NewQuantity(100, resource.DecimalSI),
 								},
 								Limits: corev1.ResourceList{
-									corev1.ResourceCPU:                      *resource.NewQuantity(2, resource.DecimalSI),
+									corev1.ResourceCPU:                      *resource.NewQuantity(1, resource.DecimalSI),
 									corev1.ResourceName("fake.io/resource"): *resource.NewQuantity(1, resource.DecimalSI),
 									corev1.ResourceMemory:                   *resource.NewQuantity(100, resource.DecimalSI),
 								},
@@ -766,8 +761,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -831,8 +826,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "pod-res-test", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -911,8 +906,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -977,8 +972,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "default", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -1035,8 +1030,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "pod-res-test", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {
@@ -1119,8 +1114,8 @@ func TestPodScanner(t *testing.T) {
 					},
 				},
 			}
-			mockAPIHelper.On("GetClient").Return(mockClient, nil)
-			mockAPIHelper.On("GetPod", mock.AnythingOfType("*kubernetes.Clientset"), "pod-res-test", "test-pod-0").Return(pod, nil)
+			fakeCli = fakeclient.NewSimpleClientset(pod)
+			resScan.(*PodResourcesScanner).k8sClient = fakeCli
 			res, err := resScan.Scan()
 
 			Convey("Error is nil", func() {