OCM-9917 | fix: Issue with interactive + replicas edit machinepool

cmd/edit/machinepool/cmd_test.go

@@ -81,8 +81,8 @@ var _ = Describe("Edit Machinepool", func() {
  cmd := NewEditMachinePoolCommand()
  Expect(cmd.Flag("cluster").Value.Set(clusterId)).To(Succeed())
  Expect(cmd.Flags().Set("labels", "test=test")).To(Succeed())
- Expect(runner(context.Background(), t.RosaRuntime, cmd, []string{"--machinepool",
-  nodePoolId, "--min-replicas", "2", "--enable-autoscaling", "true", "-y"})).To(Succeed())
+ Expect(cmd.Flags().Set("replicas", "1")).To(Succeed())
+ Expect(runner(context.Background(), t.RosaRuntime, cmd, []string{})).To(Succeed())
  })
  It("Machinepool ID passed in without flag in random location", func() {
  // First get
@@ -96,8 +96,11 @@ var _ = Describe("Edit Machinepool", func() {
  cmd := NewEditMachinePoolCommand()
  Expect(cmd.Flag("cluster").Value.Set(clusterId)).To(Succeed())
  Expect(cmd.Flags().Set("labels", "test=test")).To(Succeed())
+ Expect(cmd.Flags().Set("min-replicas", "1")).To(Succeed())
+ Expect(cmd.Flags().Set("max-replicas", "1")).To(Succeed())
+ Expect(cmd.Flags().Set("enable-autoscaling", "true")).To(Succeed())
  Expect(runner(context.Background(), t.RosaRuntime, cmd,
- []string{"--min-replicas", "2", nodePoolId, "--enable-autoscaling", "true", "-y"})).To(Succeed())
+ []string{})).To(Succeed())
  })
  })
 

cmd/edit/machinepool/options.go

@@ -73,6 +73,9 @@ func (m *EditMachinepoolOptions) Bind(args *EditMachinepoolUserOptions, argv []s
  if m.args.minReplicas > m.args.maxReplicas {
  return fmt.Errorf("Min replicas must be less than max replicas")
  }
+ if m.args.replicas != 0 {
+ return fmt.Errorf("Autoscaling enabled on machine pool '%s'. can't set replicas", m.Machinepool())
+ }
  }
 
  return nil

pkg/machinepool/machinepool.go

@@ -1399,7 +1399,6 @@ func getMachinePoolReplicas(cmd *cobra.Command,
  }
 
  if autoscaling {
-
  // set default values from previous autoscaling values
  if !isMinReplicasSet {
  minReplicas = existingAutoscaling.MinReplicas()
@@ -1409,12 +1408,16 @@ func getMachinePoolReplicas(cmd *cobra.Command,
  }
 
  // Prompt for min replicas if neither min or max is set or interactive mode
- if !isMinReplicasSet && (interactive.Enabled() || !isMaxReplicasSet && askForScalingParams) {
+ if !isMinReplicasSet && (interactive.Enabled() || !isMaxReplicasSet &&
+ askForScalingParams) {
  minReplicas, err = interactive.GetInt(interactive.Input{
  Question: "Min replicas",
  Help: cmd.Flags().Lookup("min-replicas").Usage,
  Default: minReplicas,
  Required: replicasRequired,
+ Validators: []interactive.Validator{
+ mpHelpers.MinNodePoolReplicaValidator(false),
+ },
  })
  if err != nil {
  err = fmt.Errorf("Expected a valid number of min replicas: %s", err)
@@ -1423,12 +1426,16 @@ func getMachinePoolReplicas(cmd *cobra.Command,
  }
 
  // Prompt for max replicas if neither min or max is set or interactive mode
- if !isMaxReplicasSet && (interactive.Enabled() || !isMinReplicasSet && askForScalingParams) {
+ if !isMaxReplicasSet && (interactive.Enabled() || !isMinReplicasSet &&
+ askForScalingParams) {
  maxReplicas, err = interactive.GetInt(interactive.Input{
  Question: "Max replicas",
  Help: cmd.Flags().Lookup("max-replicas").Usage,
  Default: maxReplicas,
  Required: replicasRequired,
+ Validators: []interactive.Validator{
+ mpHelpers.MaxNodePoolReplicaValidator(minReplicas),
+ },
  })
  if err != nil {
  err = fmt.Errorf("Expected a valid number of max replicas: %s", err)
@@ -1447,8 +1454,13 @@ func getMachinePoolReplicas(cmd *cobra.Command,
  })
  if err != nil {
  err = fmt.Errorf("Expected a valid number of replicas: %s", err)
+ return
  }
  }
+
+ err = validateEditInput("machine", autoscaling, minReplicas, maxReplicas, replicas, isReplicasSet,
+ isAutoscalingSet, isMinReplicasSet, isMaxReplicasSet, machinePoolID)
+
  return
 }
 
@@ -1519,11 +1531,6 @@ func editMachinePool(cmd *cobra.Command, machinePoolId string,
  return fmt.Errorf("Failed to get autoscaling or replicas: '%s'", err)
  }
 
- if !autoscaling && replicas < 0 ||
- (autoscaling && isMinReplicasSet && minReplicas < 0) {
- return fmt.Errorf("The number of machine pool replicas needs to be a non-negative integer")
- }
-
  if cluster.MultiAZ() && isMultiAZMachinePool(machinePool) &&
  (!autoscaling && replicas%3 != 0 ||
  (autoscaling && (minReplicas%3 != 0 || maxReplicas%3 != 0))) {
@@ -1928,7 +1935,6 @@ func getNodePoolReplicas(cmd *cobra.Command,
  err = fmt.Errorf("Autoscaling is not enabled on machine pool '%s'. can't set min or max replicas",
  nodePoolID)
  return
-
  }
 
  // if the user set replicas but enabled autoscaling or hasn't disabled existing autoscaling
@@ -2010,6 +2016,10 @@ func getNodePoolReplicas(cmd *cobra.Command,
  return
  }
  }
+
+ err = validateEditInput("node", autoscaling, minReplicas, maxReplicas, replicas, isReplicasSet,
+ isAutoscalingSet, isMinReplicasSet, isMaxReplicasSet, nodePoolID)
+
  return
 }
 

pkg/machinepool/validation.go

@@ -22,3 +22,33 @@ func validateCount[K any](kubeletConfigs []K) error {
  }
  return nil
 }
+
+func validateEditInput(poolType string, autoscaling bool, minReplicas int, maxReplicas int, replicas int,
+ isReplicasSet bool, isAutoscalingSet bool, isMinReplicasSet bool, isMaxReplicasSet bool, id string) error {
+
+ if autoscaling && minReplicas <= 0 && isMinReplicasSet {
+ return fmt.Errorf("Min replicas must be a positive number when autoscaling is set")
+ }
+
+ if autoscaling && maxReplicas <= 0 && isMaxReplicasSet {
+ return fmt.Errorf("Max replicas must be a positive number when autoscaling is set")
+ }
+
+ if !autoscaling && replicas <= 0 {
+ return fmt.Errorf("Replicas must be a positive number")
+ }
+
+ if autoscaling && isReplicasSet && isAutoscalingSet {
+ return fmt.Errorf("Autoscaling enabled on %s pool '%s'. can't set replicas", poolType, id)
+ }
+
+ if autoscaling && isAutoscalingSet && maxReplicas < minReplicas {
+ return fmt.Errorf("Max replicas must not be greater than min replicas when autoscaling is enabled")
+ }
+
+ if !autoscaling && (isMinReplicasSet || isMaxReplicasSet) {
+ return fmt.Errorf("Autoscaling disabled on %s pool '%s'. can't set min or max replicas", poolType, id)
+ }
+
+ return nil
+}

pkg/machinepool/validation_test.go

@@ -65,4 +65,48 @@ var _ = Describe("MachinePool validation", func() {
  Expect(err.Error()).To(Equal("Input for kubelet config flag is not valid"))
  })
  })
+ Context("Validate edit machinepool options", func() {
+ It("Fails with autoscaling + replicas set", func() {
+ Expect(validateEditInput("machine", true, 1,
+ 2, 1, true, true, true,
+ true, "test")).ToNot(Succeed())
+ })
+ It("Fails with max, min, and replicas <= 0", func() {
+ Expect(validateEditInput("machine", true, 0,
+ 1, 0, false, true, true,
+ true, "test")).ToNot(Succeed())
+ Expect(validateEditInput("machine", true, 1,
+ 0, 0, false, true, true,
+ true, "test")).ToNot(Succeed())
+ Expect(validateEditInput("machine", false, 0,
+ 0, 0, true, true, false,
+ false, "test")).ToNot(Succeed())
+ })
+ It("Fails with max < min replicas", func() {
+ Expect(validateEditInput("machine", true, 5,
+ 4, 0, false, true, true,
+ true, "test")).ToNot(Succeed())
+ })
+ It("Fails when no autoscaling, but min/max are set", func() {
+ Expect(validateEditInput("machine", false, 1,
+ 0, 1, true, false, true,
+ false, "test")).ToNot(Succeed())
+ Expect(validateEditInput("machine", false, 0,
+ 1, 1, true, false, false,
+ true, "test")).ToNot(Succeed())
+ Expect(validateEditInput("machine", false, 1,
+ 1, 1, true, false, true,
+ true, "test")).ToNot(Succeed())
+ })
+ It("Passes (autoscaling)", func() {
+ Expect(validateEditInput("machine", true, 1,
+ 2, 0, false, true, true,
+ true, "test")).To(Succeed())
+ })
+ It("Passes (not autoscaling)", func() {
+ Expect(validateEditInput("machine", false, 0,
+ 0, 2, true, false, false,
+ false, "test")).To(Succeed())
+ })
+ })
 })