diff --git a/cmd/create/machinepool/cmd.go b/cmd/create/machinepool/cmd.go index ef5ea2ef5f..e6069df9ea 100644 --- a/cmd/create/machinepool/cmd.go +++ b/cmd/create/machinepool/cmd.go @@ -20,19 +20,15 @@ import ( "context" "fmt" - cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" "github.com/spf13/cobra" "github.com/openshift/rosa/pkg/aws" - mpHelpers "github.com/openshift/rosa/pkg/helper/machinepools" "github.com/openshift/rosa/pkg/machinepool" mpOpts "github.com/openshift/rosa/pkg/options/machinepool" "github.com/openshift/rosa/pkg/properties" "github.com/openshift/rosa/pkg/rosa" ) -var args machinepool.MachinePoolArgs - type CreateMachinePoolSpec struct { Service machinepool.MachinePoolService } @@ -53,16 +49,6 @@ func NewCreateMachinePoolCommand() *cobra.Command { return cmd } -// Create machine pool based on cluster type -func (m CreateMachinePool) createMachinePoolBasedOnClusterType(r *rosa.Runtime, - cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, - options *mpOpts.CreateMachinepoolUserOptions) error { - if cluster.Hypershift().Enabled() { - return m.service.CreateNodePools(r, cmd, clusterKey, cluster, options) - } - return m.service.CreateMachinePool(r, cmd, clusterKey, cluster, options) -} - // Original function refactored to use the new helper functions func CreateMachinepoolRunner(userOptions *mpOpts.CreateMachinepoolUserOptions) rosa.CommandRunner { return func(ctx context.Context, r *rosa.Runtime, cmd *cobra.Command, argv []string) error { @@ -70,16 +56,19 @@ func CreateMachinepoolRunner(userOptions *mpOpts.CreateMachinepoolUserOptions) r options := NewCreateMachinepoolOptions() clusterKey := r.GetClusterKey() options.args = userOptions + newService := NewCreateMachinePool(CreateMachinePoolSpec{ + Service: machinepool.NewMachinePoolService(), + }) cluster := r.FetchCluster() - if err := validateClusterState(cluster, clusterKey); err != nil { + if err := machinepool.ValidateClusterState(cluster, clusterKey); err != nil { return err } val, ok := cluster.Properties()[properties.UseLocalCredentials] useLocalCredentials := ok && val == "true" - if err := validateLabels(cmd, options.args); err != nil { + if err := machinepool.ValidateLabels(cmd, options.args); err != nil { return err } @@ -91,28 +80,8 @@ func CreateMachinepoolRunner(userOptions *mpOpts.CreateMachinepoolUserOptions) r if err != nil { return fmt.Errorf("Failed to create awsClient: %s", err) } - service := NewCreateMachinePool(CreateMachinePoolSpec{ - Service: machinepool.NewMachinePoolService(), - }) - return service.createMachinePoolBasedOnClusterType(r, cmd, clusterKey, cluster, options.Machinepool()) - } -} - -// Validate the cluster's state is ready -func validateClusterState(cluster *cmv1.Cluster, clusterKey string) error { - if cluster.State() != cmv1.ClusterStateReady { - return fmt.Errorf("Cluster '%s' is not yet ready", clusterKey) - } - return nil -} - -// Parse labels if the 'labels' flag is set -func validateLabels(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions) error { - if cmd.Flags().Changed("labels") { - if _, err := mpHelpers.ParseLabels(args.Labels); err != nil { - return fmt.Errorf("%s", err) - } + return newService.service.CreateMachinePoolBasedOnClusterType(r, + cmd, clusterKey, cluster, options.Machinepool()) } - return nil } diff --git a/cmd/create/machinepool/cmd_test.go b/cmd/create/machinepool/cmd_test.go index 5e4e11b87f..a12a81b7ab 100644 --- a/cmd/create/machinepool/cmd_test.go +++ b/cmd/create/machinepool/cmd_test.go @@ -28,22 +28,18 @@ var _ = Describe("Create machine pool", func() { When("cluster is classic", func() { It("should create machine pool", func() { serviceMock := machinepool.NewMockMachinePoolService(ctrl) - serviceMock.EXPECT().CreateMachinePool(gomock.Any(), gomock.Any(), + serviceMock.EXPECT().CreateMachinePoolBasedOnClusterType(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1) - createMachinepool := NewCreateMachinePool(CreateMachinePoolSpec{ - Service: serviceMock, - }) - mockClassicClusterReady := test.MockCluster(func(c *cmv1.ClusterBuilder) { c.AWS(cmv1.NewAWS().SubnetIDs("subnet-0b761d44d3d9a4663", "subnet-0f87f640e56934cbc")) c.Region(cmv1.NewCloudRegion().ID("us-east-1")) c.State(cmv1.ClusterStateReady) c.Hypershift(cmv1.NewHypershift().Enabled(false)) }) - err := createMachinepool.createMachinePoolBasedOnClusterType(rosa.NewRuntime(), NewCreateMachinePoolCommand(), + err := serviceMock.CreateMachinePoolBasedOnClusterType(rosa.NewRuntime(), NewCreateMachinePoolCommand(), "82339823", mockClassicClusterReady, NewCreateMachinepoolUserOptions()) - Expect(err).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) }) }) @@ -76,8 +72,8 @@ var _ = Describe("Validation functions", func() { Context("validateClusterState", func() { It("should return nil when the cluster state is ready", func() { - err := validateClusterState(mockClassicClusterReady, "test-cluster") - Expect(err).To(BeNil()) + err := machinepool.ValidateClusterState(mockClassicClusterReady, "test-cluster") + Expect(err).ToNot(HaveOccurred()) }) It("should return an error when the cluster state is not ready", func() { @@ -87,30 +83,29 @@ var _ = Describe("Validation functions", func() { c.State(cmv1.ClusterStateInstalling) c.Hypershift(cmv1.NewHypershift().Enabled(false)) }) - err := validateClusterState(mockClassicClusterInstalling, "test-cluster") + err := machinepool.ValidateClusterState(mockClassicClusterInstalling, "test-cluster") Expect(err).To(MatchError("Cluster 'test-cluster' is not yet ready")) }) }) Context("validateLabels", func() { It("should return nil when the labels flag has not changed", func() { - err := validateLabels(mockCmd, mockArgs) - Expect(err).To(BeNil()) + err := machinepool.ValidateLabels(mockCmd, mockArgs) + Expect(err).ToNot(HaveOccurred()) }) It("should return nil when the labels flag is set with valid labels", func() { mockArgs.Labels = "key1=value1,key2=value2" mockCmd.Flags().Set("labels", "key1=value1,key2=value2") - err := validateLabels(mockCmd, mockArgs) - Expect(err).To(BeNil()) + err := machinepool.ValidateLabels(mockCmd, mockArgs) + Expect(err).ToNot(HaveOccurred()) }) It("should return an error when the labels flag is set with invalid labels", func() { mockArgs.Labels = "key1=value1,key2" mockCmd.Flags().Set("labels", "key1=value1,key2") - err := validateLabels(mockCmd, mockArgs) - Expect(err).NotTo(BeNil()) + err := machinepool.ValidateLabels(mockCmd, mockArgs) + Expect(err).To(HaveOccurred()) }) }) }) - diff --git a/cmd/create/machinepool/options.go b/cmd/create/machinepool/options.go index 3288cf2285..65461ac95f 100644 --- a/cmd/create/machinepool/options.go +++ b/cmd/create/machinepool/options.go @@ -5,6 +5,8 @@ import ( "github.com/openshift/rosa/pkg/reporter" ) +const instanceType = "m5.xlarge" + type CreateMachinepoolOptions struct { reporter *reporter.Object @@ -13,7 +15,7 @@ type CreateMachinepoolOptions struct { func NewCreateMachinepoolUserOptions() *mpOpts.CreateMachinepoolUserOptions { return &mpOpts.CreateMachinepoolUserOptions{ - InstanceType: "m5.xlarge", + InstanceType: instanceType, AutoscalingEnabled: false, MultiAvailabilityZone: true, Autorepair: true, @@ -30,11 +32,3 @@ func NewCreateMachinepoolOptions() *CreateMachinepoolOptions { func (m *CreateMachinepoolOptions) Machinepool() *mpOpts.CreateMachinepoolUserOptions { return m.args } - -func (m *CreateMachinepoolOptions) Bind(args *mpOpts.CreateMachinepoolUserOptions, argv []string) error { - m.args = args - if len(argv) > 0 { - m.args.Name = argv[0] - } - return nil -} diff --git a/cmd/create/machinepool/options_test.go b/cmd/create/machinepool/options_test.go index 077e0edb3d..721a0d1449 100644 --- a/cmd/create/machinepool/options_test.go +++ b/cmd/create/machinepool/options_test.go @@ -19,41 +19,26 @@ var _ = Describe("CreateMachinepoolOptions", func() { userOptions = NewCreateMachinepoolUserOptions() }) - Describe("NewCreateMachinepoolUserOptions", func() { + Context("NewCreateMachinepoolUserOptions", func() { It("should create default user options", func() { - Expect(userOptions.InstanceType).To(Equal("m5.xlarge")) + Expect(userOptions.InstanceType).To(Equal(instanceType)) Expect(userOptions.AutoscalingEnabled).To(BeFalse()) Expect(userOptions.MultiAvailabilityZone).To(BeTrue()) Expect(userOptions.Autorepair).To(BeTrue()) }) }) - Describe("NewCreateMachinepoolOptions", func() { + Context("NewCreateMachinepoolOptions", func() { It("should create default machine pool options", func() { Expect(machinepoolOptions.reporter).To(BeAssignableToTypeOf(&reporter.Object{})) Expect(machinepoolOptions.args).To(BeAssignableToTypeOf(&mpOpts.CreateMachinepoolUserOptions{})) }) }) - Describe("Machinepool", func() { + Context("Machinepool", func() { It("should return the args field", func() { machinepoolOptions.args = userOptions Expect(machinepoolOptions.Machinepool()).To(Equal(userOptions)) }) }) - - Describe("Bind", func() { - It("should bind the passed arguments", func() { - argv := []string{"test-pool"} - Expect(machinepoolOptions.Bind(userOptions, argv)).To(Succeed()) - Expect(machinepoolOptions.args).To(Equal(userOptions)) - Expect(machinepoolOptions.args.Name).To(Equal("test-pool")) - }) - - It("should not modify args.Name if no arguments passed", func() { - initialName := userOptions.Name - Expect(machinepoolOptions.Bind(userOptions, []string{})).To(Succeed()) - Expect(machinepoolOptions.args.Name).To(Equal(initialName)) - }) - }) }) diff --git a/cmd/create/machinepool/suite_test.go b/cmd/create/machinepool/suite_test.go new file mode 100644 index 0000000000..7472b70bc0 --- /dev/null +++ b/cmd/create/machinepool/suite_test.go @@ -0,0 +1,13 @@ +package machinepool + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestMachinepools(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Machinepool/nodepool suite") +} diff --git a/cmd/describe/autoscaler/cmd_test.go b/cmd/describe/autoscaler/cmd_test.go index e86ca069aa..558e6227df 100644 --- a/cmd/describe/autoscaler/cmd_test.go +++ b/cmd/describe/autoscaler/cmd_test.go @@ -196,4 +196,3 @@ var _ = Describe("rosa describe autoscaler", func() { }) }) }) - diff --git a/pkg/machinepool/helper.go b/pkg/machinepool/helper.go index 36230ac282..908e091317 100644 --- a/pkg/machinepool/helper.go +++ b/pkg/machinepool/helper.go @@ -2,7 +2,6 @@ package machinepool import ( "fmt" - "os" "strconv" awssdk "github.com/aws/aws-sdk-go-v2/aws" @@ -12,12 +11,31 @@ import ( "github.com/spf13/cobra" "github.com/openshift/rosa/pkg/aws" + mpHelpers "github.com/openshift/rosa/pkg/helper/machinepools" "github.com/openshift/rosa/pkg/interactive" interactiveSgs "github.com/openshift/rosa/pkg/interactive/securitygroups" mpOpts "github.com/openshift/rosa/pkg/options/machinepool" "github.com/openshift/rosa/pkg/rosa" ) +// Parse labels if the 'labels' flag is set +func ValidateLabels(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions) error { + if cmd.Flags().Changed("labels") { + if _, err := mpHelpers.ParseLabels(args.Labels); err != nil { + return fmt.Errorf("%s", err) + } + } + return nil +} + +// Validate the cluster's state is ready +func ValidateClusterState(cluster *cmv1.Cluster, clusterKey string) error { + if cluster.State() != cmv1.ClusterStateReady { + return fmt.Errorf("Cluster '%s' is not yet ready", clusterKey) + } + return nil +} + func getSubnetFromUser(cmd *cobra.Command, r *rosa.Runtime, isSubnetSet bool, cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) (string, error) { var selectSubnet bool @@ -215,7 +233,7 @@ func spotMaxPriceValidator(val interface{}) error { } func getSubnetFromAvailabilityZone(cmd *cobra.Command, r *rosa.Runtime, isAvailabilityZoneSet bool, - cluster *cmv1.Cluster, args MachinePoolArgs) (string, error) { + cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) (string, error) { privateSubnets, err := r.AWSClient.GetVPCPrivateSubnets(cluster.AWS().SubnetIDs()[0]) if err != nil { @@ -260,7 +278,10 @@ func getSubnetFromAvailabilityZone(cmd *cobra.Command, r *rosa.Runtime, isAvaila } r.Reporter.Infof("There are several subnets for availability zone '%s'", availabilityZone) interactive.Enable() - subnet := getSubnetFromUser(cmd, r, false, cluster, args) + subnet, err := getSubnetFromUser(cmd, r, false, cluster, args) + if err != nil { + return "", err + } return subnet, nil } diff --git a/pkg/machinepool/helper_test.go b/pkg/machinepool/helper_test.go index 6dc9811692..63f7dfbdac 100644 --- a/pkg/machinepool/helper_test.go +++ b/pkg/machinepool/helper_test.go @@ -1,6 +1,11 @@ package machinepool import ( + "fmt" + + gomock "go.uber.org/mock/gomock" + + awssdk "github.com/aws/aws-sdk-go-v2/aws" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -9,10 +14,10 @@ import ( mock "github.com/openshift/rosa/pkg/aws" "github.com/openshift/rosa/pkg/helper/features" + mpOpts "github.com/openshift/rosa/pkg/options/machinepool" "github.com/openshift/rosa/pkg/rosa" "github.com/openshift/rosa/pkg/test" . "github.com/openshift/rosa/pkg/test" - mpOpts "github.com/openshift/rosa/pkg/options/machinepool" ) var _ = Describe("Machine pool helper", func() { @@ -195,7 +200,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() { var ( r *rosa.Runtime cmd *cobra.Command - args *MachinePoolArgs + args *mpOpts.CreateMachinepoolUserOptions mockClient *mock.MockClient az string subnetId1 string @@ -211,7 +216,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() { az = "us-east-1a" subnetId1 = "subnet-123" subnetId2 = "subnet-456" - args = &MachinePoolArgs{} + args = &mpOpts.CreateMachinepoolUserOptions{} }) When("no availability zone is set", func() { @@ -228,7 +233,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() { }) It("returns the correct subnet when one subnet is expected", func() { - subnet, err := getSubnetFromAvailabilityZone(cmd, r, false, cluster, *args) + subnet, err := getSubnetFromAvailabilityZone(cmd, r, false, cluster, args) Expect(err).ToNot(HaveOccurred()) Expect(subnet).To(Equal(subnetId1)) }) @@ -249,7 +254,7 @@ var _ = Describe("getSubnetFromAvailabilityZone functionality", func() { }) It("handles errors correctly when the availability zone does not match", func() { - subnet, err := getSubnetFromAvailabilityZone(cmd, r, true, cluster, *args) + subnet, err := getSubnetFromAvailabilityZone(cmd, r, true, cluster, args) Expect(err).To(HaveOccurred()) Expect(subnet).To(Equal("")) }) diff --git a/pkg/machinepool/machine_pool_args.go b/pkg/machinepool/machine_pool_args.go deleted file mode 100644 index 1ef1772f0b..0000000000 --- a/pkg/machinepool/machine_pool_args.go +++ /dev/null @@ -1,28 +0,0 @@ -package machinepool - -type MachinePoolArgs struct { - Name string - InstanceType string - Replicas int - AutoscalingEnabled bool - MinReplicas int - MaxReplicas int - Labels string - Taints string - UseSpotInstances bool - SpotMaxPrice string - MultiAvailabilityZone bool - AvailabilityZone string - Subnet string - Version string - Autorepair bool - TuningConfigs string - KubeletConfigs string - RootDiskSize string - SecurityGroupIds []string - NodeDrainGracePeriod string - Tags []string - MaxSurge string - MaxUnavailable string - EC2MetadataHttpTokens string -} diff --git a/pkg/machinepool/machinepool.go b/pkg/machinepool/machinepool.go index 62895b9066..3af506e814 100644 --- a/pkg/machinepool/machinepool.go +++ b/pkg/machinepool/machinepool.go @@ -49,10 +49,7 @@ type MachinePoolService interface { DeleteMachinePool(r *rosa.Runtime, machinePoolId string, clusterKey string, cluster *cmv1.Cluster) error EditMachinePool(cmd *cobra.Command, machinePoolID string, clusterKey string, cluster *cmv1.Cluster, r *rosa.Runtime) error - CreateMachinePool(r *rosa.Runtime, cmd *cobra.Command, - clusterKey string, cluster *cmv1.Cluster, - options *mpOpts.CreateMachinepoolUserOptions) error - CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, + CreateMachinePoolBasedOnClusterType(r *rosa.Runtime, cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, options *mpOpts.CreateMachinepoolUserOptions) error } @@ -66,6 +63,16 @@ func NewMachinePoolService() MachinePoolService { return &machinePool{} } +// Create machine pool based on cluster type +func (m machinePool) CreateMachinePoolBasedOnClusterType(r *rosa.Runtime, + cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, + options *mpOpts.CreateMachinepoolUserOptions) error { + if cluster.Hypershift().Enabled() { + return m.CreateNodePools(r, cmd, clusterKey, cluster, options) + } + return m.CreateMachinePool(r, cmd, clusterKey, cluster, options) +} + func (m *machinePool) CreateMachinePool(r *rosa.Runtime, cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) error { @@ -478,76 +485,6 @@ func (m *machinePool) CreateMachinePool(r *rosa.Runtime, cmd *cobra.Command, clu return nil } -// getMachinePoolAvailabilityZones derives the availability zone from the user input or the cluster spec -func getMachinePoolAvailabilityZones(r *rosa.Runtime, cluster *cmv1.Cluster, multiAZMachinePool bool, - availabilityZoneUserInput string, subnetUserInput string) ([]string, error) { - // Single AZ machine pool for a multi-AZ cluster - if cluster.MultiAZ() && !multiAZMachinePool && availabilityZoneUserInput != "" { - return []string{availabilityZoneUserInput}, nil - } - - // Single AZ machine pool for a BYOVPC cluster - if subnetUserInput != "" { - availabilityZone, err := r.AWSClient.GetSubnetAvailabilityZone(subnetUserInput) - if err != nil { - return []string{}, err - } - - return []string{availabilityZone}, nil - } - - // Default option of cluster's nodes availability zones - return cluster.Nodes().AvailabilityZones(), nil -} - -func minReplicaValidator(multiAZMachinePool bool) interactive.Validator { - return func(val interface{}) error { - minReplicas, 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 multiAZMachinePool && minReplicas%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 { - 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") - } - if multiAZMachinePool && maxReplicas%3 != 0 { - return fmt.Errorf("Multi AZ clusters require that the replicas be a multiple of 3") - } - return nil - } -} - -func spotMaxPriceValidator(val interface{}) error { - spotMaxPrice := fmt.Sprintf("%v", val) - if spotMaxPrice == "on-demand" { - return nil - } - price, err := strconv.ParseFloat(spotMaxPrice, commonUtils.MaxByteSize) - if err != nil { - return fmt.Errorf("Expected a numeric value for spot max price") - } - - if price <= 0 { - return fmt.Errorf("Spot max price must be positive") - } - return nil -} - func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) error { @@ -695,8 +632,6 @@ func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clust // Machine pool instance type: // NodePools don't support MultiAZ yet, so the availabilityZonesFilters is calculated from the cluster - - // Machine pool instance type: instanceType := args.InstanceType if instanceType == "" && !interactive.Enabled() { return fmt.Errorf("You must supply a valid instance type") @@ -913,8 +848,7 @@ func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clust }, }) if err != nil { - r.Reporter.Errorf("Expected a valid value for max surge: %s", err) - os.Exit(1) + return fmt.Errorf("Expected a valid value for max surge: %s", err) } } @@ -930,8 +864,7 @@ func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clust }, }) if err != nil { - r.Reporter.Errorf("Expected a valid value for max unavailable: %s", err) - os.Exit(1) + return fmt.Errorf("Expected a valid value for max unavailable: %s", err) } } if maxSurge != "" || maxUnavailable != "" { @@ -974,62 +907,6 @@ func (m *machinePool) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clust return nil } -func getSubnetFromAvailabilityZone(cmd *cobra.Command, r *rosa.Runtime, isAvailabilityZoneSet bool, - cluster *cmv1.Cluster, args *mpOpts.CreateMachinepoolUserOptions) (string, error) { - - privateSubnets, err := r.AWSClient.GetVPCPrivateSubnets(cluster.AWS().SubnetIDs()[0]) - if err != nil { - return "", err - } - - // Fetching the availability zones from the VPC private subnets - subnetsMap := make(map[string][]string) - for _, privateSubnet := range privateSubnets { - subnetsPerAZ, exist := subnetsMap[*privateSubnet.AvailabilityZone] - if !exist { - subnetsPerAZ = []string{*privateSubnet.SubnetId} - } else { - subnetsPerAZ = append(subnetsPerAZ, *privateSubnet.SubnetId) - } - subnetsMap[*privateSubnet.AvailabilityZone] = subnetsPerAZ - } - availabilityZones := make([]string, 0) - for availabilizyZone := range subnetsMap { - availabilityZones = append(availabilityZones, availabilizyZone) - } - - availabilityZone := cluster.Nodes().AvailabilityZones()[0] - if !isAvailabilityZoneSet && interactive.Enabled() { - availabilityZone, err = interactive.GetOption(interactive.Input{ - Question: "AWS availability zone", - Help: cmd.Flags().Lookup("availability-zone").Usage, - Options: availabilityZones, - Default: availabilityZone, - Required: true, - }) - if err != nil { - return "", fmt.Errorf("Expected a valid AWS availability zone: %s", err) - } - } else if isAvailabilityZoneSet { - availabilityZone = args.AvailabilityZone - } - - if subnets, ok := subnetsMap[availabilityZone]; ok { - if len(subnets) == 1 { - return subnets[0], nil - } - r.Reporter.Infof("There are several subnets for availability zone '%s'", availabilityZone) - interactive.Enable() - subnet, err := getSubnetFromUser(cmd, r, false, cluster, args) - if err != nil { - return "", err - } - return subnet, nil - } - - return "", fmt.Errorf("Failed to find a private subnet for '%s' availability zone", availabilityZone) -} - // ListMachinePools lists all machinepools (or, nodepools if hypershift) in a cluster func (m *machinePool) ListMachinePools(r *rosa.Runtime, clusterKey string, cluster *cmv1.Cluster) error { // Load any existing machine pools for this cluster @@ -2016,119 +1893,122 @@ func editAutoscaling(nodePool *cmv1.NodePool, minReplicas int, maxReplicas int) return nil } -func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions, multiAZMachinePool bool, +func manageReplicas(cmd *cobra.Command, args *mpOpts.CreateMachinepoolUserOptions, multiAZMachinePool bool, isMachinePool bool) (minReplicas, maxReplicas, replicas int, autoscaling bool, err error) { - isMinReplicasSet := cmd.Flags().Changed("min-replicas") - isMaxReplicasSet := cmd.Flags().Changed("max-replicas") - isAutoscalingSet := cmd.Flags().Changed("enable-autoscaling") - isReplicasSet := cmd.Flags().Changed("replicas") - - minReplicas = args.MinReplicas - maxReplicas = args.MaxReplicas - autoscaling = args.AutoscalingEnabled - replicas = args.Replicas - - getMinValidator := func(multiAZ bool) interactive.Validator { - if isMachinePool { - return minReplicaValidator(multiAZMachinePool) - } else { - return machinepools.MinNodePoolReplicaValidator(multiAZ) - } - } - - getMaxValidator := func(minReplicas int, multiAZ bool) interactive.Validator { - if isMachinePool { - return maxReplicaValidator(minReplicas, multiAZMachinePool) - } else { - return machinepools.MaxNodePoolReplicaValidator(minReplicas) - } - } - - // Autoscaling - if !isReplicasSet && !autoscaling && !isAutoscalingSet && interactive.Enabled() { - _, err := interactive.GetBool(interactive.Input{ - Question: "Enable autoscaling", - Help: cmd.Flags().Lookup("enable-autoscaling").Usage, - Default: autoscaling, - Required: false, - }) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid value for enable-autoscaling: %s", err) - } - } - - if autoscaling { - // if the user set replicas and enabled autoscaling - if isReplicasSet { - return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Replicas can't be set when autoscaling is enabled") - } - - // Min replicas - if interactive.Enabled() || !isMinReplicasSet { - _, err := interactive.GetInt(interactive.Input{ - Question: "Min replicas", - Help: cmd.Flags().Lookup("min-replicas").Usage, - Default: minReplicas, - Required: true, - Validators: []interactive.Validator{ - getMinValidator(multiAZMachinePool), - }, - }) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid number of min replicas: %s", err) - } - } - err := getMinValidator(multiAZMachinePool)(minReplicas) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, err - } - - // Max replicas - if interactive.Enabled() || !isMaxReplicasSet { - _, err := interactive.GetInt(interactive.Input{ - Question: "Max replicas", - Help: cmd.Flags().Lookup("max-replicas").Usage, - Default: maxReplicas, - Required: true, - Validators: []interactive.Validator{ - getMaxValidator(minReplicas, multiAZMachinePool), - }, - }) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid number of max replicas: %s", err) - } - } - err = getMaxValidator(minReplicas, multiAZMachinePool)(maxReplicas) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, err - } - } else { - // if the user set min/max replicas and hasn't enabled autoscaling - if isMinReplicasSet || isMaxReplicasSet { - return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Autoscaling must be enabled in order to set min and max replicas") - } - - // Replicas - if interactive.Enabled() || !isReplicasSet { - _, err := interactive.GetInt(interactive.Input{ - Question: "Replicas", - Help: cmd.Flags().Lookup("replicas").Usage, - Default: replicas, - Required: true, - Validators: []interactive.Validator{ - getMinValidator(multiAZMachinePool), - }, - }) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid number of replicas: %s", err) - } - } - err := getMinValidator(multiAZMachinePool)(replicas) - if err != nil { - return minReplicas, maxReplicas, replicas, autoscaling, err - } - } - return minReplicas, maxReplicas, replicas, autoscaling, nil -} + isMinReplicasSet := cmd.Flags().Changed("min-replicas") + isMaxReplicasSet := cmd.Flags().Changed("max-replicas") + isAutoscalingSet := cmd.Flags().Changed("enable-autoscaling") + isReplicasSet := cmd.Flags().Changed("replicas") + + minReplicas = args.MinReplicas + maxReplicas = args.MaxReplicas + autoscaling = args.AutoscalingEnabled + replicas = args.Replicas + + getMinValidator := func(multiAZ bool) interactive.Validator { + if isMachinePool { + return minReplicaValidator(multiAZMachinePool) + } else { + return machinepools.MinNodePoolReplicaValidator(multiAZ) + } + } + getMaxValidator := func(minReplicas int, multiAZMachinePool bool) interactive.Validator { + if isMachinePool { + return maxReplicaValidator(minReplicas, multiAZMachinePool) + } else { + return machinepools.MaxNodePoolReplicaValidator(minReplicas) + } + } + // Autoscaling + if !isReplicasSet && !autoscaling && !isAutoscalingSet && interactive.Enabled() { + autoscaling, err = interactive.GetBool(interactive.Input{ + Question: "Enable autoscaling", + Help: cmd.Flags().Lookup("enable-autoscaling").Usage, + Default: autoscaling, + Required: false, + }) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, + fmt.Errorf("Expected a valid value for enable-autoscaling: %s", err) + } + } + + if autoscaling { + // if the user set replicas and enabled autoscaling + if isReplicasSet { + return minReplicas, maxReplicas, replicas, autoscaling, + fmt.Errorf("Replicas can't be set when autoscaling is enabled") + } + + // Min replicas + if interactive.Enabled() || !isMinReplicasSet { + minReplicas, err = interactive.GetInt(interactive.Input{ + Question: "Min replicas", + Help: cmd.Flags().Lookup("min-replicas").Usage, + Default: minReplicas, + Required: true, + Validators: []interactive.Validator{ + getMinValidator(multiAZMachinePool), + }, + }) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, + fmt.Errorf("Expected a valid number of min replicas: %s", err) + } + } + err := getMinValidator(multiAZMachinePool)(minReplicas) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, err + } + + // Max replicas + if interactive.Enabled() || !isMaxReplicasSet { + maxReplicas, err = interactive.GetInt(interactive.Input{ + Question: "Max replicas", + Help: cmd.Flags().Lookup("max-replicas").Usage, + Default: maxReplicas, + Required: true, + Validators: []interactive.Validator{ + getMaxValidator(maxReplicas, multiAZMachinePool), + }, + }) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, + fmt.Errorf("Expected a valid number of max replicas: %s", err) + } + } + err = getMaxValidator(maxReplicas, multiAZMachinePool)(maxReplicas) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, err + } + } else { + // if the user set min/max replicas and hasn't enabled autoscaling + if isMinReplicasSet || isMaxReplicasSet { + return minReplicas, maxReplicas, replicas, autoscaling, + fmt.Errorf("Autoscaling must be enabled in order to set min and max replicas") + } + + // Replicas + if interactive.Enabled() || !isReplicasSet { + replicas, err = interactive.GetInt(interactive.Input{ + Question: "Replicas", + Help: cmd.Flags().Lookup("replicas").Usage, + Default: replicas, + Required: true, + Validators: []interactive.Validator{ + getMinValidator(multiAZMachinePool), + }, + }) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, fmt.Errorf("Expected a valid number of replicas: %s", err) + } + } + err := getMinValidator(multiAZMachinePool)(replicas) + if err != nil { + return minReplicas, maxReplicas, replicas, autoscaling, err + } + } + return minReplicas, maxReplicas, replicas, autoscaling, nil +} diff --git a/pkg/machinepool/machinepool_mock.go b/pkg/machinepool/machinepool_mock.go index b84828ec03..5d1ceefaa7 100644 --- a/pkg/machinepool/machinepool_mock.go +++ b/pkg/machinepool/machinepool_mock.go @@ -42,32 +42,18 @@ func (m *MockMachinePoolService) EXPECT() *MockMachinePoolServiceMockRecorder { return m.recorder } -// CreateMachinePool mocks base method. -func (m *MockMachinePoolService) CreateMachinePool(r *rosa.Runtime, cmd *cobra.Command, clusterKey string, cluster *v1.Cluster, options *machinepool.CreateMachinepoolUserOptions) error { +// CreateMachinePoolBasedOnClusterType mocks base method. +func (m *MockMachinePoolService) CreateMachinePoolBasedOnClusterType(r *rosa.Runtime, cmd *cobra.Command, clusterKey string, cluster *v1.Cluster, options *machinepool.CreateMachinepoolUserOptions) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateMachinePool", r, cmd, clusterKey, cluster, options) + ret := m.ctrl.Call(m, "CreateMachinePoolBasedOnClusterType", r, cmd, clusterKey, cluster, options) ret0, _ := ret[0].(error) return ret0 } -// CreateMachinePool indicates an expected call of CreateMachinePool. -func (mr *MockMachinePoolServiceMockRecorder) CreateMachinePool(r, cmd, clusterKey, cluster, options any) *gomock.Call { +// CreateMachinePoolBasedOnClusterType indicates an expected call of CreateMachinePoolBasedOnClusterType. +func (mr *MockMachinePoolServiceMockRecorder) CreateMachinePoolBasedOnClusterType(r, cmd, clusterKey, cluster, options any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateMachinePool", reflect.TypeOf((*MockMachinePoolService)(nil).CreateMachinePool), r, cmd, clusterKey, cluster, options) -} - -// CreateNodePools mocks base method. -func (m *MockMachinePoolService) CreateNodePools(r *rosa.Runtime, cmd *cobra.Command, clusterKey string, cluster *v1.Cluster, options *machinepool.CreateMachinepoolUserOptions) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateNodePools", r, cmd, clusterKey, cluster, options) - ret0, _ := ret[0].(error) - return ret0 -} - -// CreateNodePools indicates an expected call of CreateNodePools. -func (mr *MockMachinePoolServiceMockRecorder) CreateNodePools(r, cmd, clusterKey, cluster, options any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateNodePools", reflect.TypeOf((*MockMachinePoolService)(nil).CreateNodePools), r, cmd, clusterKey, cluster, options) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateMachinePoolBasedOnClusterType", reflect.TypeOf((*MockMachinePoolService)(nil).CreateMachinePoolBasedOnClusterType), r, cmd, clusterKey, cluster, options) } // DeleteMachinePool mocks base method. diff --git a/pkg/options/machinepool/create.go b/pkg/options/machinepool/create.go index 5534b29963..5e3bad0f2d 100644 --- a/pkg/options/machinepool/create.go +++ b/pkg/options/machinepool/create.go @@ -32,6 +32,9 @@ type CreateMachinepoolUserOptions struct { SecurityGroupIds []string NodeDrainGracePeriod string Tags []string + MaxSurge string + MaxUnavailable string + EC2MetadataHttpTokens string } const ( @@ -251,6 +254,29 @@ func BuildMachinePoolCreateCommandWithOptions() (*cobra.Command, *CreateMachinep "Tags are comma separated, for example: 'key value, foo bar'", ) + flags.StringVar( + &options.EC2MetadataHttpTokens, + "ec2-metadata-http-tokens", + "", + "Should cluster nodes use both v1 and v2 endpoints or just v2 endpoint "+ + "of EC2 Instance Metadata Service (IMDS)"+ + "This flag is only supported for Hosted Control Planes.", + ) + + flags.StringVar(&options.MaxSurge, + "max-surge", + "1", + "The maximum number of nodes that can be provisioned above the desired number of nodes in the machinepool during "+ + "the upgrade. It can be an absolute number i.e. 1, or a percentage i.e. '20%'.", + ) + + flags.StringVar(&options.MaxUnavailable, + "max-unavailable", + "0", + "The maximum number of nodes in the machinepool that can be unavailable during the upgrade. It can be an "+ + "absolute number i.e. 1, or a percentage i.e. '20%'.", + ) + output.AddFlag(cmd) interactive.AddFlag(flags) return cmd, options diff --git a/pkg/options/machinepool/create_test.go b/pkg/options/machinepool/create_test.go index da1b564ea2..b24ca66b09 100644 --- a/pkg/options/machinepool/create_test.go +++ b/pkg/options/machinepool/create_test.go @@ -47,6 +47,8 @@ var _ = Describe("BuildMachinePoolCreateCommandWithOptions", func() { Expect(options.SecurityGroupIds).To(BeNil()) Expect(options.NodeDrainGracePeriod).To(Equal("")) Expect(options.Tags).To(BeNil()) + Expect(options.MaxSurge).To(Equal("1")) + Expect(options.MaxUnavailable).To(Equal("0")) }) It("should have flags set with the correct default values", func() { @@ -114,5 +116,11 @@ var _ = Describe("BuildMachinePoolCreateCommandWithOptions", func() { tags, _ := flags.GetStringSlice("tags") Expect(tags).To(BeEmpty()) + + maxSurge, _ := flags.GetString("max-surge") + Expect(maxSurge).To(Equal("1")) + + maxUnavailable, _ := flags.GetString("max-unavailable") + Expect(maxUnavailable).To(Equal("0")) }) }) diff --git a/pkg/test/helpers.go b/pkg/test/helpers.go index 2341c8ee4b..73d3e22fc1 100644 --- a/pkg/test/helpers.go +++ b/pkg/test/helpers.go @@ -169,155 +169,74 @@ func FormatNodePoolAutoscaling(nodePoolId string) string { FormatResource(nodePool)) } -// FormatMachinePoolList simulates the output of APIs for a fake machine pool list -func FormatMachinePoolList(machinePools []*v1.MachinePool) string { +func FormatList[T any](list []*T, marshalFunc func([]*T, io.Writer) error, kind string) string { var json bytes.Buffer - v1.MarshalMachinePoolList(machinePools, &json) + err := marshalFunc(list, &json) + if err != nil { + return err.Error() + } - return fmt.Sprintf(` - { - "kind": "NodePoolList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(machinePools), len(machinePools), json.String()) + return fmt.Sprintf(`{ + "kind": "%s", + "page": 1, + "size": %d, + "total": %d, + "items": %s + }`, kind, len(list), len(list), json.String()) } -func FormatNodePoolList(nodePools []*v1.NodePool) string { - var json bytes.Buffer - - v1.MarshalNodePoolList(nodePools, &json) +// Example usage for MachinePool and NodePool +func FormatMachinePoolList(machinePools []*v1.MachinePool) string { + return FormatList(machinePools, v1.MarshalMachinePoolList, "MachinePoolList") +} - return fmt.Sprintf(` - { - "kind": "NodePoolList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(nodePools), len(nodePools), json.String()) +func FormatNodePoolList(nodePools []*v1.NodePool) string { + return FormatList(nodePools, v1.MarshalNodePoolList, "NodePoolList") } func FormatKubeletConfigList(configs []*v1.KubeletConfig) string { - var json bytes.Buffer - - v1.MarshalKubeletConfigList(configs, &json) - - return fmt.Sprintf(` - { - "kind": "KubeletConfigList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(configs), len(configs), json.String()) + return FormatList(configs, v1.MarshalKubeletConfigList, "KubeletConfigList") } func FormatClusterList(clusters []*v1.Cluster) string { - var clusterJson bytes.Buffer - - v1.MarshalClusterList(clusters, &clusterJson) - - return fmt.Sprintf(` - { - "kind": "ClusterList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(clusters), len(clusters), clusterJson.String()) + return FormatList(clusters, v1.MarshalClusterList, "ClusterList") } func FormatIngressList(ingresses []*v1.Ingress) string { - var ingressJson bytes.Buffer - - v1.MarshalIngressList(ingresses, &ingressJson) - - return fmt.Sprintf(` - { - "kind": "IngressList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(ingresses), len(ingresses), ingressJson.String()) + return FormatList(ingresses, v1.MarshalIngressList, "IngressList") } func FormatVersionList(versions []*v1.Version) string { - var versionJson bytes.Buffer - - v1.MarshalVersionList(versions, &versionJson) + return FormatList(versions, v1.MarshalVersionList, "VersionList") +} - return fmt.Sprintf(` - { - "kind": "VersionList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(versions), len(versions), versionJson.String()) +func FormatMachineTypeList(mt []*v1.MachineType) string { + return FormatList(mt, v1.MarshalMachineTypeList, "MachineTypeList") } -func FormatIDPList(idps []*v1.IdentityProvider) string { - var idpJson bytes.Buffer +func FormatTuningConfigList(tc []*v1.TuningConfig) string { + return FormatList(tc, v1.MarshalTuningConfigList, "TuningConfigList") +} - v1.MarshalIdentityProviderList(idps, &idpJson) +func FormatQuotaCostList(qc []*amsv1.QuotaCost) string { + return FormatList(qc, amsv1.MarshalQuotaCostList, "QuotaCostList") +} - return fmt.Sprintf(` - { - "kind": "IdentityProviderList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(idps), len(idps), idpJson.String()) +func FormatIDPList(idps []*v1.IdentityProvider) string { + return FormatList(idps, v1.MarshalIdentityProviderList, "IdentityProviderList") } func FormatHtpasswdUserList(htpasswdUsers []*v1.HTPasswdUser) string { - var htpasswdUserJson bytes.Buffer - - v1.MarshalHTPasswdUserList(htpasswdUsers, &htpasswdUserJson) - - return fmt.Sprintf(` - { - "kind": "HTPasswdUserList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(htpasswdUsers), len(htpasswdUsers), htpasswdUserJson.String()) + return FormatList(htpasswdUsers, v1.MarshalHTPasswdUserList, "HTPasswdUserList") } func FormatExternalAuthList(externalAuths []*v1.ExternalAuth) string { - var outputJson bytes.Buffer - - v1.MarshalExternalAuthList(externalAuths, &outputJson) - - return fmt.Sprintf(` - { - "kind": "ExternalAuthList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(externalAuths), len(externalAuths), outputJson.String()) + return FormatList(externalAuths, v1.MarshalExternalAuthList, "ExternalAuthList") } func FormatNodePoolUpgradePolicyList(upgrades []*v1.NodePoolUpgradePolicy) string { - var outputJson bytes.Buffer - - v1.MarshalNodePoolUpgradePolicyList(upgrades, &outputJson) - - return fmt.Sprintf(` - { - "kind": "NodePoolUpgradePolicyList", - "page": 1, - "size": %d, - "total": %d, - "items": %s - }`, len(upgrades), len(upgrades), outputJson.String()) + return FormatList(upgrades, v1.MarshalNodePoolUpgradePolicyList, "NodePoolUpgradePolicyList") } // FormatResource wraps the SDK marshalling and returns a string starting from an object