[release-0.47] Avoid incrementing NRU when NNCE (#776)

At the beginning, check if NNCE exists for a policy and is in progressing state. If it is, then it means that handler was killed while applying the desired state - do not increment unavailableNodeCount in such case. There is still a window where killing handler would cause inconsistency - between status is set to Failed or Success and the unavailableNodeCount is decremented. Signed-off-by: Radim Hrazdil <[email protected]> Signed-off-by: Quique Llorente <[email protected]> Co-authored-by: Radim Hrazdil <[email protected]>
nmstate · Jun 15, 2021 · 27eef45 · 27eef45
1 parent 9fa5fe0
commit 27eef45
Show file tree

Hide file tree

Showing 3 changed files with 136 additions and 12 deletions.
diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go
@@ -133,7 +133,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context
 
 	policyconditions.Reset(r.Client, request.NamespacedName)
 
-	err = r.initializeEnactment(*instance)
+	previousConditions, err := r.initializeEnactment(*instance)
 	if err != nil {
 		log.Error(err, "Error initializing enactment")
 	}
@@ -171,12 +171,14 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context
 		return ctrl.Result{}, nil
 	}
 
-	err = r.incrementUnavailableNodeCount(instance)
-	if err != nil {
-		if apierrors.IsConflict(err) {
-			return ctrl.Result{RequeueAfter: nodeRunningUpdateRetryTime}, err
+	if r.shouldIncrementUnavailableNodeCount(previousConditions) {
+		err = r.incrementUnavailableNodeCount(instance)
+		if err != nil {
+			if apierrors.IsConflict(err) {
+				return ctrl.Result{RequeueAfter: nodeRunningUpdateRetryTime}, err
+			}
+			return ctrl.Result{}, err
 		}
-		return ctrl.Result{}, err
 	}
 	defer r.decrementUnavailableNodeCount(instance)
 
@@ -239,32 +241,32 @@ func (r *NodeNetworkConfigurationPolicyReconciler) SetupWithManager(mgr ctrl.Man
 	return nil
 }
 
-func (r *NodeNetworkConfigurationPolicyReconciler) initializeEnactment(policy nmstatev1beta1.NodeNetworkConfigurationPolicy) error {
+func (r *NodeNetworkConfigurationPolicyReconciler) initializeEnactment(policy nmstatev1beta1.NodeNetworkConfigurationPolicy) (*nmstateapi.ConditionList, error) {
 	enactmentKey := nmstateapi.EnactmentKey(nodeName, policy.Name)
 	log := r.Log.WithName("initializeEnactment").WithValues("policy", policy.Name, "enactment", enactmentKey.Name)
 	// Return if it's already initialize or we cannot retrieve it
 	enactment := nmstatev1beta1.NodeNetworkConfigurationEnactment{}
 	err := r.Client.Get(context.TODO(), enactmentKey, &enactment)
 	if err != nil && !apierrors.IsNotFound(err) {
-		return errors.Wrap(err, "failed getting enactment ")
+		return nil, errors.Wrap(err, "failed getting enactment ")
 	}
 	if err != nil && apierrors.IsNotFound(err) {
 		log.Info("creating enactment")
 		enactment = nmstatev1beta1.NewEnactment(nodeName, policy)
 		err = r.Client.Create(context.TODO(), &enactment)
 		if err != nil {
-			return errors.Wrapf(err, "error creating NodeNetworkConfigurationEnactment: %+v", enactment)
+			return nil, errors.Wrapf(err, "error creating NodeNetworkConfigurationEnactment: %+v", enactment)
 		}
 		err = r.waitEnactmentCreated(enactmentKey)
 		if err != nil {
-			return errors.Wrapf(err, "error waitting for NodeNetworkConfigurationEnactment: %+v", enactment)
+			return nil, errors.Wrapf(err, "error waitting for NodeNetworkConfigurationEnactment: %+v", enactment)
 		}
 	} else {
 		enactmentConditions := enactmentconditions.New(r.Client, enactmentKey)
 		enactmentConditions.Reset()
 	}
 
-	return enactmentstatus.Update(r.Client, enactmentKey, func(status *nmstateapi.NodeNetworkConfigurationEnactmentStatus) {
+	return &enactment.Status.Conditions, enactmentstatus.Update(r.Client, enactmentKey, func(status *nmstateapi.NodeNetworkConfigurationEnactmentStatus) {
 		status.DesiredState = policy.Spec.DesiredState
 		status.PolicyGeneration = policy.Generation
 	})
@@ -288,6 +290,10 @@ func (r *NodeNetworkConfigurationPolicyReconciler) waitEnactmentCreated(enactmen
 	return pollErr
 }
 
+func (r *NodeNetworkConfigurationPolicyReconciler) shouldIncrementUnavailableNodeCount(conditions *nmstateapi.ConditionList) bool {
+	return !enactmentstatus.IsProgressing(conditions)
+}
+
 func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) error {
 	policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()}
 	err := r.Client.Get(context.TODO(), policyKey, policy)

diff --git a/controllers/nodenetworkconfigurationpolicy_controller_test.go b/controllers/nodenetworkconfigurationpolicy_controller_test.go
@@ -1,14 +1,23 @@
 package controllers
 
 import (
+	"context"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
-
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/kubernetes/scheme"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 
+	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
+	"github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus/conditions"
 )
 
 var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func() {
@@ -62,4 +71,104 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func()
 				ReconcileUpdate: true,
 			}),
 	)
+
+	type incrementUnavailableNodeCountCase struct {
+		currentUnavailableNodeCount  int
+		expectedUnavailableNodeCount int
+		expectedReconcileError       string
+		expectedReconcileResult      ctrl.Result
+		previousEnactmentConditions  func(*shared.ConditionList, string)
+		shouldConflict               bool
+	}
+	DescribeTable("when claimNodeRunningUpdate is called and",
+		func(c incrementUnavailableNodeCountCase) {
+			reconciler := NodeNetworkConfigurationPolicyReconciler{}
+			s := scheme.Scheme
+			s.AddKnownTypes(nmstatev1beta1.GroupVersion,
+				&nmstatev1beta1.NodeNetworkConfigurationPolicy{},
+				&nmstatev1beta1.NodeNetworkConfigurationEnactment{},
+				&nmstatev1beta1.NodeNetworkConfigurationEnactmentList{},
+			)
+
+			node := corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: nodeName,
+				},
+			}
+			nncp := nmstatev1beta1.NodeNetworkConfigurationPolicy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+				Status: shared.NodeNetworkConfigurationPolicyStatus{
+					UnavailableNodeCount: c.currentUnavailableNodeCount,
+				},
+			}
+			nnce := nmstatev1beta1.NodeNetworkConfigurationEnactment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: shared.EnactmentKey(nodeName, nncp.Name).Name,
+				},
+				Status: shared.NodeNetworkConfigurationEnactmentStatus{},
+			}
+
+			// simulate NNCE existnce/non-existence by setting conditions
+			c.previousEnactmentConditions(&nnce.Status.Conditions, "")
+
+			objs := []runtime.Object{&nncp, &nnce, &node}
+
+			// Create a fake client to mock API calls.
+			clb := fake.ClientBuilder{}
+			clb.WithScheme(s)
+			clb.WithRuntimeObjects(objs...)
+			cl := clb.Build()
+
+			reconciler.Client = cl
+			reconciler.Log = ctrl.Log.WithName("controllers").WithName("NodeNetworkConfigurationPolicy")
+
+			res, err := reconciler.Reconcile(context.TODO(), ctrl.Request{
+				NamespacedName: types.NamespacedName{Name: nncp.Name},
+			})
+
+			if c.shouldConflict {
+				Expect(err.Error()).To(Equal(c.expectedReconcileError))
+			}
+			Expect(res).To(Equal(c.expectedReconcileResult))
+
+			obtainedNNCP := nmstatev1beta1.NodeNetworkConfigurationPolicy{}
+			cl.Get(context.TODO(), types.NamespacedName{Name: nncp.Name}, &obtainedNNCP)
+			Expect(obtainedNNCP.Status.UnavailableNodeCount).To(Equal(c.expectedUnavailableNodeCount))
+		},
+		Entry("No node applying policy with empty enactment, should succeed incrementing UnavailableNodeCount",
+			incrementUnavailableNodeCountCase{
+				currentUnavailableNodeCount:  0,
+				expectedUnavailableNodeCount: 0,
+				previousEnactmentConditions:  func(*shared.ConditionList, string) {},
+				expectedReconcileResult:      ctrl.Result{},
+				shouldConflict:               false,
+			}),
+		Entry("No node applying policy with progressing enactment, should succeed incrementing UnavailableNodeCount",
+			incrementUnavailableNodeCountCase{
+				currentUnavailableNodeCount:  0,
+				expectedUnavailableNodeCount: 0,
+				previousEnactmentConditions:  conditions.SetProgressing,
+				expectedReconcileResult:      ctrl.Result{},
+				shouldConflict:               false,
+			}),
+		Entry("One node applying policy with empty enactment, should conflict incrementing UnavailableNodeCount",
+			incrementUnavailableNodeCountCase{
+				currentUnavailableNodeCount:  1,
+				expectedUnavailableNodeCount: 1,
+				previousEnactmentConditions:  func(*shared.ConditionList, string) {},
+				expectedReconcileResult:      ctrl.Result{RequeueAfter: nodeRunningUpdateRetryTime},
+				expectedReconcileError:       "Operation cannot be fulfilled on nodenetworkconfigurationpolicies \"test\": maximal number of 1 nodes are already processing policy configuration",
+				shouldConflict:               true,
+			}),
+		Entry("One node applying policy with Progressing enactment, should succeed incrementing UnavailableNodeCount",
+			incrementUnavailableNodeCountCase{
+				currentUnavailableNodeCount:  1,
+				expectedUnavailableNodeCount: 0,
+				previousEnactmentConditions:  conditions.SetProgressing,
+				expectedReconcileResult:      ctrl.Result{},
+				shouldConflict:               false,
+			}),
+	)
 })
diff --git a/pkg/enactmentstatus/status.go b/pkg/enactmentstatus/status.go
@@ -9,6 +9,7 @@ import (
 	"github.com/pkg/errors"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/util/retry"
@@ -54,3 +55,11 @@ func Update(client client.Client, key types.NamespacedName, statusSetter func(*n
 		})
 	})
 }
+
+func IsProgressing(conditions *nmstate.ConditionList) bool {
+	progressingCondition := conditions.Find(nmstate.NodeNetworkConfigurationEnactmentConditionProgressing)
+	if progressingCondition != nil && progressingCondition.Status == corev1.ConditionTrue {
+		return true
+	}
+	return false
+}