nfd-master: stop creating NFD version annotations

We now have metrics for getting detailed information about the NFD instances running. There should be no need to pollute the node object with NFD version annotations. One problem with the annotations also that they were incomplete in the sense that they only covered nfd-master and nfd-worker but not nfd-topology-updater or nfd-gc. Also, there was a problem with stale annotations, giving misleading information. E.g. there was no way to remove old/stale master.version annotations if nfd-master was scheduled on another node where it was previously running.
2024-12-14 11:57:51 +00:00 · 2023-10-05 11:26:25 +03:00 · 2023-10-05 11:26:25 +03:00 · 1d8a83b045
commit 1d8a83b045
parent 7c0913ed7d
6 changed files with 16 additions and 36 deletions
--- a/docs/get-started/introduction.md
+++ b/docs/get-started/introduction.md
@ -102,16 +102,15 @@ NFD also annotates nodes it is running on:

 | Annotation                                                   | Description
 | ------------------------------------------------------------ | -----------
-| [&lt;instance&gt;.]nfd.node.kubernetes.io/master.version     | Version of the nfd-master instance running on the node. Informative use only.
-| [&lt;instance&gt;.]nfd.node.kubernetes.io/worker.version     | Version of the nfd-worker instance running on the node. Informative use only.
 | [&lt;instance&gt;.]nfd.node.kubernetes.io/feature-labels     | Comma-separated list of node labels managed by NFD. NFD uses this internally so must not be edited by users.
 | [&lt;instance&gt;.]nfd.node.kubernetes.io/extended-resources | Comma-separated list of node extended resources managed by NFD. NFD uses this internally so must not be edited by users.

 > **NOTE:** the [`-instance`](../reference/master-commandline-reference.md#instance)
 > command line flag affects the annotation names

-Unapplicable annotations are not created, i.e. for example master.version is
-only created on nodes running nfd-master.
+Unapplicable annotations are not created, i.e. for example
+`nfd.node.kubernetes.io/extended-resources` is only placed if some extended
+resources were created by NFD.

 ## Custom resources

--- a/pkg/apis/nfd/v1alpha1/annotations_labels.go
+++ b/pkg/apis/nfd/v1alpha1/annotations_labels.go
@ -51,6 +51,7 @@ const (
 	FeatureLabelsAnnotation = AnnotationNs + "/feature-labels"

 	// MasterVersionAnnotation is the annotation that holds the version of nfd-master running on the node
+	// DEPRECATED: will not be used in NFD v0.15 or later.
 	MasterVersionAnnotation = AnnotationNs + "/master.version"

 	// WorkerVersionAnnotation is the annotation that holds the version of nfd-worker running on the node
--- a/pkg/nfd-master/nfd-master-internal_test.go
+++ b/pkg/nfd-master/nfd-master-internal_test.go
@ -44,7 +44,6 @@ import (
 	nfdinformers "sigs.k8s.io/node-feature-discovery/pkg/generated/informers/externalversions"
 	"sigs.k8s.io/node-feature-discovery/pkg/labeler"
 	"sigs.k8s.io/node-feature-discovery/pkg/utils"
-	"sigs.k8s.io/node-feature-discovery/pkg/version"
 	"sigs.k8s.io/yaml"
 )

@ -238,8 +237,7 @@ func TestUpdateMasterNode(t *testing.T) {
 		mockClient := &k8sclient.Clientset{}
 		mockNode := newMockNode()
 		Convey("When update operation succeeds", func() {
-			expectedPatches := []apihelper.JsonPatch{
-				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.AnnotationNs+"/master.version", version.Get())}
+			expectedPatches := []apihelper.JsonPatch{}
 			mockHelper.On("GetClient").Return(mockClient, nil)
 			mockHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil)
 			mockHelper.On("PatchNode", mockClient, mockNodeName, mock.MatchedBy(jsonPatchMatcher(expectedPatches))).Return(nil)
@ -378,7 +376,6 @@ func TestSetLabels(t *testing.T) {

 		Convey("When node update succeeds", func() {
 			expectedPatches := []apihelper.JsonPatch{
-				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.WorkerVersionAnnotation, workerVer),
 				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, strings.Join(mockLabelNames, ",")),
 			}
 			for k, v := range mockLabels {
@ -397,7 +394,6 @@ func TestSetLabels(t *testing.T) {

 		Convey("When -label-whitelist is specified", func() {
 			expectedPatches := []apihelper.JsonPatch{
-				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.WorkerVersionAnnotation, workerVer),
 				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "feature-2"),
 				apihelper.NewJsonPatch("add", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/feature-2", mockLabels["feature-2"]),
 			}
@ -429,7 +425,6 @@ func TestSetLabels(t *testing.T) {
 				"--invalid-name--":               "valid-val",
 				"valid-name":                     "--invalid-val--"}
 			expectedPatches := []apihelper.JsonPatch{
-				apihelper.NewJsonPatch("add", "/metadata/annotations", instance+"."+nfdv1alpha1.WorkerVersionAnnotation, workerVer),
 				apihelper.NewJsonPatch("add", "/metadata/annotations",
 					instance+"."+nfdv1alpha1.FeatureLabelsAnnotation,
 					"feature-1,valid.ns/feature-2,"+vendorFeatureLabel+","+vendorProfileLabel),
@ -457,7 +452,6 @@ func TestSetLabels(t *testing.T) {

 		Convey("When -resource-labels is specified", func() {
 			expectedPatches := []apihelper.JsonPatch{
-				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.WorkerVersionAnnotation, workerVer),
 				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.FeatureLabelsAnnotation, "feature-2"),
 				apihelper.NewJsonPatch("add", "/metadata/annotations", nfdv1alpha1.ExtendedResourceAnnotation, "feature-1,feature-3"),
 				apihelper.NewJsonPatch("add", "/metadata/labels", nfdv1alpha1.FeatureLabelNs+"/feature-2", mockLabels["feature-2"]),
--- a/pkg/nfd-master/nfd-master.go
+++ b/pkg/nfd-master/nfd-master.go
@ -479,7 +479,10 @@ func (m *nfdMaster) prune() error {
 	return nil
 }

-// Advertise NFD master information
+// Update annotations on the node where nfd-master is running. Currently the
+// only function is to remove the deprecated
+// "nfd.node.kubernetes.io/master.version" annotation, if it exists.
+// TODO: Drop when nfdv1alpha1.MasterVersionAnnotation is removed.
 func (m *nfdMaster) updateMasterNode() error {
 	cli, err := m.apihelper.GetClient()
 	if err != nil {
@ -491,9 +494,9 @@ func (m *nfdMaster) updateMasterNode() error {
 	}

 	// Advertise NFD version as an annotation
-	p := createPatches(nil,
+	p := createPatches([]string{m.instanceAnnotation(nfdv1alpha1.MasterVersionAnnotation)},
 		node.Annotations,
-		Annotations{m.instanceAnnotation(nfdv1alpha1.MasterVersionAnnotation): version.Get()},
+		nil,
 		"/metadata/annotations")
 	err = m.apihelper.PatchNode(cli, node.Name, p)
 	if err != nil {
@ -680,8 +683,7 @@ func (m *nfdMaster) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*pb.Se
 			return &pb.SetLabelsReply{}, err
 		}

-		// Advertise NFD worker version as an annotation
-		annotations := Annotations{m.instanceAnnotation(nfdv1alpha1.WorkerVersionAnnotation): r.NfdVersion}
+		annotations := Annotations{}

 		// Create labels et al
 		if err := m.refreshNodeFeatures(cli, r.NodeName, annotations, r.GetLabels(), r.GetFeatures()); err != nil {
@ -761,13 +763,6 @@ func (m *nfdMaster) nfdAPIUpdateOneNode(nodeName string) error {
 		}

 		klog.V(4).InfoS("merged nodeFeatureSpecs", "newNodeFeatureSpec", utils.DelayedDumper(features))
-
-		if objs[0].Namespace == m.namespace && objs[0].Name == nodeName {
-			// This is the one created by nfd-worker
-			if v := objs[0].Annotations[nfdv1alpha1.WorkerVersionAnnotation]; v != "" {
-				annotations[nfdv1alpha1.WorkerVersionAnnotation] = v
-			}
-		}
 	}

 	// Update node labels et al. This may also mean removing all NFD-owned
@ -1067,7 +1062,10 @@ func (m *nfdMaster) updateNodeObject(cli *kubernetes.Clientset, nodeName string,
 		createPatches(
 			[]string{
 				m.instanceAnnotation(nfdv1alpha1.FeatureLabelsAnnotation),
-				m.instanceAnnotation(nfdv1alpha1.ExtendedResourceAnnotation)},
+				m.instanceAnnotation(nfdv1alpha1.ExtendedResourceAnnotation),
+				// Clean up deprecated/stale nfd version annotations
+				m.instanceAnnotation(nfdv1alpha1.MasterVersionAnnotation),
+				m.instanceAnnotation(nfdv1alpha1.WorkerVersionAnnotation)},
 			node.Annotations,
 			annotations,
 			"/metadata/annotations")...)
--- a/test/e2e/e2e-test-config.example.yaml
+++ b/test/e2e/e2e-test-config.example.yaml
@ -73,8 +73,6 @@ defaultFeatures:
    - "feature.node.kubernetes.io/system-os_release.VERSION_ID.major"
    - "feature.node.kubernetes.io/system-os_release.VERSION_ID.minor"
  annotationWhitelist:
-    - "nfd.node.kubernetes.io/master.version"
-    - "nfd.node.kubernetes.io/worker.version"
    - "nfd.node.kubernetes.io/feature-labels"
  nodes:
    - name: name-of-this-item  # name is purely informational
--- a/test/e2e/node_feature_discovery_test.go
+++ b/test/e2e/node_feature_discovery_test.go
@ -249,16 +249,6 @@ var _ = SIGDescribe("NFD master and worker", func() {

 				By("Waiting for the nfd-master pod to be running")
 				Expect(e2epod.WaitTimeoutForPodRunningInNamespace(ctx, f.ClientSet, masterPod.Name, masterPod.Namespace, time.Minute)).NotTo(HaveOccurred())
-
-				By("Verifying the node where nfd-master is running")
-				// Get updated masterPod object (we want to know where it was scheduled)
-				masterPod, err = f.ClientSet.CoreV1().Pods(f.Namespace.Name).Get(ctx, masterPod.Name, metav1.GetOptions{})
-				Expect(err).NotTo(HaveOccurred())
-				// Node running nfd-master should have master version annotation
-				masterPodNode, err := f.ClientSet.CoreV1().Nodes().Get(ctx, masterPod.Spec.NodeName, metav1.GetOptions{})
-				Expect(err).NotTo(HaveOccurred())
-				Expect(masterPodNode.Annotations).To(HaveKey(nfdv1alpha1.AnnotationNs + "/master.version"))
-
 				By("Waiting for the nfd-master service to be up")
 				Expect(e2enetwork.WaitForService(ctx, f.ClientSet, f.Namespace.Name, nfdSvc.Name, true, time.Second, 10*time.Second)).NotTo(HaveOccurred())
 			})