Decrease NNS refresh frequency and remove dyn attr (#682)

Trying to refresh NNS every 5 seconds makes nmstatectl to call python
everytime consuming CPU resources, decreasing the frequency will reduce
CPU consumption.

Also increasing the refresh period to 1 minute make CI fail on timeout,
to overcome that we force NNS refresh if a NNCP is applied.

And filter gc-timer and hello-timer hey are already not reflecting
reality since we remove them before
compare them so the value that is present at NNS is not accurate, let's
just remove them from NNS so we can use FilterOut to compare states too,
so we don't compare filtered stuff like veth or calico.

Proper solution is to use varlink so python is already started up at
different container, this is done at #663.

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

controllers/labels.go

@@ -0,0 +1,5 @@
+package controllers
+
+const (
+	forceRefreshLabel = "nmstate.io/force-nns-refresh"
+)

controllers/node_controller.go

@@ -30,6 +30,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
+	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
 	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
 	"github.com/nmstate/kubernetes-nmstate/pkg/node"
@@ -38,15 +39,15 @@ import (
 )
 
 // Added for test purposes
-type NmstateUpdater func(client client.Client, node *corev1.Node, namespace client.ObjectKey, observedStateRaw string) error
+type NmstateUpdater func(client client.Client, node *corev1.Node, namespace client.ObjectKey, observedState shared.State) error
 type NmstatectlShow func() (string, error)
 
 // NodeReconciler reconciles a Node object
 type NodeReconciler struct {
 	client.Client
 	Log            logr.Logger
 	Scheme         *runtime.Scheme
-	lastState      string
+	lastState      shared.State
 	nmstateUpdater NmstateUpdater
 	nmstatectlShow NmstatectlShow
 }
@@ -57,14 +58,19 @@ 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) {
-	currentState, err := r.nmstatectlShow()
+	currentStateRaw, err := r.nmstatectlShow()
 	if err != nil {
 		// We cannot call nmstatectl show let's reconcile again
 		return ctrl.Result{}, err
 	}
 
+	currentState, err := state.FilterOut(shared.NewState(currentStateRaw))
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
 	// Reduce apiserver hits by checking node's network state with last one
-	if r.lastState != "" && !r.networkStateChanged(currentState) {
+	if r.lastState.String() == currentState.String() {
 		return ctrl.Result{RequeueAfter: node.NetworkStateRefresh}, err
 	} else {
 		r.Log.Info("Network configuration changed, updating NodeNetworkState")
@@ -122,7 +128,3 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		WithEventFilter(onCreationForThisNode).
 		Complete(r)
 }
-
-func (r *NodeReconciler) networkStateChanged(currentState string) bool {
-	return state.RemoveDynamicAttributes(r.lastState) != state.RemoveDynamicAttributes(currentState)
-}

controllers/node_controller_test.go

@@ -19,17 +19,21 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
+	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
 	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
 	nmstatenode "github.com/nmstate/kubernetes-nmstate/pkg/node"
+	"github.com/nmstate/kubernetes-nmstate/pkg/state"
 )
 
 var _ = Describe("Node controller reconcile", func() {
 	var (
-		cl               client.Client
-		reconciler       NodeReconciler
-		existingNodeName = "node01"
-		node             = corev1.Node{
+		cl                       client.Client
+		reconciler               NodeReconciler
+		observedState            string
+		filteredOutObservedState shared.State
+		existingNodeName         = "node01"
+		node                     = corev1.Node{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: existingNodeName,
 				UID:  "12345",
@@ -58,9 +62,23 @@ var _ = Describe("Node controller reconcile", func() {
 		reconciler.Scheme = s
 		reconciler.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
 		reconciler.nmstatectlShow = nmstatectl.Show
-		reconciler.lastState = "lastState"
+		reconciler.lastState = shared.NewState("lastState")
+		observedState = `
+---
+interfaces:
+  - name: eth1
+    type: ethernet
+    state: up
+routes:
+  running: []
+  config: []
+`
+		var err error
+		filteredOutObservedState, err = state.FilterOut(shared.NewState(observedState))
+		Expect(err).ToNot(HaveOccurred())
+
 		reconciler.nmstatectlShow = func() (string, error) {
-			return "currentState", nil
+			return observedState, nil
 		}
 	})
 	Context("and nmstatectl show is failing", func() {
@@ -82,13 +100,13 @@ var _ = Describe("Node controller reconcile", func() {
 			request reconcile.Request
 		)
 		BeforeEach(func() {
-			reconciler.lastState = "currentState"
+			reconciler.lastState = filteredOutObservedState
 			reconciler.nmstateUpdater = func(client.Client, *corev1.Node,
-				client.ObjectKey, string) error {
+				client.ObjectKey, shared.State) error {
 				return fmt.Errorf("we are not suppose to catch this error")
 			}
 		})
-		It("should return a Result with RequeueAfter set", func() {
+		It("should not call nmstateUpdater and 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}))
@@ -114,19 +132,42 @@ var _ = Describe("Node controller reconcile", func() {
 		BeforeEach(func() {
 			request.Name = existingNodeName
 		})
-		Context("and nodenetworkstate is there too", func() {
-			AfterEach(func() {
-				reconciler.nmstateUpdater = nmstate.CreateOrUpdateNodeNetworkState
-			})
-			It("should return a Result with RequeueAfter set (trigger re-reconciliation)", func() {
-				// Mocking nmstatectl.Show
-				reconciler.nmstateUpdater = func(client client.Client, node *corev1.Node,
-					namespace client.ObjectKey, observedStateRaw string) error {
-					return nil
+		Context(", nodenetworkstate is there too with last state and observed state is different", func() {
+			var (
+				expectedStateRaw = `---
+interfaces:
+  - name: eth1
+    type: ethernet
+    state: up
+  - name: eth2
+    type: ethernet
+    state: up
+routes:
+  running: []
+  config: []
+`
+			)
+			BeforeEach(func() {
+				By("Create the NNS with last state")
+				err := reconciler.nmstateUpdater(cl, &node, types.NamespacedName{Name: node.Name}, filteredOutObservedState)
+				Expect(err).ToNot(HaveOccurred())
+
+				By("Mock nmstate show so we return different value from last state")
+				reconciler.nmstatectlShow = func() (string, error) {
+					return expectedStateRaw, nil
 				}
+
+			})
+			It("should call nmstateUpdater and 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: nmstatenode.NetworkStateRefresh}))
+				obtainedNNS := nmstatev1beta1.NodeNetworkState{}
+				err = cl.Get(context.TODO(), types.NamespacedName{Name: existingNodeName}, &obtainedNNS)
+				Expect(err).ToNot(HaveOccurred())
+				filteredOutExpectedState, err := state.FilterOut(shared.NewState(expectedStateRaw))
+				Expect(err).ToNot(HaveOccurred())
+				Expect(obtainedNNS.Status.CurrentState.String()).To(Equal(filteredOutExpectedState.String()))
 			})
 		})
 		Context("and nodenetworkstate is not there", func() {

controllers/nodenetworkconfigurationpolicy_controller.go

@@ -281,6 +281,8 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(request ctrl.Reques
 
 	enactmentConditions.NotifySuccess()
 
+	r.forceNNSRefresh(nodeName)
+
 	return ctrl.Result{}, nil
 }
 
@@ -326,6 +328,26 @@ func (r *NodeNetworkConfigurationPolicyReconciler) SetupWithManager(mgr ctrl.Man
 	return nil
 }
 
+func (r *NodeNetworkConfigurationPolicyReconciler) forceNNSRefresh(name string) {
+	log := r.Log.WithName("forceNNSRefresh").WithValues("node", name)
+	log.Info("forcing NodeNetworkState refresh after NNCP applied")
+	nns := &nmstatev1beta1.NodeNetworkState{}
+	err := r.Client.Get(context.TODO(), types.NamespacedName{Name: name}, nns)
+	if err != nil {
+		log.WithValues("error", err).Info("WARNING: failed retrieving NodeNetworkState to force refresh, it will be refreshed after regular period")
+		return
+	}
+	if nns.Labels == nil {
+		nns.Labels = map[string]string{}
+	}
+	nns.Labels[forceRefreshLabel] = fmt.Sprintf("%d", time.Now().UnixNano())
+
+	err = r.Client.Update(context.Background(), nns)
+	if err != nil {
+		log.WithValues("error", err).Info("WARNING: failed forcing NNS refresh, it will be refreshed after regular period")
+	}
+}
+
 func desiredState(object runtime.Object) (nmstateapi.State, error) {
 	var state nmstateapi.State
 	switch v := object.(type) {

controllers/nodenetworkstate_controller.go

@@ -30,9 +30,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
+	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
 	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
 	"github.com/nmstate/kubernetes-nmstate/pkg/nmstatectl"
+	"github.com/nmstate/kubernetes-nmstate/pkg/state"
 	corev1 "k8s.io/api/core/v1"
 )
 
@@ -63,12 +65,17 @@ func (r *NodeNetworkStateReconciler) Reconcile(request ctrl.Request) (ctrl.Resul
 		return ctrl.Result{}, err
 	}
 
-	currentState, err := nmstatectl.Show()
+	currentStateRaw, err := nmstatectl.Show()
 	if err != nil {
 		// We cannot call nmstatectl show let's reconcile again
 		return ctrl.Result{}, err
 	}
 
+	currentState, err := state.FilterOut(shared.NewState(currentStateRaw))
+	if err != nil {
+		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")
@@ -89,8 +96,9 @@ func (r *NodeNetworkStateReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		DeleteFunc: func(deleteEvent event.DeleteEvent) bool {
 			return nmstate.EventIsForThisNode(deleteEvent.Meta)
 		},
-		UpdateFunc: func(event.UpdateEvent) bool {
-			return false
+		UpdateFunc: func(updateEvent event.UpdateEvent) bool {
+			return nmstate.EventIsForThisNode(updateEvent.MetaNew) &&
+				shouldForceRefresh(updateEvent)
 		},
 		GenericFunc: func(event.GenericEvent) bool {
 			return false
@@ -102,3 +110,15 @@ func (r *NodeNetworkStateReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		WithEventFilter(onDeleteForThisNode).
 		Complete(r)
 }
+
+func shouldForceRefresh(updateEvent event.UpdateEvent) bool {
+	newForceRefresh, hasForceRefreshNow := updateEvent.MetaNew.GetLabels()[forceRefreshLabel]
+	if !hasForceRefreshNow {
+		return false
+	}
+	oldForceRefresh, hasForceRefreshLabelPreviously := updateEvent.MetaOld.GetLabels()[forceRefreshLabel]
+	if !hasForceRefreshLabelPreviously {
+		return true
+	}
+	return oldForceRefresh != newForceRefresh
+}

go.mod

@@ -4,6 +4,7 @@ go 1.15
 
 require (
 	github.com/Masterminds/semver v1.5.0
+	github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883
 	github.com/evanphx/json-patch v4.9.0+incompatible
 	github.com/github-release/github-release v0.8.1
 	github.com/go-logr/logr v0.1.0

go.sum

@@ -127,6 +127,7 @@ github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRF
 github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d h1:UQZhZ2O0vMHr2cI+DC1Mbh0TJxzA3RcLoMsFw+aXw7E=
 github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho=
 github.com/aliyun/aliyun-oss-go-sdk v2.0.4+incompatible/go.mod h1:T/Aws4fEfogEE9v+HPhhw+CntffsBHJ8nXQCwKr0/g8=
+github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 h1:bvNMNQO63//z+xNgfBlViaCIJKLlCJ6/fmUseuG0wVQ=
 github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8=
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 h1:kFOfPq6dUM1hTo4JG6LR5AXSUEsOjtdm0kw0FtQtMJA=
 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239/go.mod h1:2FmKhYUyUczH0OGQWaF5ceTx0UBShxjsH6f8oGKYe2c=