Reduce calls to apiserver at nns updates (#664)

The node controller refresh NNSs every 5 seconds at every Reconcile it
do three things, GET Node, call nmstatectl show and UPDATE NNS, this is
too much of apiserver traffic and end up with big cpu usage. This change
reduce the calls to GET and UPDATE by comparing current network state
with last retrieved network state and if they differ then it do them.

Signed-off-by: Quique Llorente <[email protected]>

automation/check-patch.e2e-k8s.sh

@@ -33,7 +33,7 @@ main() {
     make cluster-sync
 
     export E2E_TEST_SUITE_ARGS="--junit-output=$ARTIFACTS/junit.functest.xml"
-    if [ $NMSTATE_PARALLEL_ROLLOUT == "true" ]; then
+    if [ "$NMSTATE_PARALLEL_ROLLOUT" == "true" ]; then
        E2E_TEST_SUITE_ARGS="${E2E_TEST_SUITE_ARGS} -ginkgo.skip='user-guide|nns|sequential'"
     else
        E2E_TEST_SUITE_ARGS="${E2E_TEST_SUITE_ARGS} -ginkgo.skip='parallel'"

controllers/node_controller.go

@@ -19,7 +19,7 @@ package controllers
 
 import (
 	"context"
-	"time"
+	"regexp"
 
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
@@ -32,22 +32,27 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
 	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
+	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
+	"github.com/nmstate/kubernetes-nmstate/pkg/node"
 	corev1 "k8s.io/api/core/v1"
 )
 
 // Added for test purposes
-type NmstateUpdater func(client client.Client, node *corev1.Node, namespace client.ObjectKey) error
+type NmstateUpdater func(client client.Client, node *corev1.Node, namespace client.ObjectKey, observedStateRaw string) error
+type NmstatectlShow func() (string, error)
 
 var (
-	nodeRefresh    = 5 * time.Second
-	nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
+	gcTimerRexp = regexp.MustCompile(` *gc-timer: *[0-9]*\n`)
 )
 
 // NodeReconciler reconciles a Node object
 type NodeReconciler struct {
 	client.Client
-	Log    logr.Logger
-	Scheme *runtime.Scheme
+	Log            logr.Logger
+	Scheme         *runtime.Scheme
+	lastState      string
+	nmstateUpdater NmstateUpdater
+	nmstatectlShow NmstatectlShow
 }
 
 // Reconcile reads that state of the cluster for a Node object and makes changes based on the state read
@@ -56,12 +61,22 @@ type NodeReconciler struct {
 // The Controller will requeue the Request to be processed again if the returned error is non-nil or
 // Result.Requeue is true, otherwise upon completion it will remove the work from the queue.
 func (r *NodeReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
-	_ = context.Background()
-	_ = r.Log.WithValues("node", request.NamespacedName)
+	currentState, err := r.nmstatectlShow()
+	if err != nil {
+		// We cannot call nmstatectl show let's reconcile again
+		return ctrl.Result{}, err
+	}
+
+	// Reduce apiserver hits by checking node's network state with last one
+	if r.lastState != "" && !r.networkStateChanged(currentState) {
+		return ctrl.Result{RequeueAfter: node.NetworkStateRefresh}, err
+	} else {
+		r.Log.Info("Network configuration changed, updating NodeNetworkState")
+	}
 
 	// Fetch the Node instance
 	instance := &corev1.Node{}
-	err := r.Client.Get(context.TODO(), request.NamespacedName, instance)
+	err = r.Client.Get(context.TODO(), request.NamespacedName, instance)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
 			// Request object not found, could have been deleted after reconcile request.
@@ -72,15 +87,23 @@ func (r *NodeReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		// Error reading the object - requeue the request.
 		return ctrl.Result{}, err
 	}
-	err = nmstateUpdater(r.Client, instance, request.NamespacedName)
+	err = r.nmstateUpdater(r.Client, instance, request.NamespacedName, currentState)
 	if err != nil {
 		err = errors.Wrap(err, "error at node reconcile creating NodeNetworkState")
+		return ctrl.Result{}, err
 	}
-	return ctrl.Result{RequeueAfter: nodeRefresh}, err
+
+	// Cache currentState after successfully storing it at NodeNetworkState
+	r.lastState = currentState
+
+	return ctrl.Result{RequeueAfter: node.NetworkStateRefresh}, nil
 }
 
 func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager) error {
 
+	r.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
+	r.nmstatectlShow = nmstatectl.Show
+
 	// By default all this functors return true so controller watch all events,
 	// but we only want to watch create/delete for current node.
 	onCreationForThisNode := predicate.Funcs{
@@ -103,3 +126,12 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		WithEventFilter(onCreationForThisNode).
 		Complete(r)
 }
+
+func (r *NodeReconciler) networkStateChanged(currentState string) bool {
+	return removeDynamicAttributes(r.lastState) != removeDynamicAttributes(currentState)
+}
+
+func removeDynamicAttributes(state string) string {
+	// Remove attributes that make network state always different
+	return gcTimerRexp.ReplaceAllLiteralString(state, "")
+}

controllers/node_controller_test.go

@@ -2,6 +2,7 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 
 	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
 
@@ -19,6 +20,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
+	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
+	nmstatenode "github.com/nmstate/kubernetes-nmstate/pkg/node"
 )
 
 var _ = Describe("Node controller reconcile", func() {
@@ -39,6 +42,7 @@ var _ = Describe("Node controller reconcile", func() {
 		}
 	)
 	BeforeEach(func() {
+		reconciler = NodeReconciler{}
 		s := scheme.Scheme
 		s.AddKnownTypes(nmstatev1beta1.GroupVersion,
 			&nmstatev1beta1.NodeNetworkState{},
@@ -52,7 +56,43 @@ var _ = Describe("Node controller reconcile", func() {
 		reconciler.Client = cl
 		reconciler.Log = ctrl.Log.WithName("controllers").WithName("Node")
 		reconciler.Scheme = s
-		nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
+		reconciler.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
+		reconciler.nmstatectlShow = nmstatectl.Show
+		reconciler.lastState = "lastState"
+		reconciler.nmstatectlShow = func() (string, error) {
+			return "currentState", nil
+		}
+	})
+	Context("and nmstatectl show is failing", func() {
+		var (
+			request reconcile.Request
+		)
+		BeforeEach(func() {
+			reconciler.nmstatectlShow = func() (string, error) {
+				return "", fmt.Errorf("forced failure at unit test")
+			}
+		})
+		It("should return the error from nmstatectl", func() {
+			_, err := reconciler.Reconcile(request)
+			Expect(err).To(MatchError("forced failure at unit test"))
+		})
+	})
+	Context("and network state didn't change", func() {
+		var (
+			request reconcile.Request
+		)
+		BeforeEach(func() {
+			reconciler.lastState = "currentState"
+			reconciler.nmstateUpdater = func(client.Client, *corev1.Node,
+				client.ObjectKey, string) error {
+				return fmt.Errorf("we are not suppose to catch this error")
+			}
+		})
+		It("should return a Result with RequeueAfter set", func() {
+			result, err := reconciler.Reconcile(request)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(result).To(Equal(reconcile.Result{RequeueAfter: nmstatenode.NetworkStateRefresh}))
+		})
 	})
 	Context("when node is not found", func() {
 		var (
@@ -76,17 +116,17 @@ var _ = Describe("Node controller reconcile", func() {
 		})
 		Context("and nodenetworkstate is there too", func() {
 			AfterEach(func() {
-				nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
+				reconciler.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
 			})
 			It("should return a Result with RequeueAfter set (trigger re-reconciliation)", func() {
 				// Mocking nmstatectl.Show
-				nmstateUpdater = func(client client.Client, node *corev1.Node,
-					namespace client.ObjectKey) error {
+				reconciler.nmstateUpdater = func(client client.Client, node *corev1.Node,
+					namespace client.ObjectKey, observedStateRaw string) error {
 					return nil
 				}
 				result, err := reconciler.Reconcile(request)
 				Expect(err).ToNot(HaveOccurred())
-				Expect(result).To(Equal(reconcile.Result{RequeueAfter: nodeRefresh}))
+				Expect(result).To(Equal(reconcile.Result{RequeueAfter: nmstatenode.NetworkStateRefresh}))
 			})
 		})
 		Context("and nodenetworkstate is not there", func() {
@@ -112,7 +152,7 @@ var _ = Describe("Node controller reconcile", func() {
 			It("should return a Result with RequeueAfter set (trigger re-reconciliation)", func() {
 				result, err := reconciler.Reconcile(request)
 				Expect(err).ToNot(HaveOccurred())
-				Expect(result).To(Equal(reconcile.Result{RequeueAfter: nodeRefresh}))
+				Expect(result).To(Equal(reconcile.Result{RequeueAfter: nmstatenode.NetworkStateRefresh}))
 			})
 		})
 	})

controllers/nodenetworkstate_controller.go

@@ -0,0 +1,104 @@
+/*
+Copyright The Kubernetes NMState Authors.
+
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+
+	"github.com/go-logr/logr"
+	"github.com/pkg/errors"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
+
+	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
+	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
+	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
+	corev1 "k8s.io/api/core/v1"
+)
+
+// NodeNetworkStateReconciler reconciles a NodeNetworkState object
+type NodeNetworkStateReconciler struct {
+	client.Client
+	Log    logr.Logger
+	Scheme *runtime.Scheme
+}
+
+// Reconcile reads that state of the cluster for a NodeNetworkState object and makes changes based on the state read
+// and what is in the NodeNetworkState.Spec
+// Note:
+// The Controller will requeue the Request to be processed again if the returned error is non-nil or
+// Result.Requeue is true, otherwise upon completion it will remove the work from the queue.
+func (r *NodeNetworkStateReconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
+	// Fetch the Node instance
+	node := &corev1.Node{}
+	err := r.Client.Get(context.TODO(), request.NamespacedName, node)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			// Node is gone the NodeNetworkState delete event has being
+			// triggered by k8s garbage collector we don't need to
+			// re-create the NodeNetworkState
+			return ctrl.Result{}, nil
+		}
+		// Error reading the object - requeue the request.
+		return ctrl.Result{}, err
+	}
+
+	currentState, err := nmstatectl.Show()
+	if err != nil {
+		// We cannot call nmstatectl show let's reconcile again
+		return ctrl.Result{}, err
+	}
+
+	nmstate.CreateOrUpdateNodeNetworkState(r.Client, node, request.NamespacedName, currentState)
+	if err != nil {
+		err = errors.Wrap(err, "error at node reconcile creating NodeNetworkStateNetworkState")
+		return ctrl.Result{}, err
+	}
+
+	return ctrl.Result{}, nil
+}
+
+func (r *NodeNetworkStateReconciler) SetupWithManager(mgr ctrl.Manager) error {
+
+	// By default all this functors return true so controller watch all events,
+	// but we only want to watch delete for current node.
+	onDeleteForThisNode := predicate.Funcs{
+		CreateFunc: func(event.CreateEvent) bool {
+			return false
+		},
+		DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
+			return nmstate.EventIsForThisNode(deleteEvent.Meta)
+		},
+		UpdateFunc: func(event.UpdateEvent) bool {
+			return false
+		},
+		GenericFunc: func(event.GenericEvent) bool {
+			return false
+		},
+	}
+
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&nmstatev1beta1.NodeNetworkState{}).
+		WithEventFilter(onDeleteForThisNode).
+		Complete(r)
+}

main.go

@@ -158,6 +158,14 @@ func main() {
 			setupLog.Error(err, "unable to create NodeNetworkConfigurationPolicy controller", "controller", "NMState")
 			os.Exit(1)
 		}
+		if err = (&controllers.NodeNetworkStateReconciler{
+			Client: mgr.GetClient(),
+			Log:    ctrl.Log.WithName("controllers").WithName("NodeNetworkState"),
+			Scheme: mgr.GetScheme(),
+		}).SetupWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create NodeNetworkState controller", "controller", "NMState")
+			os.Exit(1)
+		}
 	}
 
 	setProfiler()

pkg/helper/client.go

@@ -72,7 +72,7 @@ func GetNodeNetworkState(client client.Client, nodeName string) (nmstatev1beta1.
 	return nodeNetworkState, err
 }
 
-func InitializeNodeNetworkState(client client.Client, node *corev1.Node) error {
+func InitializeNodeNetworkState(client client.Client, node *corev1.Node) (*nmstatev1beta1.NodeNetworkState, error) {
 	ownerRefList := []metav1.OwnerReference{{Name: node.ObjectMeta.Name, Kind: "Node", APIVersion: "v1", UID: node.UID}}
 
 	nodeNetworkState := nmstatev1beta1.NodeNetworkState{
@@ -85,35 +85,34 @@ func InitializeNodeNetworkState(client client.Client, node *corev1.Node) error {
 
 	err := client.Create(context.TODO(), &nodeNetworkState)
 	if err != nil {
-		return fmt.Errorf("error creating NodeNetworkState: %v, %+v", err, nodeNetworkState)
+		return nil, fmt.Errorf("error creating NodeNetworkState: %v, %+v", err, nodeNetworkState)
 	}
 
-	return nil
+	return &nodeNetworkState, nil
 }
 
-func CreateOrUpdateNodeNetworkState(client client.Client, node *corev1.Node, namespace client.ObjectKey) error {
+func CreateOrUpdateNodeNetworkState(client client.Client, node *corev1.Node, namespace client.ObjectKey, observedStateRaw string) error {
 	nnsInstance := &nmstatev1beta1.NodeNetworkState{}
 	err := client.Get(context.TODO(), namespace, nnsInstance)
 	if err != nil {
 		if !apierrors.IsNotFound(err) {
 			return errors.Wrap(err, "Failed to get nmstate")
 		} else {
-			return InitializeNodeNetworkState(client, node)
+			nnsInstance, err = InitializeNodeNetworkState(client, node)
+			if err != nil {
+				return err
+			}
 		}
 	}
-	return UpdateCurrentState(client, nnsInstance)
+	return UpdateCurrentState(client, nnsInstance, observedStateRaw)
 }
 
-func UpdateCurrentState(client client.Client, nodeNetworkState *nmstatev1beta1.NodeNetworkState) error {
-	observedStateRaw, err := nmstatectl.Show()
-	if err != nil {
-		return errors.Wrap(err, "error running nmstatectl show")
-	}
+func UpdateCurrentState(client client.Client, nodeNetworkState *nmstatev1beta1.NodeNetworkState, observedStateRaw string) error {
 	observedState := nmstate.State{Raw: []byte(observedStateRaw)}
 
 	stateToReport, err := filterOut(observedState, interfacesFilterGlob)
 	if err != nil {
-		fmt.Printf("failed filtering out interfaces from NodeNetworkState, keeping orignal content, please fix the glob: %v", err)
+		log.Error(err, "failed filtering out interfaces from NodeNetworkState, keeping orignal content, please fix the glob")
 		stateToReport = observedState
 	}