nfd-master: patch node object instead of rewriting it

When updating node labels and annotations use JSON patches instead of
doing a read-modify-write on the whole node object. Patching is already
being used in managing extended resources so some of the existing code
was re-usable.

This patch should mitigate the problem of node update failures caused by
race conditions (a change in the node object between our read and write)
resulting e.g. in errors/restarts in nfd worker pods.

go.mod

@@ -8,6 +8,7 @@ require (
 	github.com/klauspost/cpuid v1.2.3
 	github.com/onsi/ginkgo v1.11.0
 	github.com/onsi/gomega v1.7.0
+	github.com/smartystreets/assertions v1.2.0
 	github.com/smartystreets/goconvey v1.6.4
 	github.com/stretchr/testify v1.4.0
 	github.com/vektra/errors v0.0.0-20140903201135-c64d83aba85a

go.sum

@@ -572,6 +572,8 @@ github.com/sirupsen/logrus v1.6.0 h1:UBcNElsrwanuuMsnGSlYmtmgbb23qDR5dG+6X6Oo89I
 github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88=
 github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d h1:zE9ykElWQ6/NYmHa3jpm/yHnI4xSofP+UP6SpjHcSeM=
 github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
+github.com/smartystreets/assertions v1.2.0 h1:42S6lae5dvLc7BrLu/0ugRtcFVjoJNMC/N3yZFZkDFs=
+github.com/smartystreets/assertions v1.2.0/go.mod h1:tcbTF8ujkAEcZ8TElKY+i30BzYlVhC/LOxJk7iOWnoo=
 github.com/smartystreets/goconvey v1.6.4 h1:fv0U8FUIMPNf1L9lnHLvLhgicrIVChEkdzIKYqbNC9s=
 github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=
 github.com/soheilhy/cmux v0.1.4 h1:0HKaf1o97UwFjHH9o5XsHUOF+tqmdA7KEzXLpiyaw0E=

pkg/apihelper/apihelpers.go

@@ -35,6 +35,9 @@ type APIHelpers interface {
 	// UpdateNode updates the node via the API server using a client.
 	UpdateNode(*k8sclient.Clientset, *api.Node) error
 
-	// PatchStatus updates the node status via the API server using a client.
+	// PatchNode updates the node object via the API server using a client.
-	PatchStatus(*k8sclient.Clientset, string, interface{}) error
+	PatchNode(*k8sclient.Clientset, string, []JsonPatch) error
+
+	// PatchNodeStatus updates the node status via the API server using a client.
+	PatchNodeStatus(*k8sclient.Clientset, string, []JsonPatch) error
 }

pkg/apihelper/k8shelpers.go

@@ -78,12 +78,25 @@ func (h K8sHelpers) UpdateNode(c *k8sclient.Clientset, n *api.Node) error {
 	return nil
 }
 
-func (h K8sHelpers) PatchStatus(c *k8sclient.Clientset, nodeName string, marshalable interface{}) error {
+func (h K8sHelpers) PatchNode(c *k8sclient.Clientset, nodeName string, patches []JsonPatch) error {
-	// Send the updated node to the apiserver.
+	if len(patches) > 0 {
-	patch, err := json.Marshal(marshalable)
+		data, err := json.Marshal(patches)
-	if err == nil {
+		if err == nil {
-		_, err = c.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.JSONPatchType, patch, meta_v1.PatchOptions{}, "status")
+			_, err = c.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.JSONPatchType, data, meta_v1.PatchOptions{})
+		}
+		return err
 	}
-
+	return nil
-	return err
+}
+
+func (h K8sHelpers) PatchNodeStatus(c *k8sclient.Clientset, nodeName string, patches []JsonPatch) error {
+	if len(patches) > 0 {
+		data, err := json.Marshal(patches)
+		if err == nil {
+			_, err = c.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.JSONPatchType, data, meta_v1.PatchOptions{}, "status")
+		}
+		return err
+	}
+	return nil
+
 }

pkg/apihelper/mock_APIHelpers.go

@@ -85,12 +85,26 @@ func (_m *MockAPIHelpers) GetNodes(_a0 *kubernetes.Clientset) (*v1.NodeList, err
 	return r0, r1
 }
 
-// PatchStatus provides a mock function with given fields: _a0, _a1, _a2
+// PatchNode provides a mock function with given fields: _a0, _a1, _a2
-func (_m *MockAPIHelpers) PatchStatus(_a0 *kubernetes.Clientset, _a1 string, _a2 interface{}) error {
+func (_m *MockAPIHelpers) PatchNode(_a0 *kubernetes.Clientset, _a1 string, _a2 []JsonPatch) error {
 	ret := _m.Called(_a0, _a1, _a2)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(*kubernetes.Clientset, string, interface{}) error); ok {
+	if rf, ok := ret.Get(0).(func(*kubernetes.Clientset, string, []JsonPatch) error); ok {
+		r0 = rf(_a0, _a1, _a2)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// PatchNodeStatus provides a mock function with given fields: _a0, _a1, _a2
+func (_m *MockAPIHelpers) PatchNodeStatus(_a0 *kubernetes.Clientset, _a1 string, _a2 []JsonPatch) error {
+	ret := _m.Called(_a0, _a1, _a2)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(*kubernetes.Clientset, string, []JsonPatch) error); ok {
 		r0 = rf(_a0, _a1, _a2)
 	} else {
 		r0 = ret.Error(0)

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

@@ -22,6 +22,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/smartystreets/assertions"
 	. "github.com/smartystreets/goconvey/convey"
 	"github.com/stretchr/testify/mock"
 	"github.com/vektra/errors"
@@ -33,6 +34,7 @@ import (
 	"sigs.k8s.io/node-feature-discovery/pkg/apihelper"
 	"sigs.k8s.io/node-feature-discovery/pkg/labeler"
 	"sigs.k8s.io/node-feature-discovery/pkg/version"
+	"sigs.k8s.io/yaml"
 )
 
 const (
@@ -55,13 +57,21 @@ func newMockNode() *api.Node {
 func TestUpdateNodeFeatures(t *testing.T) {
 	Convey("When I update the node using fake client", t, func() {
 		fakeFeatureLabels := map[string]string{LabelNs + "/source-feature.1": "1", LabelNs + "/source-feature.2": "2", LabelNs + "/source-feature.3": "val3"}
-		fakeAnnotations := map[string]string{AnnotationNs + "/version": version.Get()}
+		fakeAnnotations := map[string]string{"my-annotation": "my-val"}
 		fakeExtResources := ExtendedResources{LabelNs + "/source-feature.1": "", LabelNs + "/source-feature.2": ""}
+
 		fakeFeatureLabelNames := make([]string, 0, len(fakeFeatureLabels))
 		for k := range fakeFeatureLabels {
-			fakeFeatureLabelNames = append(fakeFeatureLabelNames, k)
+			fakeFeatureLabelNames = append(fakeFeatureLabelNames, strings.TrimPrefix(k, LabelNs+"/"))
 		}
 		sort.Strings(fakeFeatureLabelNames)
+
+		fakeExtResourceNames := make([]string, 0, len(fakeExtResources))
+		for k := range fakeExtResources {
+			fakeExtResourceNames = append(fakeExtResourceNames, strings.TrimPrefix(k, LabelNs+"/"))
+		}
+		sort.Strings(fakeExtResourceNames)
+
 		fakeAnnotations[AnnotationNs+"/feature-labels"] = strings.Join(fakeFeatureLabelNames, ",")
 
 		mockAPIHelper := new(apihelper.MockAPIHelpers)
@@ -72,25 +82,24 @@ func TestUpdateNodeFeatures(t *testing.T) {
 		mockNode.Annotations[AnnotationNs+"/feature-labels"] = "old-feature"
 
 		Convey("When I successfully update the node with feature labels", func() {
+			metadataPatches := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("replace", "/metadata/annotations", AnnotationNs+"/feature-labels", strings.Join(fakeFeatureLabelNames, ",")),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", "my-annotation", "my-val"),
+				apihelper.NewJsonPatch("remove", "/metadata/labels", LabelNs+"/old-feature", ""),
+			}
+			for k, v := range fakeFeatureLabels {
+				metadataPatches = append(metadataPatches, apihelper.NewJsonPatch("add", "/metadata/labels", k, v))
+			}
+
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Once()
-			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(nil).Once()
+			mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil)
-			mockAPIHelper.On("PatchStatus", mockClient, mockNodeName, mock.Anything).Return(nil).Twice()
+			mockAPIHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.Anything).Return(nil).Twice()
 			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
 			})
-			Convey("Node object should have updated with labels and annotations", func() {
-				So(len(mockNode.Labels), ShouldEqual, len(fakeFeatureLabels))
-				for k, v := range fakeFeatureLabels {
-					So(mockNode.Labels[k], ShouldEqual, v)
-				}
-				So(len(mockNode.Annotations), ShouldEqual, len(fakeAnnotations))
-				for k, v := range fakeAnnotations {
-					So(mockNode.Annotations[k], ShouldEqual, v)
-				}
-			})
 		})
 
 		Convey("When I fail to update the node with feature labels", func() {
@@ -128,7 +137,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Once()
-			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(expectedError).Once()
+			mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(expectedError).Once()
 			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
@@ -145,9 +154,11 @@ func TestUpdateMasterNode(t *testing.T) {
 		mockClient := &k8sclient.Clientset{}
 		mockNode := newMockNode()
 		Convey("When update operation succeeds", func() {
+			expectedPatches := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("add", "/metadata/annotations", AnnotationNs+"/master.version", version.Get())}
 			mockHelper.On("GetClient").Return(mockClient, nil)
 			mockHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil)
-			mockHelper.On("UpdateNode", mockClient, mockNode).Return(nil)
+			mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil)
 			err := updateMasterNode(mockHelper)
 			Convey("No error should be returned", func() {
 				So(err, ShouldBeNil)
@@ -175,7 +186,7 @@ func TestUpdateMasterNode(t *testing.T) {
 		Convey("When updating node object fails", func() {
 			mockHelper.On("GetClient").Return(mockClient, nil)
 			mockHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil)
-			mockHelper.On("UpdateNode", mockClient, mockNode).Return(mockErr)
+			mockHelper.On("PatchNode", mockClient, mockNodeName, mock.Anything).Return(mockErr)
 			err := updateMasterNode(mockHelper)
 			Convey("An error should be returned", func() {
 				So(err, ShouldEqual, mockErr)
@@ -189,31 +200,31 @@ func TestAddingExtResources(t *testing.T) {
 		Convey("When there are no matching labels", func() {
 			mockNode := newMockNode()
 			mockResourceLabels := ExtendedResources{}
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldEqual, 0)
+			So(len(patches), ShouldEqual, 0)
 		})
 
 		Convey("When there are matching labels", func() {
 			mockNode := newMockNode()
 			mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1", LabelNs + "feature-2": "2"}
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldBeGreaterThan, 0)
+			So(len(patches), ShouldBeGreaterThan, 0)
 		})
 
 		Convey("When the resource already exists", func() {
 			mockNode := newMockNode()
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
 			mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1"}
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldEqual, 0)
+			So(len(patches), ShouldEqual, 0)
 		})
 
 		Convey("When the resource already exists but its capacity has changed", func() {
 			mockNode := newMockNode()
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(2, resource.BinarySI)
 			mockResourceLabels := ExtendedResources{LabelNs + "/feature-1": "1"}
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldBeGreaterThan, 0)
+			So(len(patches), ShouldBeGreaterThan, 0)
 		})
 	})
 }
@@ -226,8 +237,8 @@ func TestRemovingExtResources(t *testing.T) {
 			mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-1,feature-2"
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldEqual, 0)
+			So(len(patches), ShouldEqual, 0)
 		})
 		Convey("When the related label is gone", func() {
 			mockNode := newMockNode()
@@ -235,8 +246,8 @@ func TestRemovingExtResources(t *testing.T) {
 			mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-4,feature-2"
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-4")] = *resource.NewQuantity(4, resource.BinarySI)
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldBeGreaterThan, 0)
+			So(len(patches), ShouldBeGreaterThan, 0)
 		})
 		Convey("When the extended resource is no longer wanted", func() {
 			mockNode := newMockNode()
@@ -244,8 +255,8 @@ func TestRemovingExtResources(t *testing.T) {
 			mockNode.Status.Capacity[api.ResourceName(LabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
 			mockResourceLabels := ExtendedResources{LabelNs + "/feature-2": "2"}
 			mockNode.Annotations[AnnotationNs+"/extended-resources"] = "feature-1,feature-2"
-			resourceOps := createExtendedResourcePatches(mockNode, mockResourceLabels)
+			patches := createExtendedResourcePatches(mockNode, mockResourceLabels)
-			So(len(resourceOps), ShouldBeGreaterThan, 0)
+			So(len(patches), ShouldBeGreaterThan, 0)
 		})
 	})
 }
@@ -268,75 +279,69 @@ func TestSetLabels(t *testing.T) {
 			mockLabelNames = append(mockLabelNames, k)
 		}
 		sort.Strings(mockLabelNames)
-		expectedAnnotations := map[string]string{"worker.version": workerVer}
-		expectedAnnotations["feature-labels"] = strings.Join(mockLabelNames, ",")
-		expectedAnnotations["extended-resources"] = ""
 
 		Convey("When node update succeeds", func() {
+			expectedPatches := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("add", "/metadata/annotations", workerVersionAnnotation, workerVer),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", featureLabelAnnotation, strings.Join(mockLabelNames, ",")),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", extendedResourceAnnotation, ""),
+			}
+			for k, v := range mockLabels {
+				expectedPatches = append(expectedPatches, apihelper.NewJsonPatch("add", "/metadata/labels", LabelNs+"/"+k, v))
+			}
+
 			mockHelper.On("GetClient").Return(mockClient, nil)
 			mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil)
-			mockHelper.On("UpdateNode", mockClient, mockNode).Return(nil)
+			mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil)
+			mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.Anything).Return(nil)
 			_, err := mockServer.SetLabels(mockCtx, mockReq)
 			Convey("No error should be returned", func() {
 				So(err, ShouldBeNil)
 			})
-			Convey("Node object should have updated with labels and annotations", func() {
-				So(len(mockNode.Labels), ShouldEqual, len(mockLabels))
-				for k, v := range mockLabels {
-					So(mockNode.Labels[LabelNs+"/"+k], ShouldEqual, v)
-				}
-				So(len(mockNode.Annotations), ShouldEqual, len(expectedAnnotations))
-				for k, v := range expectedAnnotations {
-					So(mockNode.Annotations[AnnotationNs+"/"+k], ShouldEqual, v)
-				}
-			})
 		})
 
 		Convey("When --label-whitelist is specified", func() {
+			expectedPatches := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("add", "/metadata/annotations", workerVersionAnnotation, workerVer),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", featureLabelAnnotation, "feature-2"),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", extendedResourceAnnotation, ""),
+				apihelper.NewJsonPatch("add", "/metadata/labels", LabelNs+"/feature-2", mockLabels["feature-2"]),
+			}
+
 			mockServer.args.LabelWhiteList = regexp.MustCompile("^f.*2$")
 			mockHelper.On("GetClient").Return(mockClient, nil)
 			mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil)
-			mockHelper.On("UpdateNode", mockClient, mockNode).Return(nil)
+			mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil)
+			mockHelper.On("PatchNodeStatus", mockClient, mockNodeName, mock.Anything).Return(nil)
 			_, err := mockServer.SetLabels(mockCtx, mockReq)
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
 			})
-			Convey("Node object should only have whitelisted labels", func() {
-				So(len(mockNode.Labels), ShouldEqual, 1)
-				So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "/feature-2": "val-2"})
-
-				a := map[string]string{AnnotationNs + "/worker.version": workerVer,
-					AnnotationNs + "/feature-labels":     "feature-2",
-					AnnotationNs + "/extended-resources": ""}
-				So(len(mockNode.Annotations), ShouldEqual, len(a))
-				So(mockNode.Annotations, ShouldResemble, a)
-			})
 		})
 
 		Convey("When --extra-label-ns is specified", func() {
-			mockServer.args.ExtraLabelNs = map[string]struct{}{"valid.ns": struct{}{}}
-			mockHelper.On("GetClient").Return(mockClient, nil)
-			mockHelper.On("GetNode", mockClient, workerName).Return(mockNode, nil)
-			mockHelper.On("UpdateNode", mockClient, mockNode).Return(nil)
 			// In the gRPC request the label names may omit the default ns
 			mockLabels := map[string]string{"feature-1": "val-1",
 				"valid.ns/feature-2":   "val-2",
 				"invalid.ns/feature-3": "val-3"}
+			expectedPatches := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("add", "/metadata/annotations", workerVersionAnnotation, workerVer),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", featureLabelAnnotation, "feature-1,valid.ns/feature-2"),
+				apihelper.NewJsonPatch("add", "/metadata/annotations", extendedResourceAnnotation, ""),
+				apihelper.NewJsonPatch("add", "/metadata/labels", LabelNs+"/feature-1", mockLabels["feature-1"]),
+				apihelper.NewJsonPatch("add", "/metadata/labels", "valid.ns/feature-2", mockLabels["valid.ns/feature-2"]),
+			}
+
+			mockServer.args.ExtraLabelNs = map[string]struct{}{"valid.ns": struct{}{}}
+			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.Anything).Return(nil)
 			mockReq := &labeler.SetLabelsRequest{NodeName: workerName, NfdVersion: workerVer, Labels: mockLabels}
 			_, err := mockServer.SetLabels(mockCtx, mockReq)
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
 			})
-			Convey("Node object should only have allowed label namespaces", func() {
-				So(len(mockNode.Labels), ShouldEqual, 2)
-				So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "/feature-1": "val-1", "valid.ns/feature-2": "val-2"})
-
-				a := map[string]string{AnnotationNs + "/worker.version": workerVer,
-					AnnotationNs + "/feature-labels":     "feature-1,valid.ns/feature-2",
-					AnnotationNs + "/extended-resources": ""}
-				So(len(mockNode.Annotations), ShouldEqual, len(a))
-				So(mockNode.Annotations, ShouldResemble, a)
-			})
 		})
 
 		mockErr := errors.New("mock-error")
@@ -358,28 +363,46 @@ func TestSetLabels(t *testing.T) {
 	})
 }
 
-func TestAddLabels(t *testing.T) {
+func TestCreatePatches(t *testing.T) {
-	Convey("When adding labels", t, func() {
+	Convey("When creating JSON patches", t, func() {
-		labels := map[string]string{}
+		existingItems := map[string]string{"key-1": "val-1", "key-2": "val-2", "key-3": "val-3"}
-		n := &api.Node{
+		jsonPath := "/root"
-			ObjectMeta: meta_v1.ObjectMeta{
-				Labels: map[string]string{},
-			},
-		}
 
-		Convey("If no labels are passed", func() {
+		Convey("When when there are neither itmes to remoe nor to add or update", func() {
-			addLabels(n, labels)
+			p := createPatches([]string{"foo", "bar"}, existingItems, map[string]string{}, jsonPath)
-
+			So(len(p), ShouldEqual, 0)
-			Convey("None should be added", func() {
-				So(len(n.Labels), ShouldEqual, 0)
-			})
 		})
 
-		Convey("They should be added to the node.Labels", func() {
+		Convey("When when there are itmes to remoe but none to add or update", func() {
-			test1 := "test1"
+			p := createPatches([]string{"key-2", "key-3", "foo"}, existingItems, map[string]string{}, jsonPath)
-			labels[LabelNs+"/"+test1] = "true"
+			expected := []apihelper.JsonPatch{
-			addLabels(n, labels)
+				apihelper.NewJsonPatch("remove", jsonPath, "key-2", ""),
-			So(n.Labels, ShouldContainKey, LabelNs+"/"+test1)
+				apihelper.NewJsonPatch("remove", jsonPath, "key-3", ""),
+			}
+			So(sortJsonPatches(p), ShouldResemble, sortJsonPatches(expected))
+		})
+
+		Convey("When when there are no itmes to remove but new items to add", func() {
+			newItems := map[string]string{"new-key": "new-val", "key-1": "new-1"}
+			p := createPatches([]string{"key-1"}, existingItems, newItems, jsonPath)
+			expected := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("add", jsonPath, "new-key", newItems["new-key"]),
+				apihelper.NewJsonPatch("replace", jsonPath, "key-1", newItems["key-1"]),
+			}
+			So(sortJsonPatches(p), ShouldResemble, sortJsonPatches(expected))
+		})
+
+		Convey("When when there are items to remove add and update", func() {
+			newItems := map[string]string{"new-key": "new-val", "key-2": "new-2", "key-4": "val-4"}
+			p := createPatches([]string{"key-1", "key-2", "key-3", "foo"}, existingItems, newItems, jsonPath)
+			expected := []apihelper.JsonPatch{
+				apihelper.NewJsonPatch("add", jsonPath, "new-key", newItems["new-key"]),
+				apihelper.NewJsonPatch("add", jsonPath, "key-4", newItems["key-4"]),
+				apihelper.NewJsonPatch("replace", jsonPath, "key-2", newItems["key-2"]),
+				apihelper.NewJsonPatch("remove", jsonPath, "key-1", ""),
+				apihelper.NewJsonPatch("remove", jsonPath, "key-3", ""),
+			}
+			So(sortJsonPatches(p), ShouldResemble, sortJsonPatches(expected))
 		})
 	})
 }
@@ -397,16 +420,16 @@ func TestRemoveLabelsWithPrefix(t *testing.T) {
 		}
 
 		Convey("a unique label should be removed", func() {
-			removeLabelsWithPrefix(n, "single")
+			p := removeLabelsWithPrefix(n, "single")
-			So(len(n.Labels), ShouldEqual, 2)
+			So(p, ShouldResemble, []apihelper.JsonPatch{apihelper.NewJsonPatch("remove", "/metadata/labels", "single-label", "")})
-			So(n.Labels, ShouldNotContainKey, "single")
 		})
 
 		Convey("a non-unique search string should remove all matching keys", func() {
-			removeLabelsWithPrefix(n, "multiple")
+			p := removeLabelsWithPrefix(n, "multiple")
-			So(len(n.Labels), ShouldEqual, 1)
+			So(sortJsonPatches(p), ShouldResemble, sortJsonPatches([]apihelper.JsonPatch{
-			So(n.Labels, ShouldNotContainKey, "multiple_A")
+				apihelper.NewJsonPatch("remove", "/metadata/labels", "multiple_A", ""),
-			So(n.Labels, ShouldNotContainKey, "multiple_B")
+				apihelper.NewJsonPatch("remove", "/metadata/labels", "multiple_B", ""),
+			}))
 		})
 
 		Convey("a search string with no matches should not alter labels", func() {
@@ -418,3 +441,25 @@ func TestRemoveLabelsWithPrefix(t *testing.T) {
 		})
 	})
 }
+
+func jsonPatchMatcher(expected []apihelper.JsonPatch) func([]apihelper.JsonPatch) bool {
+	return func(actual []apihelper.JsonPatch) bool {
+		// We don't care about modifying the original slices
+		ok, msg := assertions.So(sortJsonPatches(actual), ShouldResemble, sortJsonPatches(expected))
+		if !ok {
+			// We parse the cryptic string message for better readability
+			var f assertions.FailureView
+			if err := yaml.Unmarshal([]byte(msg), &f); err == nil {
+				Printf("%s\n", f.Message)
+			} else {
+				Printf("%s\n", msg)
+			}
+		}
+		return ok
+	}
+}
+
+func sortJsonPatches(p []apihelper.JsonPatch) []apihelper.JsonPatch {
+	sort.Slice(p, func(i, j int) bool { return p[i].Path < p[j].Path })
+	return p
+}

pkg/nfd-master/nfd-master.go

@@ -47,6 +47,12 @@ const (
 
 	// Namespace for all NFD-related annotations
 	AnnotationNs = "nfd.node.kubernetes.io"
+
+	// NFD Annotations
+	extendedResourceAnnotation = AnnotationNs + "/extended-resources"
+	featureLabelAnnotation     = AnnotationNs + "/feature-labels"
+	masterVersionAnnotation    = AnnotationNs + "/master.version"
+	workerVersionAnnotation    = AnnotationNs + "/worker.version"
 )
 
 // package loggers
@@ -246,10 +252,10 @@ func updateMasterNode(helper apihelper.APIHelpers) error {
 	}
 
 	// Advertise NFD version as an annotation
-	addAnnotations(node, Annotations{AnnotationNs + "/master.version": version.Get()})
+	p := createPatches(nil, node.Annotations, Annotations{masterVersionAnnotation: version.Get()}, "/metadata/annotations")
-	err = helper.UpdateNode(cli, node)
+	err = helper.PatchNode(cli, node.Name, p)
 	if err != nil {
-		stderrLogger.Printf("can't update node: %s", err.Error())
+		stderrLogger.Printf("failed to patch node annotations: %v", err)
 		return err
 	}
 
@@ -342,9 +348,9 @@ func (s *labelerServer) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*p
 	if !s.args.NoPublish {
 		// Advertise NFD worker version, label names and extended resources as annotations
 		labelKeys := make([]string, 0, len(labels))
-		for k := range labels {
+		for key := range labels {
 			// Drop the ns part for labels in the default ns
-			labelKeys = append(labelKeys, strings.TrimPrefix(k, LabelNs+"/"))
+			labelKeys = append(labelKeys, strings.TrimPrefix(key, LabelNs+"/"))
 		}
 		sort.Strings(labelKeys)
 
@@ -355,9 +361,9 @@ func (s *labelerServer) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*p
 		}
 		sort.Strings(extendedResourceKeys)
 
-		annotations := Annotations{AnnotationNs + "/worker.version": r.NfdVersion,
+		annotations := Annotations{workerVersionAnnotation: r.NfdVersion,
-			AnnotationNs + "/feature-labels":     strings.Join(labelKeys, ","),
+			featureLabelAnnotation:     strings.Join(labelKeys, ","),
-			AnnotationNs + "/extended-resources": strings.Join(extendedResourceKeys, ","),
+			extendedResourceAnnotation: strings.Join(extendedResourceKeys, ","),
 		}
 
 		err := updateNodeFeatures(s.apiHelper, r.NodeName, labels, annotations, extendedResources)
@@ -384,85 +390,88 @@ func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Lab
 		return err
 	}
 
-	// Resolve publishable extended resources before node is modified
+	// Create JSON patches for changes in labels and annotations
-	patches := createExtendedResourcePatches(node, extendedResources)
+	oldLabels := stringToNsNames(node.Annotations[featureLabelAnnotation], LabelNs)
-
+	patches := createPatches(oldLabels, node.Labels, labels, "/metadata/labels")
-	// Remove old labels
+	patches = append(patches, createPatches(nil, node.Annotations, annotations, "/metadata/annotations")...)
-	if l, ok := node.Annotations[AnnotationNs+"/feature-labels"]; ok {
-		oldLabels := strings.Split(l, ",")
-		for i, name := range oldLabels {
-			// Names in the annotation may omit the default ns
-			oldLabels[i] = addNs(name, LabelNs)
-		}
-		removeLabels(node, oldLabels)
-	}
 
 	// Also, remove all labels with the old prefix, and the old version label
-	removeLabelsWithPrefix(node, "node.alpha.kubernetes-incubator.io/nfd")
+	patches = append(patches, removeLabelsWithPrefix(node, "node.alpha.kubernetes-incubator.io/nfd")...)
-	removeLabelsWithPrefix(node, "node.alpha.kubernetes-incubator.io/node-feature-discovery")
+	patches = append(patches, removeLabelsWithPrefix(node, "node.alpha.kubernetes-incubator.io/node-feature-discovery")...)
 
-	// Add labels to the node object.
+	// Patch the node object in the apiserver
-	addLabels(node, labels)
+	err = helper.PatchNode(cli, node.Name, patches)
-
-	// Add annotations
-	addAnnotations(node, annotations)
-
-	// Send the updated node to the apiserver.
-	err = helper.UpdateNode(cli, node)
 	if err != nil {
-		stderrLogger.Printf("can't update node: %s", err.Error())
+		stderrLogger.Printf("error while patching node object: %s", err.Error())
 		return err
 	}
 
 	// patch node status with extended resource changes
-	if len(patches) > 0 {
+	patches = createExtendedResourcePatches(node, extendedResources)
-		err = helper.PatchStatus(cli, node.Name, patches)
+	err = helper.PatchNodeStatus(cli, node.Name, patches)
-		if err != nil {
+	if err != nil {
-			stderrLogger.Printf("error while patching extended resources: %s", err.Error())
+		stderrLogger.Printf("error while patching extended resources: %s", err.Error())
-			return err
+		return err
-		}
 	}
 
 	return err
 }
 
 // Remove any labels having the given prefix
-func removeLabelsWithPrefix(n *api.Node, search string) {
+func removeLabelsWithPrefix(n *api.Node, search string) []apihelper.JsonPatch {
+	var p []apihelper.JsonPatch
+
 	for k := range n.Labels {
 		if strings.HasPrefix(k, search) {
-			delete(n.Labels, k)
+			p = append(p, apihelper.NewJsonPatch("remove", "/metadata/labels", k, ""))
 		}
 	}
+
+	return p
 }
 
-// Removes NFD labels from a Node object
+// createPatches is a generic helper that returns json patch operations to perform
-func removeLabels(n *api.Node, labelNames []string) {
+func createPatches(removeKeys []string, oldItems map[string]string, newItems map[string]string, jsonPath string) []apihelper.JsonPatch {
-	for _, l := range labelNames {
+	patches := []apihelper.JsonPatch{}
-		delete(n.Labels, l)
+
+	// Determine items to remove
+	for _, key := range removeKeys {
+		if _, ok := oldItems[key]; ok {
+			if _, ok := newItems[key]; !ok {
+				patches = append(patches, apihelper.NewJsonPatch("remove", jsonPath, key, ""))
+			}
+		}
 	}
+
+	// Determine items to add or replace
+	for key, newVal := range newItems {
+		if oldVal, ok := oldItems[key]; ok {
+			if newVal != oldVal {
+				patches = append(patches, apihelper.NewJsonPatch("replace", jsonPath, key, newVal))
+			}
+		} else {
+			patches = append(patches, apihelper.NewJsonPatch("add", jsonPath, key, newVal))
+		}
+	}
+
+	return patches
 }
 
 // createExtendedResourcePatches returns a slice of operations to perform on
 // the node status
 func createExtendedResourcePatches(n *api.Node, extendedResources ExtendedResources) []apihelper.JsonPatch {
-	var patches []apihelper.JsonPatch
+	patches := []apihelper.JsonPatch{}
 
 	// Form a list of namespaced resource names managed by us
-	if old, ok := n.Annotations[AnnotationNs+"/extended-resources"]; ok {
+	oldResources := stringToNsNames(n.Annotations[extendedResourceAnnotation], LabelNs)
-		oldResources := strings.Split(old, ",")
-		for i, name := range oldResources {
-			// Names in the annotation may omit the default ns
-			oldResources[i] = addNs(name, LabelNs)
-		}
 
-		// figure out which resources to remove
+	// figure out which resources to remove
-		for _, resource := range oldResources {
+	for _, resource := range oldResources {
-			if _, ok := n.Status.Capacity[api.ResourceName(resource)]; ok {
+		if _, ok := n.Status.Capacity[api.ResourceName(resource)]; ok {
-				// check if the ext resource is still needed
+			// check if the ext resource is still needed
-				if _, extResNeeded := extendedResources[resource]; !extResNeeded {
+			if _, extResNeeded := extendedResources[resource]; !extResNeeded {
-					patches = append(patches, apihelper.NewJsonPatch("remove", "/status/capacity", resource, ""))
+				patches = append(patches, apihelper.NewJsonPatch("remove", "/status/capacity", resource, ""))
-					patches = append(patches, apihelper.NewJsonPatch("remove", "/status/allocatable", resource, ""))
+				patches = append(patches, apihelper.NewJsonPatch("remove", "/status/allocatable", resource, ""))
-				}
 			}
 		}
 	}
@@ -485,20 +494,6 @@ func createExtendedResourcePatches(n *api.Node, extendedResources ExtendedResour
 	return patches
 }
 
-// Add NFD labels to a Node object.
-func addLabels(n *api.Node, labels Labels) {
-	for k, v := range labels {
-		n.Labels[k] = v
-	}
-}
-
-// Add Annotations to a Node object
-func addAnnotations(n *api.Node, annotations Annotations) {
-	for k, v := range annotations {
-		n.Annotations[k] = v
-	}
-}
-
 // addNs adds a namespace if one isn't already found from src string
 func addNs(src string, nsToAdd string) string {
 	if strings.Contains(src, "/") {
@@ -515,3 +510,17 @@ func splitNs(fullname string) (string, string) {
 	}
 	return "", fullname
 }
+
+// stringToNsNames is a helper for converting a string of comma-separated names
+// into a slice of fully namespaced names
+func stringToNsNames(cslist, ns string) []string {
+	var names []string
+	if cslist != "" {
+		names = strings.Split(cslist, ",")
+		for i, name := range names {
+			// Expect that names may omit the ns part
+			names[i] = addNs(name, ns)
+		}
+	}
+	return names
+}