OCM-10460 | fix: don't overwrite existing autoscaler value in edit flow

openshift · Aug 26, 2024 · bb4d27c · bb4d27c
1 parent f181e00
commit bb4d27c
Show file tree

Hide file tree

Showing 6 changed files with 403 additions and 106 deletions.
diff --git a/cmd/edit/autoscaler/autoscaler_suite_test.go b/cmd/edit/autoscaler/autoscaler_suite_test.go
@@ -0,0 +1,13 @@
+package autoscaler
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestAutoscaler(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Edit Austoscaler Suite")
+}
diff --git a/cmd/edit/autoscaler/cmd.go b/cmd/edit/autoscaler/cmd.go
@@ -17,10 +17,9 @@ limitations under the License.
 package autoscaler
 
 import (
-	"os"
-	"strconv"
+	"context"
+	"fmt"
 
-	commonUtils "github.com/openshift-online/ocm-common/pkg/utils"
 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	"github.com/spf13/cobra"
 
@@ -30,15 +29,13 @@ import (
 	"github.com/openshift/rosa/pkg/rosa"
 )
 
-const argsPrefix string = ""
-
-var Cmd = &cobra.Command{
-	Use:     "autoscaler",
-	Aliases: []string{"cluster-autoscaler"},
-	Short:   "Edit the autoscaler of a cluster",
-	Long: "Configuring cluster-wide autoscaling behavior. At least one machine-pool should " +
-		"have autoscaling enabled for the configuration to be active",
-	Example: `  # Interactively edit an autoscaler to a cluster named "mycluster"
+const (
+	argsPrefix = ""
+	use        = "autoscaler"
+	short      = "Edit the autoscaler of a cluster"
+	long       = "Configuring cluster-wide autoscaling behavior. At least one machine-pool should " +
+		"have autoscaling enabled for the configuration to be active"
+	example = `  # Interactively edit an autoscaler to a cluster named "mycluster"
   rosa edit autoscaler --cluster=mycluster --interactive
 
   # Edit a cluster-autoscaler to skip nodes with local storage
@@ -48,118 +45,90 @@ var Cmd = &cobra.Command{
   rosa edit autoscaler --cluster=mycluster --log-verbosity 3
 
   # Edit a cluster-autoscaler with total CPU constraints
-  rosa edit autoscaler --cluster=mycluster --min-cores 10 --max-cores 100`,
-	Run:  run,
-	Args: cobra.NoArgs,
-}
+  rosa edit autoscaler --cluster=mycluster --min-cores 10 --max-cores 100`
+)
+
+var aliases = []string{"cluster-autoscaler"}
 
-var autoscalerArgs *clusterautoscaler.AutoscalerArgs
+func NewEditAutoscalerCommand() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:     use,
+		Aliases: aliases,
+		Short:   short,
+		Long:    long,
+		Example: example,
+		Args:    cobra.NoArgs,
+	}
 
-func init() {
-	flags := Cmd.Flags()
+	flags := cmd.Flags()
 	flags.SortFlags = false
 
-	ocm.AddClusterFlag(Cmd)
+	ocm.AddClusterFlag(cmd)
 	interactive.AddFlag(flags)
-	autoscalerArgs = clusterautoscaler.AddClusterAutoscalerFlags(Cmd, argsPrefix)
+	autoscalerArgs := clusterautoscaler.AddClusterAutoscalerFlags(cmd, argsPrefix)
+	cmd.Run = rosa.DefaultRunner(rosa.RuntimeWithOCM(), EditAutoscalerRunner(autoscalerArgs))
+	return cmd
 }
 
-func run(cmd *cobra.Command, _ []string) {
-	r := rosa.NewRuntime().WithOCM()
-	defer r.Cleanup()
+func EditAutoscalerRunner(autoscalerArgs *clusterautoscaler.AutoscalerArgs) rosa.CommandRunner {
+	return func(ctx context.Context, r *rosa.Runtime, command *cobra.Command, _ []string) error {
+		clusterKey := r.GetClusterKey()
+		cluster, err := r.OCMClient.GetCluster(clusterKey, r.Creator)
+		if err != nil {
+			return err
+		}
 
-	clusterKey := r.GetClusterKey()
+		if cluster.Hypershift().Enabled() {
+			return fmt.Errorf("Hosted Control Plane clusters do not support cluster-autoscaler configuration")
+		}
 
-	cluster := r.FetchCluster()
+		if cluster.State() != cmv1.ClusterStateReady {
+			return fmt.Errorf("Cluster '%s' is not yet ready. Current state is '%s'", clusterKey, cluster.State())
+		}
 
-	if cluster.Hypershift().Enabled() {
-		r.Reporter.Errorf("Hosted Control Plane clusters do not support cluster-autoscaler configuration")
-		os.Exit(1)
-	}
+		autoscaler, err := r.OCMClient.GetClusterAutoscaler(cluster.ID())
+		if err != nil {
+			return fmt.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
+				cluster.ID(), err)
+		}
 
-	if cluster.State() != cmv1.ClusterStateReady {
-		r.Reporter.Errorf("Cluster '%s' is not yet ready. Current state is '%s'", clusterKey, cluster.State())
-		os.Exit(1)
-	}
+		if autoscaler == nil {
+			return fmt.Errorf("No autoscaler for cluster '%s' has been found. "+
+				"You should first create it via 'rosa create autoscaler'", clusterKey)
+		}
 
-	autoscaler, err := r.OCMClient.GetClusterAutoscaler(cluster.ID())
-	if err != nil {
-		r.Reporter.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
-			cluster.ID(), err)
-		os.Exit(1)
-	}
+		if !clusterautoscaler.IsAutoscalerSetViaCLI(command.Flags(), argsPrefix) && !interactive.Enabled() {
+			interactive.Enable()
+			r.Reporter.Infof("Enabling interactive mode")
+		}
 
-	if autoscaler == nil {
-		r.Reporter.Errorf("No autoscaler for cluster '%s' has been found. "+
-			"You should first create it via 'rosa create autoscaler'", clusterKey)
-		os.Exit(1)
-	}
+		r.Reporter.Debugf("Updating autoscaler for cluster '%s'", clusterKey)
 
-	if !clusterautoscaler.IsAutoscalerSetViaCLI(cmd.Flags(), argsPrefix) && !interactive.Enabled() {
-		interactive.Enable()
-		r.Reporter.Infof("Enabling interactive mode")
-	}
+		autoscalerArgs, err := clusterautoscaler.PrefillAutoscalerArgs(command, autoscalerArgs, autoscaler)
+		if err != nil {
+			return fmt.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
+				cluster.ID(), err)
+		}
 
-	r.Reporter.Debugf("Updating autoscaler for cluster '%s'", clusterKey)
-
-	if interactive.Enabled() {
-		// pre-filling the parameters from the existing resource if running in interactive mode
-
-		autoscalerArgs.BalanceSimilarNodeGroups = autoscaler.BalanceSimilarNodeGroups()
-		autoscalerArgs.SkipNodesWithLocalStorage = autoscaler.SkipNodesWithLocalStorage()
-		autoscalerArgs.LogVerbosity = autoscaler.LogVerbosity()
-		autoscalerArgs.MaxPodGracePeriod = autoscaler.MaxPodGracePeriod()
-		autoscalerArgs.PodPriorityThreshold = autoscaler.PodPriorityThreshold()
-		autoscalerArgs.IgnoreDaemonsetsUtilization = autoscaler.IgnoreDaemonsetsUtilization()
-		autoscalerArgs.MaxNodeProvisionTime = autoscaler.MaxNodeProvisionTime()
-		autoscalerArgs.BalancingIgnoredLabels = autoscaler.BalancingIgnoredLabels()
-		autoscalerArgs.ResourceLimits.MaxNodesTotal = autoscaler.ResourceLimits().MaxNodesTotal()
-		autoscalerArgs.ResourceLimits.Cores.Min = autoscaler.ResourceLimits().Cores().Min()
-		autoscalerArgs.ResourceLimits.Cores.Max = autoscaler.ResourceLimits().Cores().Max()
-		autoscalerArgs.ResourceLimits.Memory.Min = autoscaler.ResourceLimits().Memory().Min()
-		autoscalerArgs.ResourceLimits.Memory.Max = autoscaler.ResourceLimits().Memory().Max()
-
-		// be aware we cannot easily pre-load GPU limits from existing configuration, so we'll have to
-		// request it from scratch when interactive mode is on
-
-		autoscalerArgs.ScaleDown.Enabled = autoscaler.ScaleDown().Enabled()
-		autoscalerArgs.ScaleDown.UnneededTime = autoscaler.ScaleDown().UnneededTime()
-		autoscalerArgs.ScaleDown.DelayAfterAdd = autoscaler.ScaleDown().DelayAfterAdd()
-		autoscalerArgs.ScaleDown.DelayAfterDelete = autoscaler.ScaleDown().DelayAfterDelete()
-		autoscalerArgs.ScaleDown.DelayAfterFailure = autoscaler.ScaleDown().DelayAfterFailure()
-
-		utilizationThreshold, err := strconv.ParseFloat(
-			autoscaler.ScaleDown().UtilizationThreshold(),
-			commonUtils.MaxByteSize,
-		)
+		autoscalerArgs, err = clusterautoscaler.GetAutoscalerOptions(command.Flags(), "", false, autoscalerArgs)
 		if err != nil {
-			r.Reporter.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
+			return fmt.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
 				cluster.ID(), err)
-			os.Exit(1)
 		}
-		autoscalerArgs.ScaleDown.UtilizationThreshold = utilizationThreshold
-	}
 
-	autoscalerArgs, err := clusterautoscaler.GetAutoscalerOptions(cmd.Flags(), "", false, autoscalerArgs)
-	if err != nil {
-		r.Reporter.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
-			cluster.ID(), err)
-		os.Exit(1)
-	}
+		autoscalerConfig, err := clusterautoscaler.CreateAutoscalerConfig(autoscalerArgs)
+		if err != nil {
+			return fmt.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
+				cluster.ID(), err)
+		}
 
-	autoscalerConfig, err := clusterautoscaler.CreateAutoscalerConfig(autoscalerArgs)
-	if err != nil {
-		r.Reporter.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
-			cluster.ID(), err)
-		os.Exit(1)
-	}
+		_, err = r.OCMClient.UpdateClusterAutoscaler(cluster.ID(), autoscalerConfig)
+		if err != nil {
+			return fmt.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
+				cluster.ID(), err)
+		}
 
-	_, err = r.OCMClient.UpdateClusterAutoscaler(cluster.ID(), autoscalerConfig)
-	if err != nil {
-		r.Reporter.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
-			cluster.ID(), err)
-		os.Exit(1)
+		r.Reporter.Infof("Successfully updated autoscaler configuration for cluster '%s'", cluster.ID())
+		return nil
 	}
-
-	r.Reporter.Infof("Successfully updated autoscaler configuration for cluster '%s'", cluster.ID())
 }