From ad6e33a4f907b2466f93e29fe4fb8fdd7d2164ce Mon Sep 17 00:00:00 2001 From: Tyler Lloyd Date: Tue, 7 Jan 2025 13:30:38 -0500 Subject: [PATCH] fix: let NNCResonciler process update events with different generations when IPAMv2 is enabled (#3279) * fix: let NNCReconciler process every update when IPAMv2 is enabled * refactor: cleanup PredicateFunc usage Group the predicate funcs into either specific event filters or the "universal" filters which get applied to every event type. Then And() them which is what controller-runtime event_handler is functionally doing anyway. link: https://github.com/kubernetes-sigs/controller-runtime/blob/aea2e32a936584b06ae6f7992f856fe7292b0297/pkg/internal/source/event_handler.go#L116 * revert: "refactor: cleanup PredicateFunc usage" This reverts commit 67983d0c7c387c53ddafeed73add7f412ed659ba. * chore: reduce refactoring * chore: just pass bool --- .../nodenetworkconfig/reconciler.go | 20 ++++++++++--------- cns/service/main.go | 9 ++++++--- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/cns/kubecontroller/nodenetworkconfig/reconciler.go b/cns/kubecontroller/nodenetworkconfig/reconciler.go index 10b252b48a..40b8fd1c05 100644 --- a/cns/kubecontroller/nodenetworkconfig/reconciler.go +++ b/cns/kubecontroller/nodenetworkconfig/reconciler.go @@ -157,7 +157,9 @@ func (r *Reconciler) Started(ctx context.Context) (bool, error) { } // SetupWithManager Sets up the reconciler with a new manager, filtering using NodeNetworkConfigFilter on nodeName. -func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, node *v1.Node) error { +// filterGenerationChange will check the old and new object's generation and only reconcile updates where the +// generation is the same. This is typically used in IPAMv1 but should be set to false in IPAMv2. +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, node *v1.Node, filterGenerationChange bool) error { r.nnccli = nodenetworkconfig.NewClient(mgr.GetClient()) err := ctrl.NewControllerManagedBy(mgr). For(&v1alpha.NodeNetworkConfig{}). @@ -166,20 +168,20 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, node *v1.Node) error { DeleteFunc: func(event.DeleteEvent) bool { return false }, - }). - WithEventFilter(predicate.NewPredicateFuncs(func(object client.Object) bool { - // match on node controller ref for all other events. - return metav1.IsControlledBy(object, node) - })). - WithEventFilter(predicate.Funcs{ - // check that the generation is the same - status changes don't update generation. UpdateFunc: func(ue event.UpdateEvent) bool { if ue.ObjectOld == nil || ue.ObjectNew == nil { return false } - return ue.ObjectOld.GetGeneration() == ue.ObjectNew.GetGeneration() + if filterGenerationChange { + return ue.ObjectOld.GetGeneration() == ue.ObjectNew.GetGeneration() + } + return true }, }). + WithEventFilter(predicate.NewPredicateFuncs(func(object client.Object) bool { + // match on node controller ref for all other events. + return metav1.IsControlledBy(object, node) + })). WithEventFilter(predicate.NewPredicateFuncs(func(object client.Object) bool { // only process events on objects that are not being deleted. return object.GetDeletionTimestamp().IsZero() diff --git a/cns/service/main.go b/cns/service/main.go index a3cd5f49ad..f242c3da84 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -1032,7 +1032,7 @@ func main() { // Start fs watcher here z.Info("AsyncPodDelete is enabled") logger.Printf("AsyncPodDelete is enabled") - cnsclient, err := cnsclient.New("", cnsReqTimeout) //nolint + cnsclient, err := cnsclient.New("", cnsReqTimeout) // nolint if err != nil { z.Error("failed to create cnsclient", zap.Error(err)) } @@ -1548,7 +1548,10 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn nodeIP := configuration.NodeIP() nncReconciler := nncctrl.NewReconciler(httpRestServiceImplementation, poolMonitor, nodeIP) // pass Node to the Reconciler for Controller xref - if err := nncReconciler.SetupWithManager(manager, node); err != nil { //nolint:govet // intentional shadow + // IPAMv1 - reconcile only status changes (where generation doesn't change). + // IPAMv2 - reconcile all updates. + filterGenerationChange := !cnsconfig.EnableIPAMv2 + if err := nncReconciler.SetupWithManager(manager, node, filterGenerationChange); err != nil { //nolint:govet // intentional shadow return errors.Wrapf(err, "failed to setup nnc reconciler with manager") } @@ -1618,7 +1621,7 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn // wait for the Reconciler to run once on a NNC that was made for this Node. // the nncReadyCtx has a timeout of 15 minutes, after which we will consider // this false and the NNC Reconciler stuck/failed, log and retry. - nncReadyCtx, cancel := context.WithTimeout(ctx, 15*time.Minute) //nolint // it will time out and not leak + nncReadyCtx, cancel := context.WithTimeout(ctx, 15*time.Minute) // nolint // it will time out and not leak if started, err := nncReconciler.Started(nncReadyCtx); !started { logger.Errorf("NNC reconciler has not started, does the NNC exist? err: %v", err) nncReconcilerStartFailures.Inc()