diff --git a/go.mod b/go.mod index bf92484f7..b8e803e7d 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 913d5391c..1efa099fe 100644 --- a/go.sum +++ b/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= diff --git a/pkg/apihelper/apihelpers.go b/pkg/apihelper/apihelpers.go index 59e1b285e..8a7edc98e 100644 --- a/pkg/apihelper/apihelpers.go +++ b/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. - PatchStatus(*k8sclient.Clientset, string, interface{}) error + // PatchNode updates the node object via the API server using a client. + PatchNode(*k8sclient.Clientset, string, []JsonPatch) error + + // PatchNodeStatus updates the node status via the API server using a client. + PatchNodeStatus(*k8sclient.Clientset, string, []JsonPatch) error } diff --git a/pkg/apihelper/k8shelpers.go b/pkg/apihelper/k8shelpers.go index 8a4186065..d029b02c0 100644 --- a/pkg/apihelper/k8shelpers.go +++ b/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 { - // Send the updated node to the apiserver. - patch, err := json.Marshal(marshalable) - if err == nil { - _, err = c.CoreV1().Nodes().Patch(context.TODO(), nodeName, types.JSONPatchType, patch, meta_v1.PatchOptions{}, "status") +func (h K8sHelpers) PatchNode(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{}) + } + return err } - - return err + return nil +} + +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 + } diff --git a/pkg/apihelper/mock_APIHelpers.go b/pkg/apihelper/mock_APIHelpers.go index d79f56326..136a618fc 100644 --- a/pkg/apihelper/mock_APIHelpers.go +++ b/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 -func (_m *MockAPIHelpers) PatchStatus(_a0 *kubernetes.Clientset, _a1 string, _a2 interface{}) error { +// PatchNode provides a mock function with given fields: _a0, _a1, _a2 +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) diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go index 81f80e273..2c66acf5c 100644 --- a/pkg/nfd-master/nfd-master-internal_test.go +++ b/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("PatchStatus", mockClient, mockNodeName, mock.Anything).Return(nil).Twice() + mockAPIHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(metadataPatches))).Return(nil) + 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) - So(len(resourceOps), ShouldEqual, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) - So(len(resourceOps), ShouldBeGreaterThan, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) - So(len(resourceOps), ShouldEqual, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) - So(len(resourceOps), ShouldBeGreaterThan, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) - So(len(resourceOps), ShouldEqual, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) - So(len(resourceOps), ShouldBeGreaterThan, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) - So(len(resourceOps), ShouldBeGreaterThan, 0) + patches := createExtendedResourcePatches(mockNode, mockResourceLabels) + 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) { - Convey("When adding labels", t, func() { - labels := map[string]string{} - n := &api.Node{ - ObjectMeta: meta_v1.ObjectMeta{ - Labels: map[string]string{}, - }, - } +func TestCreatePatches(t *testing.T) { + Convey("When creating JSON patches", t, func() { + existingItems := map[string]string{"key-1": "val-1", "key-2": "val-2", "key-3": "val-3"} + jsonPath := "/root" - Convey("If no labels are passed", func() { - addLabels(n, labels) - - Convey("None should be added", func() { - So(len(n.Labels), ShouldEqual, 0) - }) + Convey("When when there are neither itmes to remoe nor to add or update", func() { + p := createPatches([]string{"foo", "bar"}, existingItems, map[string]string{}, jsonPath) + So(len(p), ShouldEqual, 0) }) - Convey("They should be added to the node.Labels", func() { - test1 := "test1" - labels[LabelNs+"/"+test1] = "true" - addLabels(n, labels) - So(n.Labels, ShouldContainKey, LabelNs+"/"+test1) + Convey("When when there are itmes to remoe but none to add or update", func() { + p := createPatches([]string{"key-2", "key-3", "foo"}, existingItems, map[string]string{}, jsonPath) + expected := []apihelper.JsonPatch{ + apihelper.NewJsonPatch("remove", jsonPath, "key-2", ""), + 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") - So(len(n.Labels), ShouldEqual, 2) - So(n.Labels, ShouldNotContainKey, "single") + p := removeLabelsWithPrefix(n, "single") + So(p, ShouldResemble, []apihelper.JsonPatch{apihelper.NewJsonPatch("remove", "/metadata/labels", "single-label", "")}) }) Convey("a non-unique search string should remove all matching keys", func() { - removeLabelsWithPrefix(n, "multiple") - So(len(n.Labels), ShouldEqual, 1) - So(n.Labels, ShouldNotContainKey, "multiple_A") - So(n.Labels, ShouldNotContainKey, "multiple_B") + p := removeLabelsWithPrefix(n, "multiple") + So(sortJsonPatches(p), ShouldResemble, sortJsonPatches([]apihelper.JsonPatch{ + apihelper.NewJsonPatch("remove", "/metadata/labels", "multiple_A", ""), + 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 +} diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go index 668c836cf..6ab2ef8d5 100644 --- a/pkg/nfd-master/nfd-master.go +++ b/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()}) - err = helper.UpdateNode(cli, node) + p := createPatches(nil, node.Annotations, Annotations{masterVersionAnnotation: version.Get()}, "/metadata/annotations") + 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, - AnnotationNs + "/feature-labels": strings.Join(labelKeys, ","), - AnnotationNs + "/extended-resources": strings.Join(extendedResourceKeys, ","), + annotations := Annotations{workerVersionAnnotation: r.NfdVersion, + featureLabelAnnotation: strings.Join(labelKeys, ","), + 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 - patches := createExtendedResourcePatches(node, extendedResources) - - // Remove old labels - 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) - } + // Create JSON patches for changes in labels and annotations + oldLabels := stringToNsNames(node.Annotations[featureLabelAnnotation], LabelNs) + patches := createPatches(oldLabels, node.Labels, labels, "/metadata/labels") + patches = append(patches, createPatches(nil, node.Annotations, annotations, "/metadata/annotations")...) // Also, remove all labels with the old prefix, and the old version label - 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/nfd")...) + patches = append(patches, removeLabelsWithPrefix(node, "node.alpha.kubernetes-incubator.io/node-feature-discovery")...) - // Add labels to the node object. - addLabels(node, labels) - - // Add annotations - addAnnotations(node, annotations) - - // Send the updated node to the apiserver. - err = helper.UpdateNode(cli, node) + // Patch the node object in the apiserver + err = helper.PatchNode(cli, node.Name, patches) 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 { - err = helper.PatchStatus(cli, node.Name, patches) - if err != nil { - stderrLogger.Printf("error while patching extended resources: %s", err.Error()) - return err - } + patches = createExtendedResourcePatches(node, extendedResources) + err = helper.PatchNodeStatus(cli, node.Name, patches) + if err != nil { + stderrLogger.Printf("error while patching extended resources: %s", err.Error()) + 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 -func removeLabels(n *api.Node, labelNames []string) { - for _, l := range labelNames { - delete(n.Labels, l) +// createPatches is a generic helper that returns json patch operations to perform +func createPatches(removeKeys []string, oldItems map[string]string, newItems map[string]string, jsonPath string) []apihelper.JsonPatch { + patches := []apihelper.JsonPatch{} + + // 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 := strings.Split(old, ",") - for i, name := range oldResources { - // Names in the annotation may omit the default ns - oldResources[i] = addNs(name, LabelNs) - } + oldResources := stringToNsNames(n.Annotations[extendedResourceAnnotation], LabelNs) - // figure out which resources to remove - for _, resource := range oldResources { - if _, ok := n.Status.Capacity[api.ResourceName(resource)]; ok { - // check if the ext resource is still needed - if _, extResNeeded := extendedResources[resource]; !extResNeeded { - patches = append(patches, apihelper.NewJsonPatch("remove", "/status/capacity", resource, "")) - patches = append(patches, apihelper.NewJsonPatch("remove", "/status/allocatable", resource, "")) - } + // figure out which resources to remove + for _, resource := range oldResources { + if _, ok := n.Status.Capacity[api.ResourceName(resource)]; ok { + // check if the ext resource is still needed + if _, extResNeeded := extendedResources[resource]; !extResNeeded { + patches = append(patches, apihelper.NewJsonPatch("remove", "/status/capacity", 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 +}