From 7464a08d033b0b9bdcb1eadf458ba1c933d2c6bd Mon Sep 17 00:00:00 2001 From: hkepley Date: Mon, 23 Sep 2024 14:35:30 -0400 Subject: [PATCH] OCM-11364 | fix: Regression, HCP nodepool creation validation used for classic --- pkg/machinepool/helper.go | 8 ++++++-- pkg/machinepool/helper_test.go | 6 +++--- pkg/machinepool/machinepool.go | 22 ++++++++++++---------- pkg/machinepool/machinepool_test.go | 16 ++++++++-------- 4 files changed, 29 insertions(+), 23 deletions(-) diff --git a/pkg/machinepool/helper.go b/pkg/machinepool/helper.go index e1c63810b0..a9ca2fdd67 100644 --- a/pkg/machinepool/helper.go +++ b/pkg/machinepool/helper.go @@ -205,15 +205,19 @@ func maxReplicaValidator(minReplicas int, multiAZMachinePool bool) interactive.V } } -func minReplicaValidator(multiAZMachinePool bool, autoscaling bool) interactive.Validator { +func minReplicaValidator(multiAZMachinePool bool, autoscaling bool, isHypershift bool) interactive.Validator { return func(val interface{}) error { minReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val)) if err != nil { return err } - if autoscaling && minReplicas < 1 { + if autoscaling && minReplicas < 1 && isHypershift { return fmt.Errorf("min-replicas must be greater than zero") } + if autoscaling && minReplicas < 0 && !isHypershift { + return fmt.Errorf("min-replicas must be a number that is 0 or greater when autoscaling is" + + " enabled") + } if !autoscaling && minReplicas < 0 { return fmt.Errorf("Replicas must be a non-negative integer") } diff --git a/pkg/machinepool/helper_test.go b/pkg/machinepool/helper_test.go index 3c8328b1ac..eb5678851b 100644 --- a/pkg/machinepool/helper_test.go +++ b/pkg/machinepool/helper_test.go @@ -3,7 +3,7 @@ package machinepool import ( "fmt" - gomock "go.uber.org/mock/gomock" + "go.uber.org/mock/gomock" awssdk "github.com/aws/aws-sdk-go-v2/aws" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" @@ -417,7 +417,7 @@ var _ = Describe("getSecurityGroupsOption", func() { var _ = Describe("Machine pool min/max replicas validation", func() { DescribeTable("Machine pool min replicas validation", func(minReplicas int, autoscaling bool, multiAZ bool, hasError bool) { - err := minReplicaValidator(multiAZ, autoscaling)(minReplicas) + err := minReplicaValidator(multiAZ, autoscaling, false)(minReplicas) if hasError { Expect(err).To(HaveOccurred()) } else { @@ -440,7 +440,7 @@ var _ = Describe("Machine pool min/max replicas validation", func() { 0, true, false, - true, + false, ), Entry("One replicas - autoscaling", 1, diff --git a/pkg/machinepool/machinepool.go b/pkg/machinepool/machinepool.go index 0af23ae41b..5fec85e7ca 100644 --- a/pkg/machinepool/machinepool.go +++ b/pkg/machinepool/machinepool.go @@ -228,7 +228,8 @@ func (m *machinePool) CreateMachinePool(r *rosa.Runtime, cmd *cobra.Command, clu } } - minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool) + minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, + cluster.Hypershift().Enabled()) if err != nil { return err } @@ -588,7 +589,8 @@ func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clust } } - minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, false) + minReplicas, maxReplicas, replicas, autoscaling, err := manageReplicas(cmd, args, false, + cluster.Hypershift().Enabled()) if err != nil { return err } @@ -1323,7 +1325,7 @@ func getMachinePoolReplicas(cmd *cobra.Command, Default: minReplicas, Required: replicasRequired, Validators: []interactive.Validator{ - minReplicaValidator(multiAZ, false), + minReplicaValidator(multiAZ, false, false), }, }) if err != nil { @@ -1879,7 +1881,7 @@ func getNodePoolReplicas(cmd *cobra.Command, Default: existingAutoscaling.MinReplica(), Required: replicasRequired, Validators: []interactive.Validator{ - minReplicaValidator(false, true), + minReplicaValidator(false, true, true), }, }) if err != nil { @@ -1918,7 +1920,7 @@ func getNodePoolReplicas(cmd *cobra.Command, Default: replicas, Required: true, Validators: []interactive.Validator{ - minReplicaValidator(false, false), + minReplicaValidator(false, false, true), }, }) if err != nil { @@ -1956,7 +1958,7 @@ func editAutoscaling(nodePool *cmv1.NodePool, minReplicas int, maxReplicas int) return nil } func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions, - multiAZMachinePool bool) (minReplicas, maxReplicas, replicas int, autoscaling bool, err error) { + multiAZMachinePool bool, isHypershift bool) (minReplicas, maxReplicas, replicas int, autoscaling bool, err error) { isMinReplicasSet := cmd.Flags().Changed("min-replicas") isMaxReplicasSet := cmd.Flags().Changed("max-replicas") isAutoscalingSet := cmd.Flags().Changed("enable-autoscaling") @@ -1996,7 +1998,7 @@ func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOption Default: minReplicas, Required: true, Validators: []interactive.Validator{ - minReplicaValidator(multiAZMachinePool, autoscaling), + minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift), }, }) if err != nil { @@ -2004,7 +2006,7 @@ func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOption fmt.Errorf("Expected a valid number of min replicas: %s", err) } } - err = minReplicaValidator(multiAZMachinePool, autoscaling)(minReplicas) + err = minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift)(minReplicas) if err != nil { return minReplicas, maxReplicas, replicas, autoscaling, err } @@ -2044,14 +2046,14 @@ func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOption Default: replicas, Required: true, Validators: []interactive.Validator{ - minReplicaValidator(multiAZMachinePool, autoscaling), + minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift), }, }) if err != nil { return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid number of replicas: %s", err) } } - err = minReplicaValidator(multiAZMachinePool, autoscaling)(replicas) + err = minReplicaValidator(multiAZMachinePool, autoscaling, isHypershift)(replicas) if err != nil { return minReplicas, maxReplicas, replicas, autoscaling, err } diff --git a/pkg/machinepool/machinepool_test.go b/pkg/machinepool/machinepool_test.go index f7f8346e6f..f0963059fc 100644 --- a/pkg/machinepool/machinepool_test.go +++ b/pkg/machinepool/machinepool_test.go @@ -6,10 +6,10 @@ import ( "io" "net/http" "os" - reflect "reflect" + "reflect" "time" - gomock "go.uber.org/mock/gomock" + "go.uber.org/mock/gomock" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" . "github.com/onsi/ginkgo/v2" @@ -515,7 +515,7 @@ var _ = Describe("Utility Functions", func() { var validator interactive.Validator BeforeEach(func() { - validator = minReplicaValidator(true, false) // or false for non-multiAZ + validator = minReplicaValidator(true, false, false) // or false for non-multiAZ }) When("input is non-integer", func() { @@ -1532,7 +1532,7 @@ var _ = Describe("ManageReplicas", func() { args.AutoscalingEnabled = true cmd.Flags().Int32("replicas", 1, "Replicas of the machine pool") cmd.Flags().Set("replicas", "1") - _, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool) + _, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Replicas can't be set when autoscaling is enabled")) Expect(autoscaling).To(BeTrue()) @@ -1545,7 +1545,7 @@ var _ = Describe("ManageReplicas", func() { cmd.Flags().Set("max-replicas", "6") args.MinReplicas = 3 args.MaxReplicas = 6 - _, _, _, _, err := manageReplicas(cmd, args, multiAZMachinePool) + _, _, _, _, err := manageReplicas(cmd, args, multiAZMachinePool, false) Expect(err).ToNot(HaveOccurred()) }) }) @@ -1559,7 +1559,7 @@ var _ = Describe("ManageReplicas", func() { cmd.Flags().Set("max-replicas", "3") args.MinReplicas = 1 args.MaxReplicas = 3 - _, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool) + _, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Autoscaling must be enabled in order to set min and max replicas")) Expect(autoscaling).To(BeFalse()) @@ -1568,7 +1568,7 @@ var _ = Describe("ManageReplicas", func() { args.AutoscalingEnabled = false cmd.Flags().Int32("replicas", 1, "Replicas of the machine pool") cmd.Flags().Set("replicas", "1") - _, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool) + _, _, _, autoscaling, err := manageReplicas(cmd, args, multiAZMachinePool, false) Expect(err).ToNot(HaveOccurred()) Expect(autoscaling).To(BeFalse()) }) @@ -1594,7 +1594,7 @@ var _ = Describe("Utility Functions", func() { var validator interactive.Validator BeforeEach(func() { - validator = minReplicaValidator(true, false) // or false for non-multiAZ + validator = minReplicaValidator(true, false, false) // or false for non-multiAZ }) It("should return error for non-integer input", func() {