Fixing node events propagated to the module reconciler

Currently module reconciler is watching for events on the Node and
skips deletions, creations, generics event. On the Update event is looks
for labels' changes or that the node becomes schedulable based on the
taints. Since filter does not know the tolerations of the Modules, all
this code actually does is check if tolerations exist, then check if if
tolerations allow scheduling and then checking if there is a difference
between new and old conditions in schelabilty of those conditions, and
only in that case the module reconciler is scheduled. This means that
when the node rebooted, and the node is tainted with the taints whose
effect is notSchedule, the module reconciler will never be kicked in,
even though there is a Module has a matching toleration.
This commit changes the condition for the Module reconciler to be
scheduled: no module reconciler will be scheduled in case there is a
change in labels, or a change in taints. This means that as the node
progesses from NotReady to Ready and visa-versa, the Module reconciler
will be notified

Author: yevgeny-shnaidman

internal/filter/filter.go

@@ -3,7 +3,6 @@ package filter
 import (
 	"context"
 	"github.com/go-logr/logr"
-	"github.com/kubernetes-sigs/kernel-module-management/internal/node"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -37,7 +36,7 @@ var skipCreations predicate.Predicate = predicate.Funcs{
 	CreateFunc: func(_ event.CreateEvent) bool { return false },
 }
 
-var nodeBecomesSchedulable predicate.Predicate = predicate.Funcs{
+var nodeTaintsChanged predicate.Predicate = predicate.Funcs{
 	UpdateFunc: func(e event.UpdateEvent) bool {
 		oldNode, ok := e.ObjectOld.(*v1.Node)
 		if !ok {
@@ -49,10 +48,7 @@ var nodeBecomesSchedulable predicate.Predicate = predicate.Funcs{
 			return false
 		}
 
-		n := node.NewNode(nil)
-		isOldSchedulable := n.IsNodeSchedulable(oldNode, nil)
-		isNewSchedulable := n.IsNodeSchedulable(newNode, nil)
-		if isOldSchedulable != isNewSchedulable && isNewSchedulable {
+		if !reflect.DeepEqual(newNode.Spec.Taints, oldNode.Spec.Taints) {
 			return true
 		}
 
@@ -117,14 +113,6 @@ func New(client client.Client, nmcHelper nmc.Helper) *Filter {
 	}
 }
 
-func (f *Filter) ModuleReconcilerNodePredicate(kernelLabel string) predicate.Predicate {
-	return predicate.And(
-		skipDeletions,
-		HasLabel(kernelLabel),
-		predicate.Or(nodeBecomesSchedulable, predicate.LabelChangedPredicate{}),
-	)
-}
-
 func ListModulesForNMC(_ context.Context, obj client.Object) []reconcile.Request {
 	modules := sets.New[reconcile.Request]()
 
@@ -195,7 +183,7 @@ func NMCReconcilerNodePredicate() predicate.Predicate {
 func ModuleReconcilerNodePredicate() predicate.Predicate {
 	return predicate.And(
 		skipDeletions,
-		predicate.Or(nodeBecomesSchedulable, predicate.LabelChangedPredicate{}),
+		predicate.Or(nodeTaintsChanged, predicate.LabelChangedPredicate{}),
 	)
 }
 

internal/filter/filter_test.go

@@ -83,36 +83,6 @@ var _ = Describe("skipCreations", func() {
 	})
 })
 
-var _ = Describe("nodeBecomesSchedulable", func() {
-	DescribeTable("should work as expected", func(oldNodeSchedulable, newNodeSchedulable, expectedRes bool) {
-		oldNode := v1.Node{}
-		newNode := v1.Node{}
-		if !oldNodeSchedulable {
-			oldNode.Spec.Taints = []v1.Taint{
-				v1.Taint{
-					Effect: v1.TaintEffectNoSchedule,
-				},
-			}
-		}
-		if !newNodeSchedulable {
-			newNode.Spec.Taints = []v1.Taint{
-				v1.Taint{
-					Effect: v1.TaintEffectNoSchedule,
-				},
-			}
-		}
-
-		res := nodeBecomesSchedulable.Update(event.UpdateEvent{ObjectOld: &oldNode, ObjectNew: &newNode})
-		Expect(res).To(Equal(expectedRes))
-
-	},
-		Entry("old schedulable, new schedulable", true, true, false),
-		Entry("old schedulable, new non-schedulable", true, false, false),
-		Entry("old non-schedulable, new non-schedulable", false, false, false),
-		Entry("old non-schedulable, new schedulable", false, true, true),
-	)
-})
-
 var _ = Describe("filterRelevantNodeUpdates", func() {
 
 	It("should return false if there's no relevant change", func() {
@@ -338,21 +308,15 @@ var _ = Describe("ListModulesForNMC", func() {
 })
 
 var _ = Describe("ModuleReconcilerNodePredicate", func() {
-	const kernelLabel = "kernel-label"
 	var p predicate.Predicate
 
 	BeforeEach(func() {
-		f = New(nil, nil)
-		p = f.ModuleReconcilerNodePredicate(kernelLabel)
+		p = ModuleReconcilerNodePredicate()
 	})
 
 	It("should return true for creations", func() {
 		ev := event.CreateEvent{
-			Object: &v1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels: map[string]string{kernelLabel: "1.2.3"},
-				},
-			},
+			Object: &v1.Node{},
 		}
 
 		Expect(
@@ -367,7 +331,7 @@ var _ = Describe("ModuleReconcilerNodePredicate", func() {
 			ObjectOld: &v1.Node{},
 			ObjectNew: &v1.Node{
 				ObjectMeta: metav1.ObjectMeta{
-					Labels: map[string]string{kernelLabel: "1.2.3"},
+					Labels: map[string]string{"some label": "some value"},
 				},
 			},
 		}
@@ -378,34 +342,9 @@ var _ = Describe("ModuleReconcilerNodePredicate", func() {
 			BeTrue(),
 		)
 	})
-	It("should return false for label updates without the expected label", func() {
-		ev := event.UpdateEvent{
-			ObjectOld: &v1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels: map[string]string{"a": "b"},
-				},
-			},
-			ObjectNew: &v1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels: map[string]string{"c": "d"},
-				},
-			},
-		}
-
-		Expect(
-			p.Update(ev),
-		).To(
-			BeFalse(),
-		)
-	})
-
 	It("should return false for deletions", func() {
 		ev := event.DeleteEvent{
-			Object: &v1.Node{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels: map[string]string{kernelLabel: "1.2.3"},
-				},
-			},
+			Object: &v1.Node{},
 		}
 
 		Expect(
@@ -880,7 +819,7 @@ var _ = Describe("FindPreflightsForModule", func() {
 
 })
 
-var _ = Describe("nodeBecomesSchedulable", func() {
+var _ = Describe("nodeTaintsChanged", func() {
 	var (
 		oldNode v1.Node
 		newNode *v1.Node
@@ -900,9 +839,16 @@ var _ = Describe("nodeBecomesSchedulable", func() {
 		newNode = oldNode.DeepCopy()
 	})
 
-	nonSchedulableTaint := v1.Taint{
+	taint1 := v1.Taint{
+		Key:    "key1",
+		Value:  "value1",
 		Effect: v1.TaintEffectNoSchedule,
 	}
+	taint2 := v1.Taint{
+		Key:    "key2",
+		Value:  "value2",
+		Effect: v1.TaintEffectPreferNoSchedule,
+	}
 
 	DescribeTable("Should return the expected value", func(oldTaint *v1.Taint, newTaint *v1.Taint, expected bool) {
 		if newTaint != nil {
@@ -912,16 +858,15 @@ var _ = Describe("nodeBecomesSchedulable", func() {
 			oldNode.Spec.Taints = append(oldNode.Spec.Taints, *oldTaint)
 		}
 
-		res := nodeBecomesSchedulable.Update(event.UpdateEvent{ObjectOld: &oldNode, ObjectNew: newNode})
+		res := nodeTaintsChanged.Update(event.UpdateEvent{ObjectOld: &oldNode, ObjectNew: newNode})
 		if expected {
 			Expect(res).To(BeTrue())
 		} else {
 			Expect(res).To(BeFalse())
 		}
 	},
-		Entry("old Schedulable, new Schedulable", nil, nil, false),
-		Entry("old NonSchedulable, new NonSchedulable", &nonSchedulableTaint, &nonSchedulableTaint, false),
-		Entry("old Schedulable, new NonSchedulable", nil, &nonSchedulableTaint, false),
-		Entry("old NonSchedulable, new Schedulable", &nonSchedulableTaint, nil, true),
+		Entry("no old taints, no new taints", nil, nil, false),
+		Entry("taints are different between old and new", &taint1, &taint2, true),
+		Entry("taints are equal between old and new", &taint2, &taint2, false),
 	)
 })