From 66e0906f35669619d7329b840625a9fa88e407ab Mon Sep 17 00:00:00 2001 From: Radim Hrazdil <32546791+rhrazdil@users.noreply.github.com> Date: Thu, 2 Dec 2021 12:23:06 +0100 Subject: [PATCH] Fix defaulting of maxUnavailable when wrong input is provided (#885) (#918) Signed-off-by: Radim Hrazdil --- ...denetworkconfigurationpolicy_controller.go | 4 +- pkg/node/node_suite_test.go | 16 ++++ pkg/node/nodes.go | 26 ++++-- pkg/node/nodes_test.go | 81 +++++++++++++++++++ 4 files changed, 118 insertions(+), 9 deletions(-) create mode 100644 pkg/node/node_suite_test.go create mode 100644 pkg/node/nodes_test.go diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go index d820c63f9..5804538c3 100644 --- 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 new file mode 100644 index 000000000..81e36af27 --- /dev/null +++ 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 index 1d82b0af2..32092a85e 100644 --- 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 new file mode 100644 index 000000000..c193900c4 --- /dev/null +++ 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()), + })) +})