OCM-10777 | fix: fixing OCM-10777 + OCM-10818

openshift · Sep 9, 2024 · 9483f5e · 9483f5e
1 parent cfed125
commit 9483f5e
Show file tree

Hide file tree

Showing 6 changed files with 185 additions and 174 deletions.
diff --git a/pkg/helper/machinepools/helpers.go b/pkg/helper/machinepools/helpers.go
@@ -38,38 +38,6 @@ var allowedTaintEffects = []string{
  "PreferNoSchedule",
 }
 
-func MinNodePoolReplicaValidator(autoscaling bool) interactive.Validator {
- return func(val interface{}) error {
- minReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
- if err != nil {
- return err
- }
- if autoscaling {
- if minReplicas < 1 {
- return fmt.Errorf("min-replicas must be greater than zero")
- }
- } else {
- if minReplicas < 0 {
- return fmt.Errorf("replicas must be a non-negative integer")
- }
- }
- return nil
- }
-}
-
-func MaxNodePoolReplicaValidator(minReplicas int) interactive.Validator {
- return func(val interface{}) error {
- maxReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
- if err != nil {
- return err
- }
- if minReplicas > maxReplicas {
- return fmt.Errorf("max-replicas must be greater or equal to min-replicas")
- }
- return nil
- }
-}
-
 func ParseLabels(labels string) (map[string]string, error) {
  labelMap := make(map[string]string)
  if labels == "" || labels == interactiveModeEmptyLabels {

diff --git a/pkg/helper/machinepools/helpers_test.go b/pkg/helper/machinepools/helpers_test.go
@@ -89,64 +89,6 @@ var _ = Describe("MachinePool", func() {
 
 })
 
-var _ = Describe("Machine pool for hosted clusters", func() {
- DescribeTable("Machine pool min replicas validation",
- func(minReplicas int, autoscaling bool, hasError bool) {
- err := MinNodePoolReplicaValidator(autoscaling)(minReplicas)
- if hasError {
- Expect(err).To(HaveOccurred())
- } else {
- Expect(err).ToNot(HaveOccurred())
- }
- },
- Entry("Zero replicas - no autoscaling",
- 0,
- false,
- false,
- ),
- Entry("Negative replicas - no autoscaling",
- -1,
- false,
- true,
- ),
- Entry("Zero replicas - autoscaling",
- 0,
- true,
- true,
- ),
- Entry("One replicas - autoscaling",
- 1,
- true,
- false,
- ),
- )
- DescribeTable("Machine pool max replicas validation",
- func(minReplicas int, maxReplicas int, hasError bool) {
- err := MaxNodePoolReplicaValidator(minReplicas)(maxReplicas)
- if hasError {
- Expect(err).To(HaveOccurred())
- } else {
- Expect(err).ToNot(HaveOccurred())
- }
- },
- Entry("Max > Min -> OK",
- 1,
- 2,
- false,
- ),
- Entry("Min < Max -> OK",
- 2,
- 1,
- true,
- ),
- Entry("Min = Max -> OK",
- 2,
- 2,
- false,
- ),
- )
-})
-
 var _ = Describe("Label validations", func() {
  DescribeTable("Label validation",
  func(key string, value string, hasError bool) {

diff --git a/pkg/machinepool/helper.go b/pkg/machinepool/helper.go
@@ -189,32 +189,35 @@ func getMachinePoolAvailabilityZones(r *rosa.Runtime, cluster *cmv1.Cluster, mul
  return cluster.Nodes().AvailabilityZones(), nil
 }
 
-func minReplicaValidator(multiAZMachinePool bool) interactive.Validator {
+func maxReplicaValidator(minReplicas int, multiAZMachinePool bool) interactive.Validator {
  return func(val interface{}) error {
- minReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
+ maxReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
  if err != nil {
  return err
  }
- if minReplicas < 0 {
- return fmt.Errorf("min-replicas must be a non-negative integer")
+ if minReplicas > maxReplicas {
+ return fmt.Errorf("max-replicas must be greater or equal to min-replicas")
  }
- if multiAZMachinePool && minReplicas%3 != 0 {
+ if multiAZMachinePool && maxReplicas%3 != 0 {
  return fmt.Errorf("Multi AZ clusters require that the replicas be a multiple of 3")
  }
  return nil
  }
 }
 
-func maxReplicaValidator(minReplicas int, multiAZMachinePool bool) interactive.Validator {
+func minReplicaValidator(multiAZMachinePool bool, autoscaling bool) interactive.Validator {
  return func(val interface{}) error {
- maxReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
+ minReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
  if err != nil {
  return err
  }
- if minReplicas > maxReplicas {
- return fmt.Errorf("max-replicas must be greater or equal to min-replicas")
+ if autoscaling && minReplicas < 1 {
+ return fmt.Errorf("min-replicas must be greater than zero")
  }
- if multiAZMachinePool && maxReplicas%3 != 0 {
+ if !autoscaling && minReplicas < 0 {
+ return fmt.Errorf("Replicas must be a non-negative integer")
+ }
+ if multiAZMachinePool && minReplicas%3 != 0 {
  return fmt.Errorf("Multi AZ clusters require that the replicas be a multiple of 3")
  }
  return nil

diff --git a/pkg/machinepool/helper_test.go b/pkg/machinepool/helper_test.go
@@ -414,6 +414,95 @@ 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)
+ if hasError {
+ Expect(err).To(HaveOccurred())
+ } else {
+ Expect(err).ToNot(HaveOccurred())
+ }
+ },
+ Entry("Zero replicas - no autoscaling",
+ 0,
+ false,
+ false,
+ false,
+ ),
+ Entry("Negative replicas - no autoscaling",
+ -1,
+ false,
+ false,
+ true,
+ ),
+ Entry("Zero replicas - autoscaling",
+ 0,
+ true,
+ false,
+ true,
+ ),
+ Entry("One replicas - autoscaling",
+ 1,
+ true,
+ false,
+ false,
+ ),
+ Entry("Multi-AZ - 3 replicas",
+ 3,
+ true,
+ true,
+ false,
+ ),
+ Entry("Multi-AZ - 2 replicas",
+ 2,
+ true,
+ true,
+ true,
+ ),
+ )
+ DescribeTable("Machine pool max replicas validation",
+ func(minReplicas int, maxReplicas int, multiAZ bool, hasError bool) {
+ err := maxReplicaValidator(minReplicas, multiAZ)(maxReplicas)
+ if hasError {
+ Expect(err).To(HaveOccurred())
+ } else {
+ Expect(err).ToNot(HaveOccurred())
+ }
+ },
+ Entry("Max > Min -> OK",
+ 1,
+ 2,
+ false,
+ false,
+ ),
+ Entry("Min < Max -> OK",
+ 2,
+ 1,
+ false,
+ true,
+ ),
+ Entry("Min = Max -> OK",
+ 2,
+ 2,
+ false,
+ false,
+ ),
+ Entry("Not % 3 -> NOT OK",
+ 2,
+ 4,
+ true,
+ true,
+ ),
+ Entry("Multi-AZ -> OK",
+ 3,
+ 6,
+ true,
+ false,
+ ),
+ )
+})
+
 var _ = Describe("CreateAwsNodePoolBuilder", func() {
  It("correctly initializes AWSNodePoolBuilder with given parameters", func() {
  instanceType := "t2.micro"