Merge pull request #39 from IRCody/remove_labels

Automatic merge from submit-queue

Fixes #25: Deletes non-discovered nfd labels

Took a stab at #25. Appreciate any feedback.

Modifies AddLabels() to first remove any non-discovered nfd labels from
the node labels before adding newly discovered labels.

Adds tests to main_test.go to test the behavior of AddLabels.

edit:

This deletes non-discovered labels including those that may not be "discovered" because of the whitelist option. Wanted to make sure I called that out.

README.md

@@ -80,6 +80,11 @@ the only label value published for features is the string `"true"`._
 
 The `--sources` flag controls which sources to use for discovery.
 
+_Note: Consecutive runs of node-feature-discovery will update the labels on a
+given node. If features are not discovered on a consecutive run, the corresponding
+label will be removed. This includes any restrictions placed on the consecutive run,
+such as restricting discovered features with the --label-whitelist option._
+
 ### Intel Resource Director Technology (RDT) Features
 
 | Feature name   | Description                                                                         |

main.go

@@ -43,7 +43,12 @@ type APIHelpers interface {
 	// GetNode returns the Kubernetes node on which this container is running.
 	GetNode(*client.Client) (*api.Node, error)
 
-	// addLabels modifies the supplied node's labels collection.
+	// RemoveLabels removes labels from the supplied node that contain the
+	// search string provided. In order to publish the changes, the node must
+	// subsequently be updated via the API server using the client library.
+	RemoveLabels(*api.Node, string)
+
+	// AddLabels modifies the supplied node's labels collection.
 	// In order to publish the labels, the node must be subsequently updated via the
 	// API server using the client library.
 	AddLabels(*api.Node, Labels)
@@ -181,6 +186,8 @@ func advertiseFeatureLabels(helper APIHelpers, labels Labels) error {
 		return err
 	}
 
+	// Remove labels with our prefix
+	helper.RemoveLabels(node, prefix)
 	// Add labels to the node object.
 	helper.AddLabels(node, labels)
 
@@ -231,6 +238,16 @@ func (h k8sHelpers) GetNode(cli *client.Client) (*api.Node, error) {
 	return node, nil
 }
 
+// RemoveLabels searches through all labels on Node n and removes
+// any where the key contain the search string.
+func (h k8sHelpers) RemoveLabels(n *api.Node, search string) {
+	for k := range n.Labels {
+		if strings.Contains(k, search) {
+			delete(n.Labels, k)
+		}
+	}
+}
+
 func (h k8sHelpers) AddLabels(n *api.Node, labels Labels) {
 	for k, v := range labels {
 		n.Labels[k] = v

main_test.go

@@ -56,6 +56,7 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient).Return(mockNode, nil).Once()
 			mockAPIHelper.On("AddLabels", mockNode, fakeFeatureLabels).Return().Once()
+			mockAPIHelper.On("RemoveLabels", mockNode, prefix).Return().Once()
 			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(nil).Once()
 			err := advertiseFeatureLabels(testHelper, fakeFeatureLabels)
 
@@ -89,6 +90,7 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient).Return(mockNode, nil).Once()
+			mockAPIHelper.On("RemoveLabels", mockNode, prefix).Return().Once()
 			mockAPIHelper.On("AddLabels", mockNode, fakeFeatureLabels).Return().Once()
 			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(expectedError).Once()
 			err := advertiseFeatureLabels(testHelper, fakeFeatureLabels)
@@ -100,3 +102,66 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 
 	})
 }
+
+func TestAddLabels(t *testing.T) {
+	Convey("When adding labels", t, func() {
+		helper := k8sHelpers{}
+		labels := Labels{}
+		n := &api.Node{
+			ObjectMeta: api.ObjectMeta{
+				Labels: map[string]string{},
+			},
+		}
+
+		Convey("If no labels are passed", func() {
+			helper.AddLabels(n, labels)
+
+			Convey("None should be added", func() {
+				So(len(n.Labels), ShouldEqual, 0)
+			})
+		})
+
+		Convey("They should be added to the node.Labels", func() {
+			test1 := prefix + ".test1"
+			labels[test1] = "true"
+			helper.AddLabels(n, labels)
+			So(n.Labels, ShouldContainKey, test1)
+		})
+	})
+}
+
+func TestRemoveLabels(t *testing.T) {
+	Convey("When removing labels", t, func() {
+		helper := k8sHelpers{}
+		n := &api.Node{
+			ObjectMeta: api.ObjectMeta{
+				Labels: map[string]string{
+					"single":     "123",
+					"multiple_A": "a",
+					"multiple_B": "b",
+				},
+			},
+		}
+
+		Convey("a unique label should be removed", func() {
+			helper.RemoveLabels(n, "single")
+			So(len(n.Labels), ShouldEqual, 2)
+			So(n.Labels, ShouldNotContainKey, "single")
+		})
+
+		Convey("a non-unique search string should remove all matching keys", func() {
+			helper.RemoveLabels(n, "multiple")
+			So(len(n.Labels), ShouldEqual, 1)
+			So(n.Labels, ShouldNotContainKey, "multiple_A")
+			So(n.Labels, ShouldNotContainKey, "multiple_B")
+		})
+
+		Convey("a search string with no matches should not alter labels", func() {
+			helper.RemoveLabels(n, "unique")
+			So(n.Labels, ShouldContainKey, "single")
+			So(n.Labels, ShouldContainKey, "multiple_A")
+			So(n.Labels, ShouldContainKey, "multiple_B")
+			So(len(n.Labels), ShouldEqual, 3)
+		})
+	})
+}

mockapihelpers.go

@@ -58,6 +58,12 @@ func (_m *MockAPIHelpers) GetNode(_a0 *client.Client) (*api.Node, error) {
 	return r0, r1
 }
 
+// RemoveLabels provides a mock function with *api.Node and main.Labels as the input arguments and
+// no return value
+func (_m *MockAPIHelpers) RemoveLabels(_a0 *api.Node, _a1 string) {
+	_m.Called(_a0, _a1)
+}
+
 // AddLabels provides a mock function with *api.Node and main.Labels as the input arguments and
 // no return value
 func (_m *MockAPIHelpers) AddLabels(_a0 *api.Node, _a1 Labels) {