Merge pull request #1652 from marquiz/devel/reuse-node

nfd-master: get node object only once when updating node

pkg/nfd-master/nfd-master-internal_test.go

@@ -157,7 +157,7 @@ func TestUpdateNodeObject(t *testing.T) {
 		fakeMaster := newFakeMaster(fakeCli)
 
 		Convey("When I successfully update the node with feature labels", func() {
-			err := fakeMaster.updateNodeObject(testNodeName, featureLabels, featureAnnotations, featureExtResources, nil)
+			err := fakeMaster.updateNodeObject(testNode, featureLabels, featureAnnotations, featureExtResources, nil)
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
 			})
@@ -185,19 +185,11 @@ func TestUpdateNodeObject(t *testing.T) {
 			})
 		})
 
-		Convey("When I fail to get a node while updating feature labels", func() {
-			err := fakeMaster.updateNodeObject("non-existent-node", featureLabels, featureAnnotations, featureExtResources, nil)
-
-			Convey("Error is produced", func() {
-				So(err, ShouldBeError)
-			})
-		})
-
 		Convey("When I fail to patch a node", func() {
 			fakeCli.CoreV1().(*fakecorev1client.FakeCoreV1).PrependReactor("patch", "nodes", func(action clienttesting.Action) (handled bool, ret runtime.Object, err error) {
 				return true, &v1.Node{}, errors.New("Fake error when patching node")
 			})
-			err := fakeMaster.updateNodeObject(testNodeName, nil, featureAnnotations, ExtendedResources{"": ""}, nil)
+			err := fakeMaster.updateNodeObject(testNode, nil, featureAnnotations, ExtendedResources{"": ""}, nil)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldBeError)

pkg/nfd-master/nfd-master.go

@@ -503,7 +503,7 @@ func (m *nfdMaster) prune() error {
 		klog.InfoS("pruning node...", "nodeName", node.Name)
 
 		// Prune labels and extended resources
-		err := m.updateNodeObject(node.Name, Labels{}, Annotations{}, ExtendedResources{}, []corev1.Taint{})
+		err := m.updateNodeObject(&node, Labels{}, Annotations{}, ExtendedResources{}, []corev1.Taint{})
 		if err != nil {
 			nodeUpdateFailures.Inc()
 			return fmt.Errorf("failed to prune node %q: %v", node.Name, err)
@@ -692,8 +692,13 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se
 		klog.InfoS("gRPC SetLabels request received", "nodeName", r.NodeName)
 	}
 	if !m.config.NoPublish {
+		// Fetch the node object.
+		node, err := m.getNode(r.NodeName)
+		if err != nil {
+			return &pb.SetLabelsReply{}, err
+		}
 		// Create labels et al
-		if err := m.refreshNodeFeatures(r.NodeName, r.GetLabels(), r.GetFeatures()); err != nil {
+		if err := m.refreshNodeFeatures(node, r.GetLabels(), r.GetFeatures()); err != nil {
 			nodeUpdateFailures.Inc()
 			return &pb.SetLabelsReply{}, err
 		}
@@ -716,15 +721,15 @@ func (m *nfdMaster) nfdAPIUpdateAllNodes() error {
 	return nil
 }
 
-func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
+func (m *nfdMaster) nfdAPIUpdateOneNode(node *corev1.Node) error {
 	if m.nfdController == nil || m.nfdController.featureLister == nil {
 		return nil
 	}
 
-	sel := k8sLabels.SelectorFromSet(k8sLabels.Set{nfdv1alpha1.NodeFeatureObjNodeNameLabel: nodeName})
+	sel := k8sLabels.SelectorFromSet(k8sLabels.Set{nfdv1alpha1.NodeFeatureObjNodeNameLabel: node.Name})
 	objs, err := m.nfdController.featureLister.List(sel)
 	if err != nil {
-		return fmt.Errorf("failed to get NodeFeature resources for node %q: %w", nodeName, err)
+		return fmt.Errorf("failed to get NodeFeature resources for node %q: %w", node.Name, err)
 	}
 
 	// Sort our objects
@@ -748,7 +753,7 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
 		return nil
 	}
 
-	klog.V(1).InfoS("processing of node initiated by NodeFeature API", "nodeName", nodeName)
+	klog.V(1).InfoS("processing of node initiated by NodeFeature API", "nodeName", node.Name)
 
 	features := nfdv1alpha1.NewNodeFeatureSpec()
 
@@ -775,7 +780,7 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
 	// Update node labels et al. This may also mean removing all NFD-owned
 	// labels (et al.), for example  in the case no NodeFeature objects are
 	// present.
-	if err := m.refreshNodeFeatures(nodeName, features.Labels, &features.Features); err != nil {
+	if err := m.refreshNodeFeatures(node, features.Labels, &features.Features); err != nil {
 		return err
 	}
 
@@ -820,14 +825,14 @@ func filterExtendedResource(name, value string, features *nfdv1alpha1.Features)
 	return filteredValue, nil
 }
 
-func (m *nfdMaster) refreshNodeFeatures(nodeName string, labels map[string]string, features *nfdv1alpha1.Features) error {
+func (m *nfdMaster) refreshNodeFeatures(node *corev1.Node, labels map[string]string, features *nfdv1alpha1.Features) error {
 	if m.config.AutoDefaultNs {
 		labels = addNsToMapKeys(labels, nfdv1alpha1.FeatureLabelNs)
 	} else if labels == nil {
 		labels = make(map[string]string)
 	}
 
-	crLabels, crAnnotations, crExtendedResources, crTaints := m.processNodeFeatureRule(nodeName, features)
+	crLabels, crAnnotations, crExtendedResources, crTaints := m.processNodeFeatureRule(node.Name, features)
 
 	// Mix in CR-originated labels
 	maps.Copy(labels, crLabels)
@@ -849,9 +854,9 @@ func (m *nfdMaster) refreshNodeFeatures(nodeName string, labels map[string]strin
 		taints = filterTaints(crTaints)
 	}
 
-	err := m.updateNodeObject(nodeName, labels, annotations, extendedResources, taints)
+	err := m.updateNodeObject(node, labels, annotations, extendedResources, taints)
 	if err != nil {
-		klog.ErrorS(err, "failed to update node", "nodeName", nodeName)
+		klog.ErrorS(err, "failed to update node", "nodeName", node.Name)
 		return err
 	}
 
@@ -861,14 +866,9 @@ func (m *nfdMaster) refreshNodeFeatures(nodeName string, labels map[string]strin
 // setTaints sets node taints and annotations based on the taints passed via
 // nodeFeatureRule custom resorce. If empty list of taints is passed, currently
 // NFD owned taints and annotations are removed from the node.
-func (m *nfdMaster) setTaints(taints []corev1.Taint, nodeName string) error {
-	// Fetch the node object.
-	node, err := m.getNode(nodeName)
-	if err != nil {
-		return err
-	}
-
+func (m *nfdMaster) setTaints(taints []corev1.Taint, node *corev1.Node) error {
 	// De-serialize the taints annotation into corev1.Taint type for comparision below.
+	var err error
 	oldTaints := []corev1.Taint{}
 	if val, ok := node.Annotations[nfdv1alpha1.NodeTaintsAnnotation]; ok {
 		sts := strings.Split(val, ",")
@@ -905,11 +905,10 @@ func (m *nfdMaster) setTaints(taints []corev1.Taint, nodeName string) error {
 	}
 
 	if taintsUpdated {
-		err = controller.PatchNodeTaints(context.TODO(), m.k8sClient, nodeName, node, newNode)
-		if err != nil {
+		if err := controller.PatchNodeTaints(context.TODO(), m.k8sClient, node.Name, node, newNode); err != nil {
 			return fmt.Errorf("failed to patch the node %v", node.Name)
 		}
-		klog.InfoS("updated node taints", "nodeName", nodeName)
+		klog.InfoS("updated node taints", "nodeName", node.Name)
 	}
 
 	// Update node annotation that holds the taints managed by us
@@ -926,11 +925,10 @@ func (m *nfdMaster) setTaints(taints []corev1.Taint, nodeName string) error {
 
 	patches := createPatches([]string{nfdv1alpha1.NodeTaintsAnnotation}, node.Annotations, newAnnotations, "/metadata/annotations")
 	if len(patches) > 0 {
-		err = m.patchNode(node.Name, patches)
-		if err != nil {
+		if err := m.patchNode(node.Name, patches); err != nil {
 			return fmt.Errorf("error while patching node object: %w", err)
 		}
-		klog.V(1).InfoS("patched node annotations for taints", "nodeName", nodeName)
+		klog.V(1).InfoS("patched node annotations for taints", "nodeName", node.Name)
 	}
 	return nil
 }
@@ -1024,13 +1022,7 @@ func (m *nfdMaster) processNodeFeatureRule(nodeName string, features *nfdv1alpha
 // updateNodeObject 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) updateNodeObject(nodeName string, labels Labels, featureAnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error {
-	// Get the worker node object
-	node, err := m.getNode(nodeName)
-	if err != nil {
-		return err
-	}
-
+func (m *nfdMaster) updateNodeObject(node *corev1.Node, labels Labels, featureAnnotations Annotations, extendedResources ExtendedResources, taints []corev1.Taint) error {
 	annotations := make(Annotations)
 
 	// Store names of labels in an annotation
@@ -1083,7 +1075,7 @@ func (m *nfdMaster) updateNodeObject(nodeName string, labels Labels, featureAnno
 
 	// patch node status with extended resource changes
 	statusPatches := m.createExtendedResourcePatches(node, extendedResources)
-	err = m.patchNodeStatus(node.Name, statusPatches)
+	err := m.patchNodeStatus(node.Name, statusPatches)
 	if err != nil {
 		return fmt.Errorf("error while patching extended resources: %w", err)
 	}
@@ -1096,13 +1088,13 @@ func (m *nfdMaster) updateNodeObject(nodeName string, labels Labels, featureAnno
 
 	if len(patches) > 0 || len(statusPatches) > 0 {
 		nodeUpdates.Inc()
-		klog.InfoS("node updated", "nodeName", nodeName)
+		klog.InfoS("node updated", "nodeName", node.Name)
 	} else {
-		klog.V(1).InfoS("no updates to node", "nodeName", nodeName)
+		klog.V(1).InfoS("no updates to node", "nodeName", node.Name)
 	}
 
 	// Set taints
-	err = m.setTaints(taints, node.Name)
+	err = m.setTaints(taints, node)
 	if err != nil {
 		return err
 	}

pkg/nfd-master/node-updater-pool.go

@@ -53,9 +53,9 @@ func (u *nodeUpdaterPool) processNodeUpdateRequest(queue workqueue.RateLimitingI
 	nodeUpdateRequests.Inc()
 
 	// Check if node exists
-	if _, err := u.nfdMaster.getNode(nodeName); apierrors.IsNotFound(err) {
+	if node, err := u.nfdMaster.getNode(nodeName); apierrors.IsNotFound(err) {
 		klog.InfoS("node not found, skip update", "nodeName", nodeName)
-	} else if err := u.nfdMaster.nfdAPIUpdateOneNode(nodeName); err != nil {
+	} else if err := u.nfdMaster.nfdAPIUpdateOneNode(node); err != nil {
 		if n := queue.NumRequeues(nodeName); n < 15 {
 			klog.InfoS("retrying node update", "nodeName", nodeName, "lastError", err, "numRetries", n)
 		} else {