Calculate percentage of maxUnavailable from matching nodes

Instead of calculating number of maxUnavailable nodes from all nmstate enabled nodes, use matching nodes only. Signed-off-by: Radim Hrazdil <[email protected]>
nmstate · Mar 25, 2021 · 28be74c · 28be74c
1 parent a0c316b
commit 28be74c
Show file tree

Hide file tree

Showing 7 changed files with 75 additions and 44 deletions.
diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go
@@ -40,23 +40,19 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 
 	nmstateapi "github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
+	"github.com/nmstate/kubernetes-nmstate/pkg/enactment"
 	"github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus"
 	enactmentconditions "github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus/conditions"
 	"github.com/nmstate/kubernetes-nmstate/pkg/environment"
 	nmstate "github.com/nmstate/kubernetes-nmstate/pkg/helper"
 	"github.com/nmstate/kubernetes-nmstate/pkg/node"
 	"github.com/nmstate/kubernetes-nmstate/pkg/policyconditions"
 	"github.com/nmstate/kubernetes-nmstate/pkg/selectors"
-	"k8s.io/apimachinery/pkg/types"
-)
-
-const (
-	DEFAULT_MAXUNAVAILABLE = "50%"
 )
 
 var (
@@ -160,25 +156,6 @@ func (r *NodeNetworkConfigurationPolicyReconciler) initializeEnactment(policy nm
 	})
 }
 
-func (r *NodeNetworkConfigurationPolicyReconciler) maxUnavailableNodeCount(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) (int, error) {
-	nmstateNodes, err := node.NodesRunningNmstate(r.Client)
-	if err != nil {
-		return 0, err
-	}
-	intOrPercent := intstr.FromString(DEFAULT_MAXUNAVAILABLE)
-	if policy.Spec.MaxUnavailable != nil {
-		intOrPercent = *policy.Spec.MaxUnavailable
-	}
-	maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(&intOrPercent, len(nmstateNodes), true)
-	if err != nil {
-		return 0, err
-	}
-	if maxUnavailable < 1 {
-		maxUnavailable = 1
-	}
-	return maxUnavailable, nil
-}
-
 func (r *NodeNetworkConfigurationPolicyReconciler) enactmentsCountByPolicy(policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) (enactmentconditions.ConditionCount, error) {
 	enactments := nmstatev1beta1.NodeNetworkConfigurationEnactmentList{}
 	policyLabelFilter := client.MatchingLabels{nmstateapi.EnactmentPolicyLabel: policy.GetName()}
@@ -196,7 +173,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount
 	if err != nil {
 		return err
 	}
-	maxUnavailable, err := r.maxUnavailableNodeCount(policy)
+	maxUnavailable, err := node.MaxUnavailableNodeCount(r.Client, policy)
 	if err != nil {
 		return err
 	}
@@ -282,7 +259,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context
 
 	enactmentConditions.NotifyMatching()
 
-	enactmentCount, err := r.enactmentsCountByPolicy(instance)
+	enactmentCount, err := enactment.CountByPolicy(r.Client, instance)
 	if err != nil {
 		log.Error(err, "Error getting enactment counts")
 		return ctrl.Result{}, err

diff --git a/pkg/enactment/enactment.go b/pkg/enactment/enactment.go
@@ -0,0 +1,22 @@
+package enactment
+
+import (
+	"context"
+
+	nmstateapi "github.com/nmstate/kubernetes-nmstate/api/shared"
+	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
+	enactmentconditions "github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus/conditions"
+	"github.com/pkg/errors"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+func CountByPolicy(cli client.Client, policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) (enactmentconditions.ConditionCount, error) {
+	enactments := nmstatev1beta1.NodeNetworkConfigurationEnactmentList{}
+	policyLabelFilter := client.MatchingLabels{nmstateapi.EnactmentPolicyLabel: policy.GetName()}
+	err := cli.List(context.TODO(), &enactments, policyLabelFilter)
+	if err != nil {
+		return nil, errors.Wrap(err, "getting enactment list failed")
+	}
+	enactmentCount := enactmentconditions.Count(enactments, policy.Generation)
+	return enactmentCount, nil
+}
diff --git a/pkg/node/nodes.go b/pkg/node/nodes.go
@@ -2,13 +2,20 @@ package node
 
 import (
 	"context"
+	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
+	"github.com/nmstate/kubernetes-nmstate/pkg/enactment"
+	"k8s.io/apimachinery/pkg/util/intstr"
 
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/pkg/errors"
 )
 
+const (
+	DEFAULT_MAXUNAVAILABLE = "50%"
+)
+
 func NodesRunningNmstate(cli client.Client) ([]corev1.Node, error) {
 	nodes := corev1.NodeList{}
 	err := cli.List(context.TODO(), &nodes)
@@ -34,3 +41,27 @@ func NodesRunningNmstate(cli client.Client) ([]corev1.Node, error) {
 	}
 	return filteredNodes, nil
 }
+
+func MaxUnavailableNodeCount(cli client.Client, policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) (int, error) {
+	enactmentsCount, err := enactment.CountByPolicy(cli, policy)
+	if err != nil {
+		return 0, err
+	}
+	intOrPercent := intstr.FromString(DEFAULT_MAXUNAVAILABLE)
+	if policy.Spec.MaxUnavailable != nil {
+		intOrPercent = *policy.Spec.MaxUnavailable
+	}
+	maxUnavailable, err := ScaledMaxUnavailableNodeCount(enactmentsCount.Matching(), intOrPercent)
+	return maxUnavailable, nil
+}
+
+func ScaledMaxUnavailableNodeCount(matchingNodes int, intOrPercent intstr.IntOrString) (int, error) {
+	maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(&intOrPercent, matchingNodes, true)
+	if err != nil {
+		return 0, err
+	}
+	if maxUnavailable < 1 {
+		maxUnavailable = 1
+	}
+	return maxUnavailable, nil
+}
diff --git a/test/e2e/handler/nncp_parallel_test.go b/test/e2e/handler/nncp_parallel_test.go
@@ -7,8 +7,12 @@ import (
 	. "github.com/onsi/gomega"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+
+	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
 
 	nmstate "github.com/nmstate/kubernetes-nmstate/api/shared"
+	"github.com/nmstate/kubernetes-nmstate/pkg/node"
 )
 
 func enactmentsInProgress(policy string) int {
@@ -24,8 +28,17 @@ func enactmentsInProgress(policy string) int {
 	return progressingEnactments
 }
 
+func maxUnavailableNodes() int {
+	m, _ := node.ScaledMaxUnavailableNodeCount(len(nodes), intstr.FromString(node.DEFAULT_MAXUNAVAILABLE))
+	return m
+}
+
 var _ = Describe("NNCP with maxUnavailable", func() {
+	policy := &nmstatev1beta1.NodeNetworkConfigurationPolicy{}
+	policy.Name = TestPolicy
 	Context("when applying a policy to matching nodes", func() {
+		duration := 10 * time.Second
+		interval := 1 * time.Second
 		BeforeEach(func() {
 			By("Create a policy")
 			updateDesiredState(linuxBrUp(bridge1))
@@ -41,15 +54,13 @@ var _ = Describe("NNCP with maxUnavailable", func() {
 		It("[parallel] should be progressing on multiple nodes", func() {
 			Eventually(func() int {
 				return enactmentsInProgress(TestPolicy)
-			}).Should(BeNumerically("==", maxUnavailable))
+			}, duration, interval).Should(BeNumerically("==", maxUnavailableNodes()))
 			waitForAvailablePolicy(TestPolicy)
 		})
 		It("[parallel] should never exceed maxUnavailable nodes", func() {
-			duration := 10 * time.Second
-			interval := 1 * time.Second
 			Consistently(func() int {
 				return enactmentsInProgress(TestPolicy)
-			}, duration, interval).Should(BeNumerically("<=", maxUnavailable))
+			}, duration, interval).Should(BeNumerically("<=", maxUnavailableNodes()))
 			waitForAvailablePolicy(TestPolicy)
 		})
 	})

diff --git a/test/e2e/handler/upgrade_test.go b/test/e2e/handler/upgrade_test.go
@@ -18,7 +18,7 @@ import (
 var _ = Describe("NodeNetworkConfigurationPolicy upgrade", func() {
 	Context("when v1alpha1 is populated", func() {
 		BeforeEach(func() {
-			maxUnavailableIntOrString := intstr.FromInt(maxUnavailable)
+			maxUnavailableIntOrString := intstr.FromString(maxUnavailable)
 			policy := nmstatev1alpha1.NodeNetworkConfigurationPolicy{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: TestPolicy,

diff --git a/test/e2e/handler/utils.go b/test/e2e/handler/utils.go
@@ -37,7 +37,7 @@ const TestPolicy = "test-policy"
 var (
 	bridgeCounter  = 0
 	bondConunter   = 0
-	maxUnavailable = environment.GetIntVarWithDefault("NMSTATE_MAX_UNAVAILABLE", 1)
+	maxUnavailable = environment.GetVarWithDefault("NMSTATE_MAX_UNAVAILABLE", "50%")
 )
 
 func interfacesName(interfaces []interface{}) []string {
@@ -70,7 +70,7 @@ func setDesiredStateWithPolicyAndNodeSelector(name string, desiredState nmstate.
 	err := testenv.Client.Get(context.TODO(), key, &policy)
 	policy.Spec.DesiredState = desiredState
 	policy.Spec.NodeSelector = nodeSelector
-	maxUnavailableIntOrString := intstr.FromInt(maxUnavailable)
+	maxUnavailableIntOrString := intstr.FromString(maxUnavailable)
 	policy.Spec.MaxUnavailable = &maxUnavailableIntOrString
 	if err != nil {
 		if apierrors.IsNotFound(err) {

diff --git a/test/environment/environment.go b/test/environment/environment.go
@@ -2,7 +2,6 @@ package environment
 
 import (
 	"os"
-	"strconv"
 )
 
 func GetVarWithDefault(name string, defaultValue string) string {
@@ -12,12 +11,3 @@ func GetVarWithDefault(name string, defaultValue string) string {
 	}
 	return value
 }
-
-func GetIntVarWithDefault(name string, defaultValue int) int {
-	value := os.Getenv(name)
-	intValue, err := strconv.Atoi(value)
-	if err != nil {
-		intValue = defaultValue
-	}
-	return intValue
-}