diff --git a/automation/check-patch.e2e-k8s.sh b/automation/check-patch.e2e-k8s.sh index 6616565939..3513263797 100755 --- a/automation/check-patch.e2e-k8s.sh +++ b/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'" diff --git a/controllers/node_controller.go b/controllers/node_controller.go index f266ea5de0..690dd34a02 100644 --- a/controllers/node_controller.go +++ b/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, "") +} diff --git a/controllers/node_controller_test.go b/controllers/node_controller_test.go index 3ba69466c4..60d6271ab8 100644 --- a/controllers/node_controller_test.go +++ b/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})) }) }) }) diff --git a/controllers/nodenetworkstate_controller.go b/controllers/nodenetworkstate_controller.go new file mode 100644 index 0000000000..62b48643f3 --- /dev/null +++ b/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) +} diff --git a/main.go b/main.go index 81f0ac00ca..03f26cb98d 100644 --- a/main.go +++ b/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() diff --git a/pkg/helper/client.go b/pkg/helper/client.go index 0c414505a6..7c3423080a 100644 --- a/pkg/helper/client.go +++ b/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 } diff --git a/pkg/nmstatectl/nmstatectl.go b/pkg/nmstatectl/nmstatectl.go index 89c3ea59ff..70ed854377 100644 --- a/pkg/nmstatectl/nmstatectl.go +++ b/pkg/nmstatectl/nmstatectl.go @@ -52,7 +52,7 @@ func nmstatectl(arguments []string) (string, error) { return nmstatectlWithInput(arguments, "") } -func Show(arguments ...string) (string, error) { +func Show() (string, error) { return nmstatectl([]string{"show"}) } diff --git a/pkg/node/constants.go b/pkg/node/constants.go new file mode 100644 index 0000000000..d27190e54a --- /dev/null +++ b/pkg/node/constants.go @@ -0,0 +1,9 @@ +package node + +import ( + "time" +) + +const ( + NetworkStateRefresh = 5 * time.Second +) diff --git a/test/e2e/handler/nns_update_timestamp_test.go b/test/e2e/handler/nns_update_timestamp_test.go index bd882e3be1..2f5aebb52c 100644 --- a/test/e2e/handler/nns_update_timestamp_test.go +++ b/test/e2e/handler/nns_update_timestamp_test.go @@ -6,23 +6,60 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/types" + + "github.com/nmstate/kubernetes-nmstate/api/shared" + nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1" + nmstatenode "github.com/nmstate/kubernetes-nmstate/pkg/node" ) var _ = Describe("[nns] NNS LastSuccessfulUpdateTime", func() { - Context("when updating nns", func() { - It("timestamp should be changed", func() { - for _, node := range nodes { + var ( + originalNNSs map[string]nmstatev1beta1.NodeNetworkState + ) + BeforeEach(func() { + originalNNSs = map[string]nmstatev1beta1.NodeNetworkState{} + for _, node := range allNodes { + key := types.NamespacedName{Name: node} + originalNNSs[node] = nodeNetworkState(key) + } + }) + Context("when network configuration hasn't change", func() { + It("should not be updated", func() { + for node, originalNNS := range originalNNSs { + // Give enough time for the NNS to be updated (3 interval times) + timeout := 3 * nmstatenode.NetworkStateRefresh key := types.NamespacedName{Name: node} - originalTime := nodeNetworkState(key).Status.LastSuccessfulUpdateTime + Consistently(func() shared.NodeNetworkStateStatus { + return nodeNetworkState(key).Status + }, timeout, time.Second).Should(Equal(originalNNS.Status)) + } + }) + }) + Context("when network configuration changed", func() { + BeforeEach(func() { + setDesiredStateWithPolicyWithoutNodeSelector(TestPolicy, linuxBrUp(bridge1)) + waitForAvailableTestPolicy() + }) + AfterEach(func() { + setDesiredStateWithPolicyWithoutNodeSelector(TestPolicy, linuxBrAbsent(bridge1)) + waitForAvailableTestPolicy() + setDesiredStateWithPolicyWithoutNodeSelector(TestPolicy, resetPrimaryAndSecondaryNICs()) + waitForAvailableTestPolicy() + deletePolicy(TestPolicy) + }) + It("should be updated", func() { + for node, originalNNS := range originalNNSs { // Give enough time for the NNS to be updated (3 interval times) - timeout := time.Duration(5*3) * time.Second + timeout := 3 * nmstatenode.NetworkStateRefresh + key := types.NamespacedName{Name: node} Eventually(func() time.Time { updatedTime := nodeNetworkState(key).Status.LastSuccessfulUpdateTime return updatedTime.Time - }, timeout, 1*time.Second).Should(BeTemporally(">", originalTime.Time)) + }, timeout, time.Second).Should(BeTemporally(">", originalNNS.Status.LastSuccessfulUpdateTime.Time)) } }) }) + })