From f64c23968ad21dc855eb05fe46279e1b05c75ea4 Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Thu, 6 Apr 2023 21:20:24 +0300 Subject: [PATCH] nfd-master: fix node update Update node status before node metadata. This fixes a problem where we lose track of NFD-managed extended resources in case patching node status fails. Previously we removed all labels and annotations (including the one listing our ERs) and only after that updated node status. If node status update failed we had lost the annotation but extended resources were still there, leaving them orphaned. --- pkg/nfd-master/nfd-master-internal_test.go | 23 +++++++++++----------- pkg/nfd-master/nfd-master.go | 12 +++++------ 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 1832efc80d..3de69d2887 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/pkg/nfd-master/nfd-master-internal_test.go @@ -86,6 +86,12 @@ func TestUpdateNodeObject(t *testing.T) { } sort.Strings(fakeExtResourceNames) + // Create a list of expected node status patches + statusPatches := []apihelper.JsonPatch{} + for k, v := range fakeExtResources { + statusPatches = append(statusPatches, apihelper.NewJsonPatch("add", "/status/capacity", k, v)) + } + mockAPIHelper := new(apihelper.MockAPIHelpers) mockMaster := newMockMaster(mockAPIHelper) mockClient := &k8sclient.Clientset{} @@ -108,16 +114,10 @@ func TestUpdateNodeObject(t *testing.T) { metadataPatches = append(metadataPatches, apihelper.NewJsonPatch("add", "/metadata/annotations", k, v)) } - // Create a list of expected node status patches - statusPatches := []apihelper.JsonPatch{} - for k, v := range fakeExtResources { - statusPatches = append(statusPatches, apihelper.NewJsonPatch("add", "/status/capacity", k, v)) - } - mockAPIHelper.On("GetClient").Return(mockClient, nil) mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Twice() - mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil) mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil) + mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil) err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) Convey("Error is nil", func() { @@ -160,6 +160,7 @@ func TestUpdateNodeObject(t *testing.T) { expectedError := errors.New("fake error") mockAPIHelper.On("GetClient").Return(mockClient, nil) mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Twice() + mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(statusPatches))).Return(nil) mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(expectedError).Twice() err := mockMaster.updateNodeObject(mockClient, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources, nil) @@ -328,8 +329,8 @@ func TestSetLabels(t *testing.T) { mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil).Twice() - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) + mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) _, err := mockMaster.SetLabels(mockCtx, mockReq) Convey("No error should be returned", func() { So(err, ShouldBeNil) @@ -347,8 +348,8 @@ func TestSetLabels(t *testing.T) { mockMaster.config.LabelWhiteList.Regexp = *regexp.MustCompile("^f.*2$") mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) + mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) _, err := mockMaster.SetLabels(mockCtx, mockReq) Convey("Error is nil", func() { So(err, ShouldBeNil) @@ -385,8 +386,8 @@ func TestSetLabels(t *testing.T) { mockMaster.args.Instance = instance mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) + mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels} _, err := mockMaster.SetLabels(mockCtx, mockReq) Convey("Error is nil", func() { @@ -410,8 +411,8 @@ func TestSetLabels(t *testing.T) { mockMaster.config.ResourceLabels = map[string]struct{}{"feature-3": {}, "feature-1": {}} mockHelper.On("GetClient").Return(mockClient, nil) mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil) - mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedStatusPatches))).Return(nil) + mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil) _, err := mockMaster.SetLabels(mockCtx, mockReq) Convey("Error is nil", func() { So(err, ShouldBeNil) diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 1722809903..f1441cf89d 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/pkg/nfd-master/nfd-master.go @@ -878,12 +878,6 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, patches := createPatches(oldLabels, node.Labels, labels, "/metadata/labels") patches = append(patches, createPatches(nil, node.Annotations, annotations, "/metadata/annotations")...) - // Patch the node object in the apiserver - err = m.apihelper.PatchNode(cli, node.Name, patches) - if err != nil { - return fmt.Errorf("error while patching node object: %v", err) - } - // patch node status with extended resource changes statusPatches := m.createExtendedResourcePatches(node, extendedResources) err = m.apihelper.PatchNodeStatus(cli, node.Name, statusPatches) @@ -891,6 +885,12 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string, return fmt.Errorf("error while patching extended resources: %v", err) } + // Patch the node object in the apiserver + err = m.apihelper.PatchNode(cli, node.Name, patches) + if err != nil { + return fmt.Errorf("error while patching node object: %v", err) + } + if len(patches) > 0 || len(statusPatches) > 0 { klog.Infof("node %q updated", nodeName) } else {