Repository: kubernetes-sigs/node-feature-discovery
Pull request #952 (2022-11-06 20:40:35)

Add argument to updateNodeFeatures method to pass client from caller
This commit adds an argument to updateNodeFeatures method for receiving
client argument, which currently gets initialized within the method itself. This
is a minor improvement originally for https://github.com/kubernetes-sigs/node-feature-discovery/pull/910.

Ref:https://github.com/kubernetes-sigs/node-feature-discovery/pull/910#discussion_r1012703631


Patch:
diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go
index ff419e6e6..5111d5773 100644
--- a/pkg/nfd-master/nfd-master.go
+++ b/pkg/nfd-master/nfd-master.go
@@ -40,6 +40,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/klog/v2"
 
@@ -288,7 +289,7 @@ func (m *nfdMaster) prune() error {
 		klog.Infof("pruning node %q...", node.Name)
 
 		// Prune labels and extended resources
-		err := m.updateNodeFeatures(node.Name, Labels{}, Annotations{}, ExtendedResources{})
+		err := m.updateNodeFeatures(cli, node.Name, Labels{}, Annotations{}, ExtendedResources{})
 		if err != nil {
 			return fmt.Errorf("failed to prune labels from node %q: %v", node.Name, err)
 		}
@@ -398,10 +399,12 @@ func verifyNodeName(cert *x509.Certificate, nodeName string) error {
 
 // SetLabels implements LabelerServer
 func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.SetLabelsReply, error) {
+
 	err := authorizeClient(c, m.args.VerifyNodeName, r.NodeName)
 	if err != nil {
 		return &pb.SetLabelsReply{}, err
 	}
+
 	switch {
 	case klog.V(4).Enabled():
 		utils.KlogDump(3, "REQUEST", "  ", r)
@@ -424,10 +427,15 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se
 	labels, extendedResources := filterFeatureLabels(rawLabels, m.args.ExtraLabelNs, m.args.LabelWhiteList.Regexp, m.args.ResourceLabels)
 
 	if !m.args.NoPublish {
+		cli, err := m.apihelper.GetClient()
+		if err != nil {
+			return &pb.SetLabelsReply{}, err
+		}
+
 		// Advertise NFD worker version as an annotation
 		annotations := Annotations{m.instanceAnnotation(nfdv1alpha1.WorkerVersionAnnotation): r.NfdVersion}
 
-		err := m.updateNodeFeatures(r.NodeName, labels, annotations, extendedResources)
+		err = m.updateNodeFeatures(cli, r.NodeName, labels, annotations, extendedResources)
 		if err != nil {
 			klog.Errorf("failed to advertise labels: %v", err)
 			return &pb.SetLabelsReply{}, err
@@ -536,10 +544,9 @@ func (m *nfdMaster) crLabels(r *pb.SetLabelsRequest) map[string]string {
 // updateNodeFeatures ensures the Kubernetes node object is up to date,
 // creating new labels and extended resources where necessary and removing
 // outdated ones. Also updates the corresponding annotations.
-func (m *nfdMaster) updateNodeFeatures(nodeName string, labels Labels, annotations Annotations, extendedResources ExtendedResources) error {
-	cli, err := m.apihelper.GetClient()
-	if err != nil {
-		return err
+func (m *nfdMaster) updateNodeFeatures(cli *kubernetes.Clientset, nodeName string, labels Labels, annotations Annotations, extendedResources ExtendedResources) error {
+	if cli == nil {
+		return fmt.Errorf("no client is passed, client:  %v", cli)
 	}
 
 	// Get the worker node object


Test patch:
diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go
index eae019cff..fc522daec 100644
--- a/pkg/nfd-master/nfd-master-internal_test.go
+++ b/pkg/nfd-master/nfd-master-internal_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package nfdmaster
 
 import (
+	"fmt"
 	"regexp"
 	"sort"
 	"strings"
@@ -114,7 +115,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Once()
 			mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil)
 			mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil)
-			err := mockMaster.updateNodeFeatures(mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
+			err := mockMaster.updateNodeFeatures(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
@@ -122,22 +123,22 @@ func TestUpdateNodeFeatures(t *testing.T) {
 		})
 
 		Convey("When I fail to update the node with feature labels", func() {
-			expectedError := errors.New("fake error")
+			expectedError := fmt.Errorf("no client is passed, client:  <nil>")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
-			err := mockMaster.updateNodeFeatures(mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
+			err := mockMaster.updateNodeFeatures(nil, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
-				So(err, ShouldEqual, expectedError)
+				So(err, ShouldResemble, expectedError)
 			})
 		})
 
 		Convey("When I fail to get a mock client while updating feature labels", func() {
-			expectedError := errors.New("fake error")
+			expectedError := fmt.Errorf("no client is passed, client:  <nil>")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
-			err := mockMaster.updateNodeFeatures(mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
+			err := mockMaster.updateNodeFeatures(nil, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
-				So(err, ShouldEqual, expectedError)
+				So(err, ShouldResemble, expectedError)
 			})
 		})
 
@@ -145,7 +146,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(nil, expectedError).Once()
-			err := mockMaster.updateNodeFeatures(mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
+			err := mockMaster.updateNodeFeatures(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldEqual, expectedError)
@@ -157,7 +158,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Once()
 			mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(expectedError).Once()
-			err := mockMaster.updateNodeFeatures(mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
+			err := mockMaster.updateNodeFeatures(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
 				So(err.Error(), ShouldEndWith, expectedError.Error())