Merge pull request #2720 from openshift/revert-2716-cherry-pick-2662-…

…to-release_1.2.49

Revert "[release_1.2.49] OCM-12851| feat: default values for CAS Max Nodes Total and validation added for count of mp worker nodes"

cmd/create/autoscaler/cmd.go

@@ -104,14 +104,7 @@ func CreateAutoscalerRunner(autoscalerArgs *clusterautoscaler.AutoscalerArgs) ro
 
 		r.Reporter.Debugf("Creating autoscaler for cluster '%s'", clusterKey)
 
-		autoscalerValidationArgs := &clusterautoscaler.AutoscalerValidationArgs{
-			ClusterVersion: cluster.OpenshiftVersion(),
-			MultiAz:        cluster.MultiAZ(),
-			IsHostedCp:     cluster.Hypershift().Enabled(),
-		}
-
-		autoscalerArgs, err := clusterautoscaler.GetAutoscalerOptions(
-			command.Flags(), "", false, autoscalerArgs, autoscalerValidationArgs)
+		autoscalerArgs, err := clusterautoscaler.GetAutoscalerOptions(command.Flags(), "", false, autoscalerArgs)
 		if err != nil {
 			return fmt.Errorf("Failed creating autoscaler configuration for cluster '%s': %s",
 				cluster.ID(), err)

cmd/create/cluster/cmd.go

@@ -61,7 +61,6 @@ import (
 	interactiveOidc "github.com/openshift/rosa/pkg/interactive/oidc"
 	"github.com/openshift/rosa/pkg/interactive/securitygroups"
 	interactiveSgs "github.com/openshift/rosa/pkg/interactive/securitygroups"
-	"github.com/openshift/rosa/pkg/machinepool"
 	"github.com/openshift/rosa/pkg/ocm"
 	"github.com/openshift/rosa/pkg/output"
 	"github.com/openshift/rosa/pkg/properties"
@@ -254,7 +253,6 @@ var args struct {
 
 var clusterRegistryConfigArgs *clusterregistryconfig.ClusterRegistryConfigArgs
 var autoscalerArgs *clusterautoscaler.AutoscalerArgs
-var autoscalerValidationArgs *clusterautoscaler.AutoscalerValidationArgs
 var userSpecifiedAutoscalerValues []*pflag.Flag
 
 var Cmd = makeCmd()
@@ -2580,14 +2578,6 @@ func run(cmd *cobra.Command, _ []string) {
 		multiAZ)
 
 	var clusterAutoscaler *clusterautoscaler.AutoscalerArgs
-
-	replicaSizeValidation := &machinepool.ReplicaSizeValidation{
-		MinReplicas:         minReplicas,
-		ClusterVersion:      version,
-		PrivateSubnetsCount: privateSubnetsCount,
-		IsHostedCp:          isHostedCP,
-		MultiAz:             multiAZ,
-	}
 	if !autoscaling {
 		clusterAutoscaler = nil
 	} else {
@@ -2603,20 +2593,19 @@ func run(cmd *cobra.Command, _ []string) {
 				Default:  minReplicas,
 				Required: true,
 				Validators: []interactive.Validator{
-					replicaSizeValidation.MinReplicaValidatorOnClusterCreate(),
+					minReplicaValidator(multiAZ, isHostedCP, privateSubnetsCount),
 				},
 			})
 			if err != nil {
 				r.Reporter.Errorf("Expected a valid number of min replicas: %s", err)
 				os.Exit(1)
 			}
 		}
-		err = replicaSizeValidation.MinReplicaValidatorOnClusterCreate()(minReplicas)
+		err = minReplicaValidator(multiAZ, isHostedCP, privateSubnetsCount)(minReplicas)
 		if err != nil {
 			r.Reporter.Errorf("%s", err)
 			os.Exit(1)
 		}
-		replicaSizeValidation.MinReplicas = minReplicas
 
 		if interactive.Enabled() || !isMaxReplicasSet {
 			maxReplicas, err = interactive.GetInt(interactive.Input{
@@ -2625,15 +2614,15 @@ func run(cmd *cobra.Command, _ []string) {
 				Default:  maxReplicas,
 				Required: true,
 				Validators: []interactive.Validator{
-					replicaSizeValidation.MaxReplicaValidatorOnClusterCreate(),
+					maxReplicaValidator(multiAZ, minReplicas, isHostedCP, privateSubnetsCount),
 				},
 			})
 			if err != nil {
 				r.Reporter.Errorf("Expected a valid number of max replicas: %s", err)
 				os.Exit(1)
 			}
 		}
-		err = replicaSizeValidation.MaxReplicaValidatorOnClusterCreate()(maxReplicas)
+		err = maxReplicaValidator(multiAZ, minReplicas, isHostedCP, privateSubnetsCount)(maxReplicas)
 		if err != nil {
 			r.Reporter.Errorf("%s", err)
 			os.Exit(1)
@@ -2645,15 +2634,8 @@ func run(cmd *cobra.Command, _ []string) {
 				os.Exit(1)
 			}
 		} else {
-
-			autoscalerValidationArgs = &clusterautoscaler.AutoscalerValidationArgs{
-				ClusterVersion: version,
-				MultiAz:        multiAZ,
-				IsHostedCp:     false,
-			}
-
 			clusterAutoscaler, err = clusterautoscaler.GetAutoscalerOptions(
-				cmd.Flags(), clusterAutoscalerFlagsPrefix, true, autoscalerArgs, autoscalerValidationArgs)
+				cmd.Flags(), clusterAutoscalerFlagsPrefix, true, autoscalerArgs)
 			if err != nil {
 				r.Reporter.Errorf("%s", err)
 				os.Exit(1)
@@ -2680,15 +2662,15 @@ func run(cmd *cobra.Command, _ []string) {
 				Help:     cmd.Flags().Lookup("compute-nodes").Usage,
 				Default:  computeNodes,
 				Validators: []interactive.Validator{
-					replicaSizeValidation.MinReplicaValidatorOnClusterCreate(),
+					minReplicaValidator(multiAZ, isHostedCP, privateSubnetsCount),
 				},
 			})
 			if err != nil {
 				r.Reporter.Errorf("Expected a valid number of compute nodes: %s", err)
 				os.Exit(1)
 			}
 		}
-		err = replicaSizeValidation.MinReplicaValidatorOnClusterCreate()(computeNodes)
+		err = minReplicaValidator(multiAZ, isHostedCP, privateSubnetsCount)(computeNodes)
 		if err != nil {
 			r.Reporter.Errorf("%s", err)
 			os.Exit(1)
@@ -3763,6 +3745,39 @@ func isValidCidrRange(
 		!serviceNetwork.Contains(subnetIP)
 }
 
+func minReplicaValidator(multiAZ bool, isHostedCP bool, privateSubnetsCount int) interactive.Validator {
+	return func(val interface{}) error {
+		minReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
+		if err != nil {
+			return err
+		}
+
+		if isHostedCP && minReplicas < 2 {
+			return fmt.Errorf("hosted Control Plane clusters require a minimum of 2 nodes, "+
+				"but %d was requested", minReplicas)
+		}
+
+		return clustervalidations.MinReplicasValidator(minReplicas, multiAZ, isHostedCP, privateSubnetsCount)
+	}
+}
+
+func maxReplicaValidator(multiAZ bool, minReplicas int, isHostedCP bool,
+	privateSubnetsCount int) interactive.Validator {
+	return func(val interface{}) error {
+		maxReplicas, err := strconv.Atoi(fmt.Sprintf("%v", val))
+		if err != nil {
+			return err
+		}
+		return clustervalidations.MaxReplicasValidator(
+			minReplicas,
+			maxReplicas,
+			multiAZ,
+			isHostedCP,
+			privateSubnetsCount,
+		)
+	}
+}
+
 const (
 	HostPrefixMin = 23
 	HostPrefixMax = 26

cmd/edit/autoscaler/cmd.go

@@ -110,14 +110,7 @@ func EditAutoscalerRunner(autoscalerArgs *clusterautoscaler.AutoscalerArgs) rosa
 				cluster.ID(), err)
 		}
 
-		autoscalerValidationArgs := &clusterautoscaler.AutoscalerValidationArgs{
-			ClusterVersion: cluster.OpenshiftVersion(),
-			MultiAz:        cluster.MultiAZ(),
-			IsHostedCp:     cluster.Hypershift().Enabled(),
-		}
-
-		autoscalerArgs, err = clusterautoscaler.GetAutoscalerOptions(
-			command.Flags(), "", false, autoscalerArgs, autoscalerValidationArgs)
+		autoscalerArgs, err = clusterautoscaler.GetAutoscalerOptions(command.Flags(), "", false, autoscalerArgs)
 		if err != nil {
 			return fmt.Errorf("Failed updating autoscaler configuration for cluster '%s': %s",
 				cluster.ID(), err)

pkg/clusterautoscaler/flags.go

@@ -10,7 +10,6 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
-	"github.com/openshift/rosa/pkg/helper/versions"
 	"github.com/openshift/rosa/pkg/interactive"
 	"github.com/openshift/rosa/pkg/ocm"
 )
@@ -87,12 +86,6 @@ type ScaleDownConfig struct {
 	DelayAfterFailure    string
 }
 
-type AutoscalerValidationArgs struct {
-	IsHostedCp     bool
-	ClusterVersion string
-	MultiAz        bool
-}
-
 func AddClusterAutoscalerFlags(cmd *cobra.Command, prefix string) *AutoscalerArgs {
 	args := &AutoscalerArgs{}
 
@@ -160,7 +153,7 @@ func AddClusterAutoscalerFlags(cmd *cobra.Command, prefix string) *AutoscalerArg
 	cmd.Flags().IntVar(
 		&args.ResourceLimits.MaxNodesTotal,
 		fmt.Sprintf("%s%s", prefix, maxNodesTotalFlag),
-		0,
+		249,
 		"Total amount of nodes that can exist in the cluster, including non-scaled nodes.",
 	)
 
@@ -266,7 +259,7 @@ func AddClusterAutoscalerFlags(cmd *cobra.Command, prefix string) *AutoscalerArg
 
 func GetAutoscalerOptions(
 	cmd *pflag.FlagSet, prefix string, confirmBeforeAllArgs bool, autoscalerArgs *AutoscalerArgs,
-	autoscalerValidationArgs *AutoscalerValidationArgs) (*AutoscalerArgs, error) {
+) (*AutoscalerArgs, error) {
 
 	var err error
 	result := &AutoscalerArgs{}
@@ -446,8 +439,7 @@ func GetAutoscalerOptions(
 			Question: "Maximum amount of nodes in the cluster",
 			Help:     cmd.Lookup(fmt.Sprintf("%s%s", prefix, maxNodesTotalFlag)).Usage,
 			Required: false,
-			Default: getAutoscalerMaxNodesTotalDefaultValue(
-				result.ResourceLimits.MaxNodesTotal, autoscalerValidationArgs),
+			Default:  result.ResourceLimits.MaxNodesTotal,
 			Validators: []interactive.Validator{
 				ocm.NonNegativeInt32Validator,
 			},
@@ -953,37 +945,3 @@ func PrefillAutoscalerArgs(cmd *cobra.Command, autoscalerArgs *AutoscalerArgs,
 	}
 	return autoscalerArgs, nil
 }
-
-// temporary fn until calculated default values can be retrieved from single source of truth
-func getAutoscalerMaxNodesTotalDefaultValue(
-	prefilledMaxNodesTotalValue int, autoscalerValidationArgs *AutoscalerValidationArgs) int {
-	if prefilledMaxNodesTotalValue > 0 {
-		return prefilledMaxNodesTotalValue
-	}
-
-	// hosted cp maxNodesTotal default value calculation
-	hostedCpWorkerNodeCount := 500
-	if autoscalerValidationArgs.IsHostedCp {
-		return hostedCpWorkerNodeCount
-
-	}
-
-	// classic maxNodesTotal default value calculation
-	workerNodeCount := 180
-	infraNodeCount := 2
-	masterNodeCount := 3
-	classicMaxNodeSize249SupportedVersion, err := versions.IsGreaterThanOrEqual(
-		autoscalerValidationArgs.ClusterVersion, ocm.ClassicMaxNodeSize249Support)
-	if err != nil {
-		return workerNodeCount + infraNodeCount + masterNodeCount
-	}
-
-	if classicMaxNodeSize249SupportedVersion {
-		workerNodeCount = 249
-	}
-	if autoscalerValidationArgs.MultiAz {
-		infraNodeCount = 3
-	}
-
-	return workerNodeCount + infraNodeCount + masterNodeCount
-}

pkg/clusterautoscaler/flags_test.go

@@ -49,84 +49,3 @@ var _ = Describe("Cluster Autoscaler flags", func() {
 		Expect(prefilledArgs.ResourceLimits.Cores.Min).To(Equal(10))
 	})
 })
-
-var _ = Describe("getAutoscalerMaxNodesTotalDefaultValue function", func() {
-	// Classic cluster maxNodesTotal default value calculation
-	//180 + (default flavour) 3 master + (multi az) 3 infra
-	It("returns 186 worker nodes for classic cluster under v4.14.14", func() {
-		previousValue := 0
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.13",
-			MultiAz:        true,
-			IsHostedCp:     false,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(186))
-	})
-	//180 + (default flavour) 3 master + (single az) 2 infra
-	It("returns 185 worker nodes for classic cluster under v4.14.14", func() {
-		previousValue := 0
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.13",
-			MultiAz:        false,
-			IsHostedCp:     false,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(185))
-	})
-	//249 + (default flavour) 3 master + (single az) 2 infra
-	It("returns 255 worker nodes for classic cluster at or above v4.14.14", func() {
-		previousValue := 0
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.14",
-			MultiAz:        true,
-			IsHostedCp:     false,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(255))
-	})
-	//249 + (default flavour) 3 master + (single az) 2 infra
-	It("returns 254 worker nodes for classic cluster at or above v4.14.14", func() {
-		previousValue := 0
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.14",
-			MultiAz:        false,
-			IsHostedCp:     false,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(254))
-	})
-	//returns value which was set previously
-	It("returns prefilled value worker node count for classic cluster", func() {
-		previousValue := 100
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.14",
-			MultiAz:        true,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(100))
-	})
-
-	// Hosted CP cluster maxNodesTotal default value calculation (MultiAz bool does not affect outcome)
-	It("returns 500 worker nodes for hosted cp cluster", func() {
-		previousValue := 0
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.14",
-			MultiAz:        true,
-			IsHostedCp:     true,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(500))
-	})
-	//returns value which was set previously
-	It("returns prefilled value worker node count for classic cluster", func() {
-		previousValue := 100
-		autoscalerValidationArgs := &AutoscalerValidationArgs{
-			ClusterVersion: "v4.14.14",
-			MultiAz:        true,
-			IsHostedCp:     true,
-		}
-		maxNodesTotalDefaultValue := getAutoscalerMaxNodesTotalDefaultValue(previousValue, autoscalerValidationArgs)
-		Expect(maxNodesTotalDefaultValue).To(Equal(100))
-	})
-})