Merge pull request #2532 from PiotrProkop/reconcile-on-spec-change

perf(nfd-master): reconcile only on NodeFeature/Rule/Group spec changes

Author: kubernetes-prow[bot]

pkg/nfd-master/nfd-api-controller.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"time"
 
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	k8sclient "k8s.io/client-go/kubernetes"
@@ -71,6 +72,22 @@ func init() {
 	utilruntime.Must(nfdv1alpha1.AddToScheme(nfdscheme.Scheme))
 }
 
+// specChanged reports whether the reconcile-relevant part of an object — its
+// .Spec — differs between the old and new versions delivered to an informer
+// UpdateFunc. Metadata-only updates (annotations, labels, ownerReferences,
+// resourceVersion), status-only updates, and resync no-ops (oldObj == newObj)
+// therefore return false, letting the handler skip an unnecessary reconcile.
+// It fails open (returns true) when the objects cannot be type-asserted, so a
+// needed reconcile is never wrongly skipped.
+func specChanged[T any](oldObj, newObj interface{}, specOf func(*T) any) bool {
+	oldT, ok1 := oldObj.(*T)
+	newT, ok2 := newObj.(*T)
+	if !ok1 || !ok2 {
+		return true
+	}
+	return !apiequality.Semantic.DeepEqual(specOf(oldT), specOf(newT))
+}
+
 func newNfdController(config *restclient.Config, nfdApiControllerOptions nfdApiControllerOptions) (*nfdController, error) {
 	c := &nfdController{
 		stopChan:                       make(chan struct{}),
@@ -133,6 +150,9 @@ func newNfdController(config *restclient.Config, nfdApiControllerOptions nfdApiC
 			}
 		},
 		UpdateFunc: func(oldObj, newObj interface{}) {
+			if !specChanged(oldObj, newObj, func(o *nfdv1alpha1.NodeFeature) any { return o.Spec }) {
+				return
+			}
 			nfr := newObj.(*nfdv1alpha1.NodeFeature)
 			klog.V(2).InfoS("NodeFeature updated", "nodefeature", klog.KObj(nfr))
 			c.updateOneNode("NodeFeature", nfr)
@@ -188,6 +208,9 @@ func newNfdController(config *restclient.Config, nfdApiControllerOptions nfdApiC
 			c.updateAllNodes()
 		},
 		UpdateFunc: func(oldObject, newObject interface{}) {
+			if !specChanged(oldObject, newObject, func(o *nfdv1alpha1.NodeFeatureRule) any { return o.Spec }) {
+				return
+			}
 			klog.V(2).InfoS("NodeFeatureRule updated", "nodefeaturerule", klog.KObj(newObject.(metav1.Object)))
 			c.updateAllNodes()
 		},
@@ -210,6 +233,9 @@ func newNfdController(config *restclient.Config, nfdApiControllerOptions nfdApiC
 				c.updateNodeFeatureGroup(nfg.Name)
 			},
 			UpdateFunc: func(oldObj, newObj interface{}) {
+				if !specChanged(oldObj, newObj, func(o *nfdv1alpha1.NodeFeatureGroup) any { return o.Spec }) {
+					return
+				}
 				nfg := newObj.(*nfdv1alpha1.NodeFeatureGroup)
 				klog.V(2).InfoS("NodeFeatureGroup updated", "nodeFeatureGroup", klog.KObj(nfg))
 				c.updateNodeFeatureGroup(nfg.Name)

pkg/nfd-master/nfd-api-controller_test.go

@@ -111,3 +111,82 @@ func TestIsNamespaceSelected(t *testing.T) {
 		assert.Equal(t, res, tc.expectedResult)
 	}
 }
+
+func TestSpecChanged(t *testing.T) {
+	nfSpec := func(o *nfdv1alpha1.NodeFeature) any { return o.Spec }
+	nfrSpec := func(o *nfdv1alpha1.NodeFeatureRule) any { return o.Spec }
+	nfgSpec := func(o *nfdv1alpha1.NodeFeatureGroup) any { return o.Spec }
+
+	// NodeFeature: Spec{Features, Labels}, no status subresource.
+	nf := &nfdv1alpha1.NodeFeature{
+		ObjectMeta: metav1.ObjectMeta{Name: "n", ResourceVersion: "1"},
+		Spec: nfdv1alpha1.NodeFeatureSpec{
+			Features: nfdv1alpha1.Features{
+				Attributes: map[string]nfdv1alpha1.AttributeFeatureSet{
+					"cpu.model": {Elements: map[string]string{"family": "6"}},
+				},
+			},
+			Labels: map[string]string{"feature.node.kubernetes.io/foo": "bar"},
+		},
+	}
+	nfMetaOnly := nf.DeepCopy()
+	nfMetaOnly.ResourceVersion = "2"
+	nfMetaOnly.Annotations = map[string]string{"nfd.node.kubernetes.io/worker.version": "v0.18"}
+	nfMetaOnly.Labels = map[string]string{nfdv1alpha1.NodeFeatureObjNodeNameLabel: "node-1"}
+
+	nfFeatureChanged := nf.DeepCopy()
+	nfFeatureChanged.Spec.Features.Attributes["cpu.model"] = nfdv1alpha1.AttributeFeatureSet{Elements: map[string]string{"family": "7"}}
+
+	nfLabelsChanged := nf.DeepCopy()
+	nfLabelsChanged.Spec.Labels["feature.node.kubernetes.io/foo"] = "baz"
+
+	// NodeFeatureRule: Spec{Rules}, no status. UpdateFunc triggers a full reconcile.
+	nfr := &nfdv1alpha1.NodeFeatureRule{
+		ObjectMeta: metav1.ObjectMeta{Name: "r", ResourceVersion: "1"},
+		Spec:       nfdv1alpha1.NodeFeatureRuleSpec{Rules: []nfdv1alpha1.Rule{{Name: "rule-1"}}},
+	}
+	nfrMetaOnly := nfr.DeepCopy()
+	nfrMetaOnly.ResourceVersion = "2"
+	nfrMetaOnly.Annotations = map[string]string{"x": "y"}
+
+	nfrSpecChanged := nfr.DeepCopy()
+	nfrSpecChanged.Spec.Rules[0].Name = "rule-2"
+
+	// NodeFeatureGroup: Spec{Rules} AND Status; master writes its status, so a
+	// status-only update must NOT reconcile (else it feeds back on itself).
+	nfg := &nfdv1alpha1.NodeFeatureGroup{
+		ObjectMeta: metav1.ObjectMeta{Name: "g", ResourceVersion: "1"},
+		Spec:       nfdv1alpha1.NodeFeatureGroupSpec{Rules: []nfdv1alpha1.GroupRule{{Name: "grule-1"}}},
+	}
+	nfgStatusOnly := nfg.DeepCopy()
+	nfgStatusOnly.ResourceVersion = "2"
+	nfgStatusOnly.Status = nfdv1alpha1.NodeFeatureGroupStatus{Nodes: []nfdv1alpha1.FeatureGroupNode{{Name: "node-1"}}}
+
+	nfgSpecChanged := nfg.DeepCopy()
+	nfgSpecChanged.Spec.Rules[0].Name = "grule-2"
+
+	testcases := []struct {
+		name string
+		got  bool
+		want bool
+	}{
+		{"NodeFeature resync no-op", specChanged(nf, nf.DeepCopy(), nfSpec), false},
+		{"NodeFeature metadata-only change", specChanged(nf, nfMetaOnly, nfSpec), false},
+		{"NodeFeature feature change", specChanged(nf, nfFeatureChanged, nfSpec), true},
+		{"NodeFeature spec.Labels change", specChanged(nf, nfLabelsChanged, nfSpec), true},
+
+		{"NodeFeatureRule metadata-only change", specChanged(nfr, nfrMetaOnly, nfrSpec), false},
+		{"NodeFeatureRule spec change", specChanged(nfr, nfrSpecChanged, nfrSpec), true},
+
+		{"NodeFeatureGroup status-only change", specChanged(nfg, nfgStatusOnly, nfgSpec), false},
+		{"NodeFeatureGroup spec change", specChanged(nfg, nfgSpecChanged, nfgSpec), true},
+
+		// Fail open: never skip a reconcile when the objects can't be compared.
+		{"nil old object", specChanged(nil, nf, nfSpec), true},
+		{"mismatched types", specChanged(&nfdv1alpha1.NodeFeatureRule{}, nf, nfSpec), true},
+	}
+
+	for _, tc := range testcases {
+		assert.Equalf(t, tc.want, tc.got, tc.name)
+	}
+}