Skip to content

Commit

Permalink
nfd-master: fix node update
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@marquiz
marquiz committed Apr 6, 2023
1 parent ec014f1 commit f64c239
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 17 deletions.
23 changes: 12 additions & 11 deletions pkg/nfd-master/nfd-master-internal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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() {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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() {
Expand All @@ -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)
Expand Down
12 changes: 6 additions & 6 deletions pkg/nfd-master/nfd-master.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -878,19 +878,19 @@ 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)
if err != nil {
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 {
Expand Down

0 comments on commit f64c239

Please sign in to comment.