From 71eb1ff3b10ec0ed1b48eea697d35f2d50001987 Mon Sep 17 00:00:00 2001 From: p0lyn0mial Date: Mon, 29 Jan 2018 09:28:02 +0100 Subject: [PATCH] removes the remainder from ScalerFor method all remaining scalers were replaced by GenericScaler exept JobScaler. It is not clear whether JobScaler could use generic scaler or not. For more details see the pull request. --- .../cmd/util/factory_object_mapping.go | 2 +- pkg/kubectl/delete.go | 2 +- pkg/kubectl/scale.go | 41 ++++++++----------- pkg/kubectl/scale_test.go | 34 +++++++-------- test/e2e/framework/deployment_util.go | 2 - test/e2e/framework/rc_util.go | 2 - test/e2e/framework/util.go | 14 +------ 7 files changed, 39 insertions(+), 58 deletions(-) diff --git a/pkg/kubectl/cmd/util/factory_object_mapping.go b/pkg/kubectl/cmd/util/factory_object_mapping.go index 5c9f01f7d50..7eebe0c10f0 100644 --- a/pkg/kubectl/cmd/util/factory_object_mapping.go +++ b/pkg/kubectl/cmd/util/factory_object_mapping.go @@ -303,7 +303,7 @@ func (f *ring1Factory) Scaler(mapping *meta.RESTMapping) (kubectl.Scaler, error) scalesGetter := scaleclient.New(restClient, mapper, dynamic.LegacyAPIPathResolverFunc, resolver) gvk := mapping.GroupVersionKind.GroupVersion().WithResource(mapping.Resource) - return kubectl.ScalerFor(mapping.GroupVersionKind.GroupKind(), clientset, scalesGetter, gvk.GroupResource()) + return kubectl.ScalerFor(mapping.GroupVersionKind.GroupKind(), clientset.Batch(), scalesGetter, gvk.GroupResource()), nil } func (f *ring1Factory) Reaper(mapping *meta.RESTMapping) (kubectl.Reaper, error) { diff --git a/pkg/kubectl/delete.go b/pkg/kubectl/delete.go index 1522f71b68e..2c8e3267693 100644 --- a/pkg/kubectl/delete.go +++ b/pkg/kubectl/delete.go @@ -347,7 +347,7 @@ func (reaper *StatefulSetReaper) Stop(namespace, name string, timeout time.Durat func (reaper *JobReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *metav1.DeleteOptions) error { jobs := reaper.client.Jobs(namespace) pods := reaper.podClient.Pods(namespace) - scaler := &JobScaler{reaper.client} + scaler := ScalerFor(schema.GroupKind{Group: batch.GroupName, Kind: "Job"}, reaper.client, nil, schema.GroupResource{}) job, err := jobs.Get(name, metav1.GetOptions{}) if err != nil { return err diff --git a/pkg/kubectl/scale.go b/pkg/kubectl/scale.go index 511514df6b8..1d55d2a943d 100644 --- a/pkg/kubectl/scale.go +++ b/pkg/kubectl/scale.go @@ -34,7 +34,6 @@ import ( "k8s.io/kubernetes/pkg/apis/extensions" scaleclient "k8s.io/client-go/scale" - "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" appsclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/apps/internalversion" batchclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/batch/internalversion" coreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion" @@ -54,20 +53,15 @@ type Scaler interface { } // ScalerFor gets a scaler for a given resource -// TODO(p0lyn0mial): remove kind and internalclientset -// TODO(p0lyn0mial): once we have only one scaler, there is no need to return an error anymore. -func ScalerFor(kind schema.GroupKind, c internalclientset.Interface, scalesGetter scaleclient.ScalesGetter, gr schema.GroupResource) (Scaler, error) { +func ScalerFor(kind schema.GroupKind, jobsClient batchclient.JobsGetter, scalesGetter scaleclient.ScalesGetter, gr schema.GroupResource) Scaler { + // it seems like jobs dont't follow "normal" scale semantics. + // For example it is not clear whether HPA could make use of it or not. + // For more details see: https://github.com/kubernetes/kubernetes/pull/58468 switch kind { - case api.Kind("ReplicationController"): - return &ReplicationControllerScaler{c.Core()}, nil - case extensions.Kind("ReplicaSet"), apps.Kind("ReplicaSet"): - return &ReplicaSetScaler{c.Extensions()}, nil case batch.Kind("Job"): - return &JobScaler{c.Batch()}, nil // Either kind of job can be scaled with Batch interface. - case apps.Kind("StatefulSet"): - return &StatefulSetScaler{c.Apps()}, nil + return &jobScaler{jobsClient} // Either kind of job can be scaled with Batch interface. default: - return &GenericScaler{scalesGetter, gr}, nil + return &genericScaler{scalesGetter, gr} } } @@ -167,6 +161,7 @@ func (precondition *ScalePrecondition) ValidateReplicationController(controller return nil } +// TODO(p0lyn0mial): remove ReplicationControllerScaler type ReplicationControllerScaler struct { c coreclient.ReplicationControllersGetter } @@ -265,6 +260,7 @@ func (precondition *ScalePrecondition) ValidateReplicaSet(replicaSet *extensions return nil } +// TODO(p0lyn0mial): remove ReplicaSetScaler type ReplicaSetScaler struct { c extensionsclient.ReplicaSetsGetter } @@ -339,6 +335,7 @@ func (precondition *ScalePrecondition) ValidateJob(job *batch.Job) error { return nil } +// TODO(p0lyn0mial): remove StatefulSetsGetter type StatefulSetScaler struct { c appsclient.StatefulSetsGetter } @@ -395,13 +392,13 @@ func (scaler *StatefulSetScaler) Scale(namespace, name string, newSize uint, pre return nil } -type JobScaler struct { +type jobScaler struct { c batchclient.JobsGetter } // ScaleSimple is responsible for updating job's parallelism. It returns the // resourceVersion of the job if the update is successful. -func (scaler *JobScaler) ScaleSimple(namespace, name string, preconditions *ScalePrecondition, newSize uint) (string, error) { +func (scaler *jobScaler) ScaleSimple(namespace, name string, preconditions *ScalePrecondition, newSize uint) (string, error) { job, err := scaler.c.Jobs(namespace).Get(name, metav1.GetOptions{}) if err != nil { return "", ScaleError{ScaleGetFailure, "", err} @@ -426,7 +423,7 @@ func (scaler *JobScaler) ScaleSimple(namespace, name string, preconditions *Scal // Scale updates a Job to a new size, with optional precondition check (if preconditions is not nil), // optional retries (if retry is not nil), and then optionally waits for parallelism to reach desired // number, which can be less than requested based on job's current progress. -func (scaler *JobScaler) Scale(namespace, name string, newSize uint, preconditions *ScalePrecondition, retry, waitForReplicas *RetryParams) error { +func (scaler *jobScaler) Scale(namespace, name string, newSize uint, preconditions *ScalePrecondition, retry, waitForReplicas *RetryParams) error { if preconditions == nil { preconditions = &ScalePrecondition{-1, ""} } @@ -463,6 +460,7 @@ func (precondition *ScalePrecondition) ValidateDeployment(deployment *extensions return nil } +// TODO(p0lyn0mial): remove DeploymentScaler type DeploymentScaler struct { c extensionsclient.DeploymentsGetter } @@ -534,19 +532,16 @@ func (precondition *ScalePrecondition) validateGeneric(scale *autoscalingapi.Sca return nil } -// GenericScaler can update scales for resources in a particular namespace -// TODO(po0lyn0mial): when the work on GenericScaler is done, don't -// export the GenericScaler. Instead use ScalerFor method for getting the Scaler -// also update the UTs -type GenericScaler struct { +// genericScaler can update scales for resources in a particular namespace +type genericScaler struct { scaleNamespacer scaleclient.ScalesGetter targetGR schema.GroupResource } -var _ Scaler = &GenericScaler{} +var _ Scaler = &genericScaler{} // ScaleSimple updates a scale of a given resource. It returns the resourceVersion of the scale if the update was successful. -func (s *GenericScaler) ScaleSimple(namespace, name string, preconditions *ScalePrecondition, newSize uint) (updatedResourceVersion string, err error) { +func (s *genericScaler) ScaleSimple(namespace, name string, preconditions *ScalePrecondition, newSize uint) (updatedResourceVersion string, err error) { scale, err := s.scaleNamespacer.Scales(namespace).Get(s.targetGR, name) if err != nil { return "", ScaleError{ScaleGetFailure, "", err} @@ -570,7 +565,7 @@ func (s *GenericScaler) ScaleSimple(namespace, name string, preconditions *Scale // Scale updates a scale of a given resource to a new size, with optional precondition check (if preconditions is not nil), // optional retries (if retry is not nil), and then optionally waits for the status to reach desired count. -func (s *GenericScaler) Scale(namespace, resourceName string, newSize uint, preconditions *ScalePrecondition, retry, waitForReplicas *RetryParams) error { +func (s *genericScaler) Scale(namespace, resourceName string, newSize uint, preconditions *ScalePrecondition, retry, waitForReplicas *RetryParams) error { if preconditions == nil { preconditions = &ScalePrecondition{-1, ""} } diff --git a/pkg/kubectl/scale_test.go b/pkg/kubectl/scale_test.go index 1d9b5119eab..417f7ee91a3 100644 --- a/pkg/kubectl/scale_test.go +++ b/pkg/kubectl/scale_test.go @@ -281,13 +281,13 @@ func TestValidateReplicationController(t *testing.T) { } } -type ErrorJobs struct { +type errorJobs struct { batchclient.JobInterface conflict bool invalid bool } -func (c *ErrorJobs) Update(job *batch.Job) (*batch.Job, error) { +func (c *errorJobs) Update(job *batch.Job) (*batch.Job, error) { switch { case c.invalid: return nil, kerrors.NewInvalid(api.Kind(job.Kind), job.Name, nil) @@ -297,7 +297,7 @@ func (c *ErrorJobs) Update(job *batch.Job) (*batch.Job, error) { return nil, errors.New("Job update failure") } -func (c *ErrorJobs) Get(name string, options metav1.GetOptions) (*batch.Job, error) { +func (c *errorJobs) Get(name string, options metav1.GetOptions) (*batch.Job, error) { zero := int32(0) return &batch.Job{ Spec: batch.JobSpec{ @@ -306,14 +306,14 @@ func (c *ErrorJobs) Get(name string, options metav1.GetOptions) (*batch.Job, err }, nil } -type ErrorJobClient struct { +type errorJobClient struct { batchclient.JobsGetter conflict bool invalid bool } -func (c *ErrorJobClient) Jobs(namespace string) batchclient.JobInterface { - return &ErrorJobs{ +func (c *errorJobClient) Jobs(namespace string) batchclient.JobInterface { + return &errorJobs{ JobInterface: c.JobsGetter.Jobs(namespace), conflict: c.conflict, invalid: c.invalid, @@ -321,14 +321,14 @@ func (c *ErrorJobClient) Jobs(namespace string) batchclient.JobInterface { } func TestJobScaleRetry(t *testing.T) { - fake := &ErrorJobClient{JobsGetter: fake.NewSimpleClientset().Batch(), conflict: true} - scaler := JobScaler{fake} + fake := &errorJobClient{JobsGetter: fake.NewSimpleClientset().Batch(), conflict: true} + scaler := ScalerFor(schema.GroupKind{Group: batch.GroupName, Kind: "Job"}, fake, nil, schema.GroupResource{}) preconditions := ScalePrecondition{-1, ""} count := uint(3) name := "foo" namespace := "default" - scaleFunc := ScaleCondition(&scaler, &preconditions, namespace, name, count, nil) + scaleFunc := ScaleCondition(scaler, &preconditions, namespace, name, count, nil) pass, err := scaleFunc() if pass != false { t.Errorf("Expected an update failure to return pass = false, got pass = %v", pass) @@ -337,7 +337,7 @@ func TestJobScaleRetry(t *testing.T) { t.Errorf("Did not expect an error on update failure, got %v", err) } preconditions = ScalePrecondition{3, ""} - scaleFunc = ScaleCondition(&scaler, &preconditions, namespace, name, count, nil) + scaleFunc = ScaleCondition(scaler, &preconditions, namespace, name, count, nil) pass, err = scaleFunc() if err == nil { t.Error("Expected error on precondition failure") @@ -355,7 +355,7 @@ func job() *batch.Job { func TestJobScale(t *testing.T) { fakeClientset := fake.NewSimpleClientset(job()) - scaler := JobScaler{fakeClientset.Batch()} + scaler := ScalerFor(schema.GroupKind{Group: batch.GroupName, Kind: "Job"}, fakeClientset.Batch(), nil, schema.GroupResource{}) preconditions := ScalePrecondition{-1, ""} count := uint(3) name := "foo" @@ -374,14 +374,14 @@ func TestJobScale(t *testing.T) { } func TestJobScaleInvalid(t *testing.T) { - fake := &ErrorJobClient{JobsGetter: fake.NewSimpleClientset().Batch(), invalid: true} - scaler := JobScaler{fake} + fake := &errorJobClient{JobsGetter: fake.NewSimpleClientset().Batch(), invalid: true} + scaler := ScalerFor(schema.GroupKind{Group: batch.GroupName, Kind: "Job"}, fake, nil, schema.GroupResource{}) preconditions := ScalePrecondition{-1, ""} count := uint(3) name := "foo" namespace := "default" - scaleFunc := ScaleCondition(&scaler, &preconditions, namespace, name, count, nil) + scaleFunc := ScaleCondition(scaler, &preconditions, namespace, name, count, nil) pass, err := scaleFunc() if pass { t.Errorf("Expected an update failure to return pass = false, got pass = %v", pass) @@ -403,7 +403,7 @@ func TestJobScaleFailsPreconditions(t *testing.T) { Parallelism: &ten, }, }) - scaler := JobScaler{fake.Batch()} + scaler := ScalerFor(schema.GroupKind{Group: batch.GroupName, Kind: "Job"}, fake.Batch(), nil, schema.GroupResource{}) preconditions := ScalePrecondition{2, ""} count := uint(3) name := "foo" @@ -1425,7 +1425,7 @@ func TestGenericScaleSimple(t *testing.T) { // act for index, scenario := range scenarios { t.Run(fmt.Sprintf("running scenario %d: %s", index+1, scenario.name), func(t *testing.T) { - target := GenericScaler{scenario.scaleGetter, scenario.targetGR} + target := ScalerFor(schema.GroupKind{}, nil, scenario.scaleGetter, scenario.targetGR) resVersion, err := target.ScaleSimple("default", scenario.resName, &scenario.precondition, uint(scenario.newSize)) @@ -1522,7 +1522,7 @@ func TestGenericScale(t *testing.T) { // act for index, scenario := range scenarios { t.Run(fmt.Sprintf("running scenario %d: %s", index+1, scenario.name), func(t *testing.T) { - target := GenericScaler{scenario.scaleGetter, scenario.targetGR} + target := ScalerFor(schema.GroupKind{}, nil, scenario.scaleGetter, scenario.targetGR) err := target.Scale("default", scenario.resName, uint(scenario.newSize), &scenario.precondition, nil, scenario.waitForReplicas) diff --git a/test/e2e/framework/deployment_util.go b/test/e2e/framework/deployment_util.go index d5544e1998e..73d1c52cea0 100644 --- a/test/e2e/framework/deployment_util.go +++ b/test/e2e/framework/deployment_util.go @@ -179,8 +179,6 @@ func WatchRecreateDeployment(c clientset.Interface, d *extensions.Deployment) er return err } -//TODO(p0lyn0mial): remove internalClientset and kind. -//TODO(p0lyn0mial): update the callers. func ScaleDeployment(clientset clientset.Interface, internalClientset internalclientset.Interface, scalesGetter scaleclient.ScalesGetter, ns, name string, size uint, wait bool) error { return ScaleResource(clientset, internalClientset, scalesGetter, ns, name, size, wait, extensionsinternal.Kind("Deployment"), extensionsinternal.Resource("deployments")) } diff --git a/test/e2e/framework/rc_util.go b/test/e2e/framework/rc_util.go index 8bbdb6f4a9b..87d9486cbdd 100644 --- a/test/e2e/framework/rc_util.go +++ b/test/e2e/framework/rc_util.go @@ -159,8 +159,6 @@ func DeleteRCAndPods(clientset clientset.Interface, internalClientset internalcl return DeleteResourceAndPods(clientset, internalClientset, api.Kind("ReplicationController"), ns, name) } -//TODO(p0lyn0mial): remove internalClientset. -//TODO(p0lyn0mial): update the callers. func ScaleRC(clientset clientset.Interface, internalClientset internalclientset.Interface, scalesGetter scaleclient.ScalesGetter, ns, name string, size uint, wait bool) error { return ScaleResource(clientset, internalClientset, scalesGetter, ns, name, size, wait, api.Kind("ReplicationController"), api.Resource("replicationcontrollers")) } diff --git a/test/e2e/framework/util.go b/test/e2e/framework/util.go index 5bec020ecde..c73a388574c 100644 --- a/test/e2e/framework/util.go +++ b/test/e2e/framework/util.go @@ -2683,13 +2683,6 @@ func RemoveAvoidPodsOffNode(c clientset.Interface, nodeName string) { ExpectNoError(err) } -//TODO(p0lyn0mial): remove internalClientset and kind -func getScalerForKind(internalClientset internalclientset.Interface, kind schema.GroupKind, scalesGetter scaleclient.ScalesGetter, gr schema.GroupResource) (kubectl.Scaler, error) { - return kubectl.ScalerFor(kind, internalClientset, scalesGetter, gr) -} - -//TODO(p0lyn0mial): remove internalClientset and kind. -//TODO(p0lyn0mial): update the callers. func ScaleResource( clientset clientset.Interface, internalClientset internalclientset.Interface, @@ -2701,13 +2694,10 @@ func ScaleResource( gr schema.GroupResource, ) error { By(fmt.Sprintf("Scaling %v %s in namespace %s to %d", kind, name, ns, size)) - scaler, err := getScalerForKind(internalClientset, kind, scalesGetter, gr) - if err != nil { - return err - } + scaler := kubectl.ScalerFor(kind, internalClientset.Batch(), scalesGetter, gr) waitForScale := kubectl.NewRetryParams(5*time.Second, 1*time.Minute) waitForReplicas := kubectl.NewRetryParams(5*time.Second, 5*time.Minute) - if err = scaler.Scale(ns, name, size, nil, waitForScale, waitForReplicas); err != nil { + if err := scaler.Scale(ns, name, size, nil, waitForScale, waitForReplicas); err != nil { return fmt.Errorf("error while scaling RC %s to %d replicas: %v", name, size, err) } if !wait {