From 28be74c2d76d9c75c51c3e6b265d1d835c50f1f4 Mon Sep 17 00:00:00 2001 From: Radim Hrazdil Date: Wed, 24 Mar 2021 18:25:18 +0100 Subject: [PATCH] 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 --- ...denetworkconfigurationpolicy_controller.go | 31 +++---------------- pkg/enactment/enactment.go | 22 +++++++++++++ pkg/node/nodes.go | 31 +++++++++++++++++++ test/e2e/handler/nncp_parallel_test.go | 19 +++++++++--- test/e2e/handler/upgrade_test.go | 2 +- test/e2e/handler/utils.go | 4 +-- test/environment/environment.go | 10 ------ 7 files changed, 75 insertions(+), 44 deletions(-) create mode 100644 pkg/enactment/enactment.go diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go index 1361069bc7..43dede1a99 100644 --- a/controllers/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/nodenetworkconfigurationpolicy_controller.go @@ -40,11 +40,12 @@ 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" @@ -52,11 +53,6 @@ import ( "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 new file mode 100644 index 0000000000..c0aa0a9645 --- /dev/null +++ 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 index 67c496956b..38d54026e1 100644 --- a/pkg/node/nodes.go +++ b/pkg/node/nodes.go @@ -2,6 +2,9 @@ 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" @@ -9,6 +12,10 @@ import ( "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 index 220dd760d3..079ce8a925 100644 --- 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 index 3978adab68..b54d058aa9 100644 --- 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 index c430a21916..7240bc8715 100644 --- 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 index c743090f12..3537a54cb2 100644 --- 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 -}