Fix defaulting of maxUnavailable when wrong input is provided (#885) (#…

…918) Signed-off-by: Radim Hrazdil <[email protected]>
nmstate · Dec 2, 2021 · 66e0906 · 66e0906
1 parent 5ba9634
commit 66e0906
Show file tree

Hide file tree

Showing 4 changed files with 118 additions and 9 deletions.
diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go
@@ -348,7 +348,9 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount
 	}
 	maxUnavailable, err := node.MaxUnavailableNodeCount(r.APIClient, policy)
 	if err != nil {
-		return err
+		r.Log.Info(
+			fmt.Sprintf("failed calculating limit of max unavailable nodes, defaulting to %d, err: %s", maxUnavailable, err.Error()),
+		)
 	}
 	if policy.Status.UnavailableNodeCount >= maxUnavailable {
 		return apierrors.NewConflict(schema.GroupResource{Resource: "nodenetworkconfigurationpolicies"}, policy.Name, fmt.Errorf("maximal number of %d nodes are already processing policy configuration", policy.Status.UnavailableNodeCount))

diff --git a/pkg/node/node_suite_test.go b/pkg/node/node_suite_test.go
@@ -0,0 +1,16 @@
+package node
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/onsi/ginkgo/reporters"
+)
+
+func TestUnit(t *testing.T) {
+	RegisterFailHandler(Fail)
+	junitReporter := reporters.NewJUnitReporter("junit.controller-nodenetworkconfigurationpolicy-node-node_suite_test.xml")
+	RunSpecsWithDefaultAndCustomReporters(t, "Node Test Suite", []Reporter{junitReporter})
+}
diff --git a/pkg/node/nodes.go b/pkg/node/nodes.go
@@ -2,6 +2,7 @@ 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"
@@ -14,6 +15,7 @@ import (
 
 const (
 	DEFAULT_MAXUNAVAILABLE = "50%"
+	MIN_MAXUNAVAILABLE     = 1
 )
 
 func NodesRunningNmstate(cli client.Reader, nodeSelector map[string]string) ([]corev1.Node, error) {
@@ -45,23 +47,31 @@ func NodesRunningNmstate(cli client.Reader, nodeSelector map[string]string) ([]c
 func MaxUnavailableNodeCount(cli client.Reader, policy *nmstatev1beta1.NodeNetworkConfigurationPolicy) (int, error) {
 	enactmentsTotal, _, err := enactment.CountByPolicy(cli, policy)
 	if err != nil {
-		return 0, err
+		return MIN_MAXUNAVAILABLE, err
 	}
 	intOrPercent := intstr.FromString(DEFAULT_MAXUNAVAILABLE)
 	if policy.Spec.MaxUnavailable != nil {
 		intOrPercent = *policy.Spec.MaxUnavailable
 	}
-	maxUnavailable, err := ScaledMaxUnavailableNodeCount(enactmentsTotal, intOrPercent)
-	return maxUnavailable, nil
+	return ScaledMaxUnavailableNodeCount(enactmentsTotal, intOrPercent)
 }
 
 func ScaledMaxUnavailableNodeCount(matchingNodes int, intOrPercent intstr.IntOrString) (int, error) {
+	correctMaxUnavailable := func(maxUnavailable int) int {
+		if maxUnavailable < 1 {
+			return MIN_MAXUNAVAILABLE
+		}
+		return maxUnavailable
+	}
 	maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(&intOrPercent, matchingNodes, true)
 	if err != nil {
-		return 0, err
-	}
-	if maxUnavailable < 1 {
-		maxUnavailable = 1
+		defaultMaxUnavailable := intstr.FromString(DEFAULT_MAXUNAVAILABLE)
+		maxUnavailable, _ = intstr.GetScaledValueFromIntOrPercent(
+			&defaultMaxUnavailable,
+			matchingNodes,
+			true,
+		)
+		return correctMaxUnavailable(maxUnavailable), err
 	}
-	return maxUnavailable, nil
+	return correctMaxUnavailable(maxUnavailable), nil
 }
diff --git a/pkg/node/nodes_test.go b/pkg/node/nodes_test.go
@@ -0,0 +1,81 @@
+package node
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
+	. "github.com/onsi/gomega"
+
+	"github.com/onsi/gomega/types"
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
+
+var _ = Describe("MaxUnavailable nodes", func() {
+	type maxUnavailableCase struct {
+		nmstateEnabledNodes          int
+		maxUnavailable               intstr.IntOrString
+		expectedScaledMaxUnavailable int
+		expectedError                types.GomegaMatcher
+	}
+	DescribeTable("testing ScaledMaxUnavailableNodeCount",
+		func(c maxUnavailableCase) {
+			maxUnavailable, err := ScaledMaxUnavailableNodeCount(c.nmstateEnabledNodes, c.maxUnavailable)
+			Expect(err).To(c.expectedError)
+			Expect(maxUnavailable).To(Equal(c.expectedScaledMaxUnavailable))
+		},
+		Entry("Default maxUnavailable with odd number of nodes",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          5,
+				maxUnavailable:               intstr.FromString(DEFAULT_MAXUNAVAILABLE),
+				expectedScaledMaxUnavailable: 3,
+				expectedError:                Not(HaveOccurred()),
+			}),
+		Entry("Default maxUnavailable with even number of nodes",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          6,
+				maxUnavailable:               intstr.FromString(DEFAULT_MAXUNAVAILABLE),
+				expectedScaledMaxUnavailable: 3,
+				expectedError:                Not(HaveOccurred()),
+			}),
+		Entry("Absolute maxUnavailable with odd number of nodes",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          5,
+				maxUnavailable:               intstr.FromInt(3),
+				expectedScaledMaxUnavailable: 3,
+				expectedError:                Not(HaveOccurred()),
+			}),
+		Entry("Absolute maxUnavailable with even number of nodes",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          6,
+				maxUnavailable:               intstr.FromInt(3),
+				expectedScaledMaxUnavailable: 3,
+				expectedError:                Not(HaveOccurred()),
+			}),
+		Entry("Wrong string value",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          5,
+				maxUnavailable:               intstr.FromString("asdf"),
+				expectedScaledMaxUnavailable: 3,
+				expectedError:                HaveOccurred(),
+			}),
+		Entry("Absolute value in string",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          5,
+				maxUnavailable:               intstr.FromString("42"),
+				expectedScaledMaxUnavailable: 3,
+				expectedError:                HaveOccurred(),
+			}),
+		Entry("Zero percent",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          5,
+				maxUnavailable:               intstr.FromString("0%"),
+				expectedScaledMaxUnavailable: 1,
+				expectedError:                Not(HaveOccurred()),
+			}),
+		Entry("Zero value",
+			maxUnavailableCase{
+				nmstateEnabledNodes:          5,
+				maxUnavailable:               intstr.FromInt(0),
+				expectedScaledMaxUnavailable: 1,
+				expectedError:                Not(HaveOccurred()),
+			}))
+})