From d246cf3864ee29a05b28199e3be4224430a99bb7 Mon Sep 17 00:00:00 2001 From: wackxu Date: Thu, 16 Nov 2017 15:10:38 +0800 Subject: [PATCH 1/2] Use structured generator for kubectl autoscale --- pkg/kubectl/autoscale.go | 109 ++++++++++----------------- pkg/kubectl/autoscale_test.go | 134 ++++++++++++++-------------------- 2 files changed, 96 insertions(+), 147 deletions(-) diff --git a/pkg/kubectl/autoscale.go b/pkg/kubectl/autoscale.go index dcbfadde84a..fbe826d07bf 100644 --- a/pkg/kubectl/autoscale.go +++ b/pkg/kubectl/autoscale.go @@ -18,98 +18,69 @@ package kubectl import ( "fmt" - "strconv" autoscalingv1 "k8s.io/api/autoscaling/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) -type HorizontalPodAutoscalerV1 struct{} - -func (HorizontalPodAutoscalerV1) ParamNames() []GeneratorParam { - return []GeneratorParam{ - {"default-name", true}, - {"name", false}, - {"scaleRef-kind", false}, - {"scaleRef-name", false}, - {"scaleRef-apiVersion", false}, - {"min", false}, - {"max", true}, - {"cpu-percent", false}, - } +// HorizontalPodAutoscalerV1Generator supports stable generation of a horizontal pod autoscaler. +type HorizontalPodAutoscalerGeneratorV1 struct { + Name string + ScaleRefKind string + ScaleRefName string + ScaleRefApiVersion string + MinReplicas int32 + MaxReplicas int32 + CPUPercent int32 } -func (HorizontalPodAutoscalerV1) Generate(genericParams map[string]interface{}) (runtime.Object, error) { - return generateHPA(genericParams) -} +// Ensure it supports the generator pattern that uses parameters specified during construction. +var _ StructuredGenerator = &HorizontalPodAutoscalerGeneratorV1{} -func generateHPA(genericParams map[string]interface{}) (runtime.Object, error) { - params := map[string]string{} - for key, value := range genericParams { - strVal, isString := value.(string) - if !isString { - return nil, fmt.Errorf("expected string, saw %v for '%s'", value, key) - } - params[key] = strVal - } - - name, found := params["name"] - if !found || len(name) == 0 { - name, found = params["default-name"] - if !found || len(name) == 0 { - return nil, fmt.Errorf("'name' is a required parameter.") - } - } - minString, found := params["min"] - min := -1 - var err error - if found { - if min, err = strconv.Atoi(minString); err != nil { - return nil, err - } - } - maxString, found := params["max"] - if !found { - return nil, fmt.Errorf("'max' is a required parameter.") - } - max, err := strconv.Atoi(maxString) - if err != nil { +// StructuredGenerate outputs a horizontal pod autoscaler object using the configured fields. +func (s *HorizontalPodAutoscalerGeneratorV1) StructuredGenerate() (runtime.Object, error) { + if err := s.validate(); err != nil { return nil, err } - if min > max { - return nil, fmt.Errorf("'max' must be greater than or equal to 'min'.") - } - - cpuString, found := params["cpu-percent"] - cpu := -1 - if found { - if cpu, err = strconv.Atoi(cpuString); err != nil { - return nil, err - } - } - scaler := autoscalingv1.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ - Name: name, + Name: s.Name, }, Spec: autoscalingv1.HorizontalPodAutoscalerSpec{ ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{ - Kind: params["scaleRef-kind"], - Name: params["scaleRef-name"], - APIVersion: params["scaleRef-apiVersion"], + Kind: s.ScaleRefKind, + Name: s.ScaleRefName, + APIVersion: s.ScaleRefApiVersion, }, - MaxReplicas: int32(max), + MaxReplicas: s.MaxReplicas, }, } - if min > 0 { - v := int32(min) + + if s.MinReplicas > 0 { + v := int32(s.MinReplicas) scaler.Spec.MinReplicas = &v } - if cpu >= 0 { - c := int32(cpu) + if s.CPUPercent >= 0 { + c := int32(s.CPUPercent) scaler.Spec.TargetCPUUtilizationPercentage = &c } + return &scaler, nil } + +// validate check if the caller has set the right fields. +func (s HorizontalPodAutoscalerGeneratorV1) validate() error { + if len(s.Name) == 0 { + return fmt.Errorf("name must be specified") + } + if s.MaxReplicas <= 0 { + return fmt.Errorf("'max' is a required parameter and must be greater than zero") + } + if s.MinReplicas > s.MaxReplicas { + return fmt.Errorf("'max' must be greater than or equal to 'min'") + } + return nil +} + diff --git a/pkg/kubectl/autoscale_test.go b/pkg/kubectl/autoscale_test.go index e87a7075c13..9d2a52811c2 100644 --- a/pkg/kubectl/autoscale_test.go +++ b/pkg/kubectl/autoscale_test.go @@ -26,22 +26,26 @@ import ( func TestHPAGenerate(t *testing.T) { tests := []struct { - name string - params map[string]interface{} - expected *autoscalingv1.HorizontalPodAutoscaler - expectErr bool + name string + HPAName string + scaleRefKind string + scaleRefName string + scaleRefApiVersion string + minReplicas int32 + maxReplicas int32 + CPUPercent int32 + expected *autoscalingv1.HorizontalPodAutoscaler + expectErr bool }{ { - name: "valid case", - params: map[string]interface{}{ - "name": "foo", - "min": "1", - "max": "10", - "cpu-percent": "80", - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, + name: "valid case", + HPAName: "foo", + minReplicas: 1, + maxReplicas: 10, + CPUPercent: 80, + scaleRefKind: "kind", + scaleRefName: "name", + scaleRefApiVersion: "apiVersion", expected: &autoscalingv1.HorizontalPodAutoscaler{ ObjectMeta: metav1.ObjectMeta{ Name: "foo", @@ -60,79 +64,53 @@ func TestHPAGenerate(t *testing.T) { expectErr: false, }, { - name: "'name' is a required parameter", - params: map[string]interface{}{ - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, - expectErr: true, + name: "'name' is a required parameter", + scaleRefKind: "kind", + scaleRefName: "name", + scaleRefApiVersion: "apiVersion", + expectErr: true, }, { - name: "'max' is a required parameter", - params: map[string]interface{}{ - "default-name": "foo", - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, - expectErr: true, + name: "'max' is a required parameter", + HPAName: "foo", + scaleRefKind: "kind", + scaleRefName: "name", + scaleRefApiVersion: "apiVersion", + expectErr: true, }, { - name: "'max' must be greater than or equal to 'min'", - params: map[string]interface{}{ - "name": "foo", - "min": "10", - "max": "1", - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, - expectErr: true, + name: "'max' must be greater than or equal to 'min'", + HPAName: "foo", + minReplicas: 10, + maxReplicas: 1, + scaleRefKind: "kind", + scaleRefName: "name", + scaleRefApiVersion: "apiVersion", + expectErr: true, }, { - name: "cpu-percent must be an integer if specified", - params: map[string]interface{}{ - "name": "foo", - "min": "1", - "max": "10", - "cpu-percent": "", - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, - expectErr: true, - }, - { - name: "'min' must be an integer if specified", - params: map[string]interface{}{ - "name": "foo", - "min": "foo", - "max": "10", - "cpu-percent": "60", - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, - expectErr: true, - }, - { - name: "'max' must be an integer if specified", - params: map[string]interface{}{ - "name": "foo", - "min": "1", - "max": "bar", - "cpu-percent": "90", - "scaleRef-kind": "kind", - "scaleRef-name": "name", - "scaleRef-apiVersion": "apiVersion", - }, - expectErr: true, + name: "'max' must be greater than zero", + HPAName: "foo", + minReplicas: 1, + maxReplicas: -10, + scaleRefKind: "kind", + scaleRefName: "name", + scaleRefApiVersion: "apiVersion", + expectErr: true, }, } - generator := HorizontalPodAutoscalerV1{} + for _, test := range tests { - obj, err := generator.Generate(test.params) + generator := HorizontalPodAutoscalerGeneratorV1{ + Name: test.HPAName, + ScaleRefKind: test.scaleRefKind, + ScaleRefName: test.scaleRefName, + ScaleRefApiVersion: test.scaleRefApiVersion, + MinReplicas: test.minReplicas, + MaxReplicas: test.maxReplicas, + CPUPercent: test.CPUPercent, + } + obj, err := generator.StructuredGenerate() if test.expectErr && err != nil { continue } From 87054639a27a18bd9dd27effabac935f058f272e Mon Sep 17 00:00:00 2001 From: wackxu Date: Thu, 16 Nov 2017 17:29:47 +0800 Subject: [PATCH 2/2] refactor kubectl autoscale to use the new generator --- pkg/kubectl/autoscale.go | 5 +- pkg/kubectl/autoscale_test.go | 2 +- pkg/kubectl/cmd/autoscale.go | 50 ++++++++----------- pkg/kubectl/cmd/util/factory_client_access.go | 4 -- 4 files changed, 24 insertions(+), 37 deletions(-) diff --git a/pkg/kubectl/autoscale.go b/pkg/kubectl/autoscale.go index fbe826d07bf..39c78ca31df 100644 --- a/pkg/kubectl/autoscale.go +++ b/pkg/kubectl/autoscale.go @@ -75,12 +75,11 @@ func (s HorizontalPodAutoscalerGeneratorV1) validate() error { if len(s.Name) == 0 { return fmt.Errorf("name must be specified") } - if s.MaxReplicas <= 0 { - return fmt.Errorf("'max' is a required parameter and must be greater than zero") + if s.MaxReplicas < 1 { + return fmt.Errorf("'max' is a required parameter and must be at least 1") } if s.MinReplicas > s.MaxReplicas { return fmt.Errorf("'max' must be greater than or equal to 'min'") } return nil } - diff --git a/pkg/kubectl/autoscale_test.go b/pkg/kubectl/autoscale_test.go index 9d2a52811c2..86b6c1eab55 100644 --- a/pkg/kubectl/autoscale_test.go +++ b/pkg/kubectl/autoscale_test.go @@ -89,7 +89,7 @@ func TestHPAGenerate(t *testing.T) { expectErr: true, }, { - name: "'max' must be greater than zero", + name: "'max' must be at least 1", HPAName: "foo", minReplicas: 1, maxReplicas: -10, diff --git a/pkg/kubectl/cmd/autoscale.go b/pkg/kubectl/cmd/autoscale.go index 1166674c82d..d16d43383eb 100644 --- a/pkg/kubectl/cmd/autoscale.go +++ b/pkg/kubectl/cmd/autoscale.go @@ -64,11 +64,11 @@ func NewCmdAutoscale(f cmdutil.Factory, out io.Writer) *cobra.Command { ArgAliases: argAliases, } cmdutil.AddPrinterFlags(cmd) - cmd.Flags().String("generator", "horizontalpodautoscaler/v1", i18n.T("The name of the API generator to use. Currently there is only 1 generator.")) - cmd.Flags().Int("min", -1, "The lower limit for the number of pods that can be set by the autoscaler. If it's not specified or negative, the server will apply a default value.") - cmd.Flags().Int("max", -1, "The upper limit for the number of pods that can be set by the autoscaler. Required.") + cmd.Flags().String("generator", cmdutil.HorizontalPodAutoscalerV1GeneratorName, i18n.T("The name of the API generator to use. Currently there is only 1 generator.")) + cmd.Flags().Int32("min", -1, "The lower limit for the number of pods that can be set by the autoscaler. If it's not specified or negative, the server will apply a default value.") + cmd.Flags().Int32("max", -1, "The upper limit for the number of pods that can be set by the autoscaler. Required.") cmd.MarkFlagRequired("max") - cmd.Flags().Int("cpu-percent", -1, fmt.Sprintf("The target average CPU utilization (represented as a percent of requested CPU) over all the pods. If it's not specified or negative, a default autoscaling policy will be used.")) + cmd.Flags().Int32("cpu-percent", -1, fmt.Sprintf("The target average CPU utilization (represented as a percent of requested CPU) over all the pods. If it's not specified or negative, a default autoscaling policy will be used.")) cmd.Flags().String("name", "", i18n.T("The name for the newly created object. If not specified, the name of the input resource will be used.")) cmdutil.AddDryRunFlag(cmd) usage := "identifying the resource to autoscale." @@ -102,15 +102,6 @@ func RunAutoscale(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []s return err } - // Get the generator, setup and validate all required parameters - generatorName := cmdutil.GetFlagString(cmd, "generator") - generators := f.Generators("autoscale") - generator, found := generators[generatorName] - if !found { - return cmdutil.UsageErrorf(cmd, "generator %q not found.", generatorName) - } - names := generator.ParamNames() - count := 0 err = r.Visit(func(info *resource.Info, err error) error { if err != nil { @@ -122,24 +113,25 @@ func RunAutoscale(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []s return err } - name := info.Name - params := kubectl.MakeParams(cmd, names) - params["default-name"] = name - - params["scaleRef-kind"] = mapping.GroupVersionKind.Kind - params["scaleRef-name"] = name - params["scaleRef-apiVersion"] = mapping.GroupVersionKind.GroupVersion().String() - - if err = kubectl.ValidateParams(names, params); err != nil { - return err - } - // Check for invalid flags used against the present generator. - if err := kubectl.EnsureFlagsValid(cmd, generators, generatorName); err != nil { - return err + // get the generator + var generator kubectl.StructuredGenerator + switch generatorName := cmdutil.GetFlagString(cmd, "generator"); generatorName { + case cmdutil.HorizontalPodAutoscalerV1GeneratorName: + generator = &kubectl.HorizontalPodAutoscalerGeneratorV1{ + Name: info.Name, + MinReplicas: cmdutil.GetFlagInt32(cmd, "min"), + MaxReplicas: cmdutil.GetFlagInt32(cmd, "max"), + CPUPercent: cmdutil.GetFlagInt32(cmd, "cpu-percent"), + ScaleRefName: info.Name, + ScaleRefKind: mapping.GroupVersionKind.Kind, + ScaleRefApiVersion: mapping.GroupVersionKind.GroupVersion().String(), + } + default: + return errUnsupportedGenerator(cmd, generatorName) } // Generate new object - object, err := generator.Generate(params) + object, err := generator.StructuredGenerate() if err != nil { return err } @@ -193,7 +185,7 @@ func RunAutoscale(f cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []s func validateFlags(cmd *cobra.Command) error { errs := []error{} - max, min := cmdutil.GetFlagInt(cmd, "max"), cmdutil.GetFlagInt(cmd, "min") + max, min := cmdutil.GetFlagInt32(cmd, "max"), cmdutil.GetFlagInt32(cmd, "min") if max < 1 { errs = append(errs, fmt.Errorf("--max=MAXPODS is required and must be at least 1, max: %d", max)) } diff --git a/pkg/kubectl/cmd/util/factory_client_access.go b/pkg/kubectl/cmd/util/factory_client_access.go index ea783b99e9f..ef389d9ff86 100644 --- a/pkg/kubectl/cmd/util/factory_client_access.go +++ b/pkg/kubectl/cmd/util/factory_client_access.go @@ -568,10 +568,6 @@ func DefaultGenerators(cmdName string) map[string]kubectl.Generator { CronJobV2Alpha1GeneratorName: kubectl.CronJobV2Alpha1{}, CronJobV1Beta1GeneratorName: kubectl.CronJobV1Beta1{}, } - case "autoscale": - generator = map[string]kubectl.Generator{ - HorizontalPodAutoscalerV1GeneratorName: kubectl.HorizontalPodAutoscalerV1{}, - } case "namespace": generator = map[string]kubectl.Generator{ NamespaceV1GeneratorName: kubectl.NamespaceGeneratorV1{},