Skip to content

Commit

Permalink
Merge pull request #2301 from openshift-cherrypick-robot/cherry-pick-…
Browse files Browse the repository at this point in the history
…2289-to-release_1.2.43

[release_1.2.43] OCM-9917 | fix: Issue with interactive + replicas edit machinepool
  • Loading branch information
davidleerh authored Jul 29, 2024
2 parents d522829 + 1a396b5 commit c356d21
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 12 deletions.
9 changes: 6 additions & 3 deletions cmd/edit/machinepool/cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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())
})
})

Expand Down
3 changes: 3 additions & 0 deletions cmd/edit/machinepool/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
28 changes: 19 additions & 9 deletions pkg/machinepool/machinepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -1399,7 +1399,6 @@ func getMachinePoolReplicas(cmd *cobra.Command,
}

if autoscaling {

// set default values from previous autoscaling values
if !isMinReplicasSet {
minReplicas = existingAutoscaling.MinReplicas()
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
}

Expand Down Expand Up @@ -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))) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -2010,6 +2016,10 @@ func getNodePoolReplicas(cmd *cobra.Command,
return
}
}

err = validateEditInput("node", autoscaling, minReplicas, maxReplicas, replicas, isReplicasSet,
isAutoscalingSet, isMinReplicasSet, isMaxReplicasSet, nodePoolID)

return
}

Expand Down
30 changes: 30 additions & 0 deletions pkg/machinepool/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
44 changes: 44 additions & 0 deletions pkg/machinepool/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
})
})

0 comments on commit c356d21

Please sign in to comment.