From e894dcc8ff526f86944aaf9bdb5895fe483ecdc0 Mon Sep 17 00:00:00 2001 From: Michail Kargakis Date: Thu, 3 Dec 2015 19:53:40 +0100 Subject: [PATCH] kubectl: Make scaling smarter Skip updating resources that already meet the desired replica count. This change has an impact in both kubectl scale and kubectl delete in that reapable resources that already have the desired replicas (number provided via --replicas for scale, or zero for delete) won't be updated again and a "already scaled" message will be printed (in case of scale). --- pkg/client/unversioned/conditions.go | 2 +- pkg/kubectl/cmd/scale.go | 4 + pkg/kubectl/scale.go | 26 +- pkg/kubectl/scale_test.go | 76 +++ pkg/kubectl/stop.go | 56 +-- pkg/kubectl/stop_test.go | 682 ++++++++++++++++----------- 6 files changed, 548 insertions(+), 298 deletions(-) diff --git a/pkg/client/unversioned/conditions.go b/pkg/client/unversioned/conditions.go index d1d370f49a..9b3a003c2b 100644 --- a/pkg/client/unversioned/conditions.go +++ b/pkg/client/unversioned/conditions.go @@ -54,7 +54,7 @@ func JobHasDesiredParallelism(c ExtensionsInterface, job *extensions.Job) wait.C } // desired parallelism can be either the exact number, in which case return immediately - if job.Status.Active == *job.Spec.Parallelism { + if job.Spec.Parallelism != nil && job.Status.Active == *job.Spec.Parallelism { return true, nil } // otherwise count successful diff --git a/pkg/kubectl/cmd/scale.go b/pkg/kubectl/cmd/scale.go index 18e9fedbff..8cc4c21c8b 100644 --- a/pkg/kubectl/cmd/scale.go +++ b/pkg/kubectl/cmd/scale.go @@ -146,6 +146,10 @@ func RunScale(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []stri errs := []error{} for _, info := range infos { if err := scaler.Scale(info.Namespace, info.Name, uint(count), precondition, retry, waitForReplicas); err != nil { + if scaleErr, ok := err.(kubectl.ScaleError); ok && scaleErr.FailureType == kubectl.AlreadyScaled { + cmdutil.PrintSuccess(mapper, shortOutput, out, info.Mapping.Resource, info.Name, "already scaled") + continue + } errs = append(errs, err) continue } diff --git a/pkg/kubectl/scale.go b/pkg/kubectl/scale.go index 80f07df920..4603e7ac55 100644 --- a/pkg/kubectl/scale.go +++ b/pkg/kubectl/scale.go @@ -17,6 +17,7 @@ limitations under the License. package kubectl import ( + goerrors "errors" "fmt" "strconv" "time" @@ -79,8 +80,13 @@ const ( ScaleGetFailure ScaleErrorType = iota ScaleUpdateFailure ScaleUpdateInvalidFailure + // AlreadyScaled is not really an error but we need a way to surface to the client that + // the scaling didn't happen because we already have the desired state the user asked for. + AlreadyScaled ) +var alreadyScaledErr = goerrors.New("desired replicas already equals the requested replicas") + // A ScaleError is returned when a scale request passes // preconditions but fails to actually scale the controller. type ScaleError struct { @@ -112,12 +118,14 @@ func ScaleCondition(r Scaler, precondition *ScalePrecondition, namespace, name s case nil: return true, nil case ScaleError: - // if it's invalid we shouldn't keep waiting - if e.FailureType == ScaleUpdateInvalidFailure { + switch e.FailureType { + case ScaleUpdateInvalidFailure: + // if it's invalid we shouldn't keep waiting return false, err - } - if e.FailureType == ScaleUpdateFailure { + case ScaleUpdateFailure: return false, nil + case AlreadyScaled: + return false, err } } return false, err @@ -149,8 +157,10 @@ func (scaler *ReplicationControllerScaler) ScaleSimple(namespace, name string, p return err } } + if controller.Spec.Replicas == int(newSize) { + return ScaleError{AlreadyScaled, controller.ResourceVersion, alreadyScaledErr} + } controller.Spec.Replicas = int(newSize) - // TODO: do retry on 409 errors here? if _, err := scaler.c.ReplicationControllers(namespace).Update(controller); err != nil { if errors.IsInvalid(err) { return ScaleError{ScaleUpdateInvalidFailure, controller.ResourceVersion, err} @@ -216,6 +226,9 @@ func (scaler *JobScaler) ScaleSimple(namespace, name string, preconditions *Scal } } parallelism := int(newSize) + if job.Spec.Parallelism != nil && *job.Spec.Parallelism == parallelism { + return ScaleError{AlreadyScaled, job.ResourceVersion, alreadyScaledErr} + } job.Spec.Parallelism = ¶llelism if _, err := scaler.c.Jobs(namespace).Update(job); err != nil { if errors.IsInvalid(err) { @@ -279,6 +292,9 @@ func (scaler *DeploymentScaler) ScaleSimple(namespace, name string, precondition } } scale := extensions.ScaleFromDeployment(deployment) + if scale.Spec.Replicas == int(newSize) { + return ScaleError{AlreadyScaled, deployment.ResourceVersion, alreadyScaledErr} + } scale.Spec.Replicas = int(newSize) if _, err := scaler.c.Scales(namespace).Update("Deployment", scale); err != nil { if errors.IsInvalid(err) { diff --git a/pkg/kubectl/scale_test.go b/pkg/kubectl/scale_test.go index 02d222ecfe..2814c699fd 100644 --- a/pkg/kubectl/scale_test.go +++ b/pkg/kubectl/scale_test.go @@ -132,6 +132,31 @@ func TestReplicationControllerScaleFailsPreconditions(t *testing.T) { } } +func TestReplicationControllerAlreadyScaled(t *testing.T) { + fake := testclient.NewSimpleFake(&api.ReplicationController{ + Spec: api.ReplicationControllerSpec{ + Replicas: 3, + }, + }) + scaler := ReplicationControllerScaler{fake} + count := uint(3) + name := "foo" + err := scaler.Scale("default", name, count, nil, nil, nil) + if err == nil { + t.Fatal("expected AlreadyScaled error, got nil") + } + if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { + t.Fatalf("expected AlreadyScaled error, got %s", scaleErr.FailureType) + } + actions := fake.Actions() + if len(actions) != 1 { + t.Fatalf("unexpected actions: %v, expected 1 action (get)", actions) + } + if action, ok := actions[0].(testclient.GetAction); !ok || action.GetResource() != "replicationcontrollers" || action.GetName() != name { + t.Errorf("unexpected action: %v, expected get-replicationController %s", actions[0], name) + } +} + func TestValidateReplicationController(t *testing.T) { tests := []struct { preconditions ScalePrecondition @@ -362,6 +387,32 @@ func TestJobScaleFailsPreconditions(t *testing.T) { } } +func TestJobAlreadyScaled(t *testing.T) { + three := 3 + fake := testclient.NewSimpleFake(&extensions.Job{ + Spec: extensions.JobSpec{ + Parallelism: &three, + }, + }) + scaler := JobScaler{&testclient.FakeExperimental{fake}} + count := uint(3) + name := "foo" + err := scaler.Scale("default", name, count, nil, nil, nil) + if err == nil { + t.Fatal("expected AlreadyScaled error, got nil") + } + if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { + t.Fatalf("expected AlreadyScaled error, got %s", scaleErr.FailureType) + } + actions := fake.Actions() + if len(actions) != 1 { + t.Fatalf("unexpected actions: %v, expected 1 action (get)", actions) + } + if action, ok := actions[0].(testclient.GetAction); !ok || action.GetResource() != "jobs" || action.GetName() != name { + t.Errorf("unexpected action: %v, expected get-job %s", actions[0], name) + } +} + func TestValidateJob(t *testing.T) { zero, ten, twenty := 0, 10, 20 tests := []struct { @@ -626,6 +677,31 @@ func TestDeploymentScaleFailsPreconditions(t *testing.T) { } } +func TestDeploymentAlreadyScaled(t *testing.T) { + fake := testclient.NewSimpleFake(&extensions.Deployment{ + Spec: extensions.DeploymentSpec{ + Replicas: 3, + }, + }) + scaler := DeploymentScaler{&testclient.FakeExperimental{fake}} + count := uint(3) + name := "foo" + err := scaler.Scale("default", name, count, nil, nil, nil) + if err == nil { + t.Fatal("expected AlreadyScaled error, got nil") + } + if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { + t.Fatalf("expected AlreadyScaled error, got %s", scaleErr.FailureType) + } + actions := fake.Actions() + if len(actions) != 1 { + t.Fatalf("unexpected actions: %v, expected 1 action (get)", actions) + } + if action, ok := actions[0].(testclient.GetAction); !ok || action.GetResource() != "deployments" || action.GetName() != name { + t.Errorf("unexpected action: %v, expected get-deployment %s", actions[0], name) + } +} + func TestValidateDeployment(t *testing.T) { zero, ten, twenty := 0, 10, 20 tests := []struct { diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 11409dfc11..36c2190361 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -23,7 +23,6 @@ import ( "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/api/errors" - "k8s.io/kubernetes/pkg/api/meta" "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/apis/extensions" client "k8s.io/kubernetes/pkg/client/unversioned" @@ -83,30 +82,6 @@ func ReaperForReplicationController(c client.Interface, timeout time.Duration) ( return &ReplicationControllerReaper{c, Interval, timeout}, nil } -type ReplicationControllerReaper struct { - client.Interface - pollInterval, timeout time.Duration -} -type DaemonSetReaper struct { - client.Interface - pollInterval, timeout time.Duration -} -type JobReaper struct { - client.Interface - pollInterval, timeout time.Duration -} -type PodReaper struct { - client.Interface -} -type ServiceReaper struct { - client.Interface -} - -type objInterface interface { - Delete(name string) error - Get(name string) (meta.Object, error) -} - // getOverlappingControllers finds rcs that this controller overlaps, as well as rcs overlapping this controller. func getOverlappingControllers(c client.ReplicationControllerInterface, rc *api.ReplicationController) ([]api.ReplicationController, error) { rcs, err := c.List(api.ListOptions{}) @@ -124,6 +99,11 @@ func getOverlappingControllers(c client.ReplicationControllerInterface, rc *api. return matchingRCs, nil } +type ReplicationControllerReaper struct { + client.Interface + pollInterval, timeout time.Duration +} + func (reaper *ReplicationControllerReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *api.DeleteOptions) error { rc := reaper.ReplicationControllers(namespace) scaler, err := ScalerFor(api.Kind("ReplicationController"), *reaper) @@ -181,7 +161,9 @@ func (reaper *ReplicationControllerReaper) Stop(namespace, name string, timeout retry := NewRetryParams(reaper.pollInterval, reaper.timeout) waitForReplicas := NewRetryParams(reaper.pollInterval, timeout) if err = scaler.Scale(namespace, name, 0, nil, retry, waitForReplicas); err != nil { - return err + if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { + return err + } } } if err := rc.Delete(name); err != nil { @@ -190,6 +172,11 @@ func (reaper *ReplicationControllerReaper) Stop(namespace, name string, timeout return nil } +type DaemonSetReaper struct { + client.Interface + pollInterval, timeout time.Duration +} + func (reaper *DaemonSetReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *api.DeleteOptions) error { ds, err := reaper.Extensions().DaemonSets(namespace).Get(name) if err != nil { @@ -227,6 +214,11 @@ func (reaper *DaemonSetReaper) Stop(namespace, name string, timeout time.Duratio return nil } +type JobReaper struct { + client.Interface + pollInterval, timeout time.Duration +} + func (reaper *JobReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *api.DeleteOptions) error { jobs := reaper.Extensions().Jobs(namespace) pods := reaper.Pods(namespace) @@ -248,7 +240,9 @@ func (reaper *JobReaper) Stop(namespace, name string, timeout time.Duration, gra retry := NewRetryParams(reaper.pollInterval, reaper.timeout) waitForJobs := NewRetryParams(reaper.pollInterval, timeout) if err = scaler.Scale(namespace, name, 0, nil, retry, waitForJobs); err != nil { - return err + if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { + return err + } } // at this point only dead pods are left, that should be removed selector, _ := extensions.LabelSelectorAsSelector(job.Spec.Selector) @@ -276,6 +270,10 @@ func (reaper *JobReaper) Stop(namespace, name string, timeout time.Duration, gra return nil } +type PodReaper struct { + client.Interface +} + func (reaper *PodReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *api.DeleteOptions) error { pods := reaper.Pods(namespace) _, err := pods.Get(name) @@ -289,6 +287,10 @@ func (reaper *PodReaper) Stop(namespace, name string, timeout time.Duration, gra return nil } +type ServiceReaper struct { + client.Interface +} + func (reaper *ServiceReaper) Stop(namespace, name string, timeout time.Duration, gracePeriod *api.DeleteOptions) error { services := reaper.Services(namespace) _, err := services.Get(name) diff --git a/pkg/kubectl/stop_test.go b/pkg/kubectl/stop_test.go index 2e2b5d343e..a2dac554a4 100644 --- a/pkg/kubectl/stop_test.go +++ b/pkg/kubectl/stop_test.go @@ -19,7 +19,6 @@ package kubectl import ( "fmt" "reflect" - "strings" "testing" "time" @@ -31,355 +30,508 @@ import ( "k8s.io/kubernetes/pkg/runtime" ) +const ( + name = "foo" +) + +func overlappingButSafe() *api.ReplicationControllerList { + return &api.ReplicationControllerList{ + Items: []api.ReplicationController{ + { + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 1, + Selector: map[string]string{"k1": "v1", "k2": "v2"}}, + }, + { + ObjectMeta: api.ObjectMeta{ + Name: "baz", + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 2, + Selector: map[string]string{"k1": "v1", "k2": "v2", "k3": "v3"}}, + }, + { + ObjectMeta: api.ObjectMeta{ + Name: "zaz", + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 3, + Selector: map[string]string{"k1": "v1"}}, + }, + }, + } +} + +func exactMatches() *api.ReplicationControllerList { + return &api.ReplicationControllerList{ + Items: []api.ReplicationController{ + { + ObjectMeta: api.ObjectMeta{ + Name: "zaz", + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 3, + Selector: map[string]string{"k1": "v1"}}, + }, + { + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 3, + Selector: map[string]string{"k1": "v1"}}, + }, + }, + } +} + func TestReplicationControllerStop(t *testing.T) { - name := "foo" - ns := "default" + // test data + toBeReaped := &api.ReplicationController{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 5, + Selector: map[string]string{"k1": "v1"}}, + } + reaped := &api.ReplicationController{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 0, + Selector: map[string]string{"k1": "v1"}}, + } + noOverlapping := &api.ReplicationController{ + ObjectMeta: api.ObjectMeta{ + Name: "baz", + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 3, + Selector: map[string]string{"k3": "v3"}}, + } + overlapping := &api.ReplicationController{ + ObjectMeta: api.ObjectMeta{ + Name: "baz", + Namespace: api.NamespaceDefault, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 2, + Selector: map[string]string{"k1": "v1", "k2": "v2"}}, + } + + // tests tests := []struct { Name string - Objs []runtime.Object + Fns []testclient.ReactionFunc StopError error - ExpectedActions []string + ExpectedActions []testclient.Action }{ { Name: "OnlyOneRC", - Objs: []runtime.Object{ - &api.ReplicationController{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, + Fns: []testclient.ReactionFunc{ + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, - &api.ReplicationControllerList{ // LIST - Items: []api.ReplicationController{ - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, - }, - }, + // LIST rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.ReplicationControllerList{ + Items: []api.ReplicationController{*toBeReaped}}, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // UPDATE rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // DELETE rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil }, }, - StopError: nil, - ExpectedActions: []string{"get", "list", "get", "update", "get", "get", "delete"}, + StopError: nil, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewUpdateAction("replicationcontrollers", api.NamespaceDefault, reaped), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewDeleteAction("replicationcontrollers", api.NamespaceDefault, name), + }, + }, + { + Name: "RCWithNoPods", + Fns: []testclient.ReactionFunc{ + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // LIST rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.ReplicationControllerList{ + Items: []api.ReplicationController{*reaped}}, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // DELETE rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + }, + StopError: nil, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewDeleteAction("replicationcontrollers", api.NamespaceDefault, name), + }, }, { Name: "NoOverlapping", - Objs: []runtime.Object{ - &api.ReplicationController{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, + Fns: []testclient.ReactionFunc{ + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, - &api.ReplicationControllerList{ // LIST - Items: []api.ReplicationController{ - { - ObjectMeta: api.ObjectMeta{ - Name: "baz", - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k3": "v3"}}, - }, - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, - }, - }, + // LIST rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.ReplicationControllerList{ + Items: []api.ReplicationController{*toBeReaped, *noOverlapping}}, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // UPDATE rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // DELETE rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil }, }, - StopError: nil, - ExpectedActions: []string{"get", "list", "get", "update", "get", "get", "delete"}, + StopError: nil, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewUpdateAction("replicationcontrollers", api.NamespaceDefault, reaped), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewDeleteAction("replicationcontrollers", api.NamespaceDefault, name), + }, }, { Name: "OverlappingError", - Objs: []runtime.Object{ - - &api.ReplicationController{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, + Fns: []testclient.ReactionFunc{ + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, - &api.ReplicationControllerList{ // LIST - Items: []api.ReplicationController{ - { - ObjectMeta: api.ObjectMeta{ - Name: "baz", - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1", "k2": "v2"}}, - }, - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, - }, - }, + // LIST rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.ReplicationControllerList{ + Items: []api.ReplicationController{*toBeReaped, *overlapping}}, nil }, }, - StopError: fmt.Errorf("Detected overlapping controllers for rc foo: baz, please manage deletion individually with --cascade=false."), - ExpectedActions: []string{"get", "list"}, + StopError: fmt.Errorf("Detected overlapping controllers for rc foo: baz, please manage deletion individually with --cascade=false."), + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), + }, }, - { Name: "OverlappingButSafeDelete", - Objs: []runtime.Object{ - - &api.ReplicationController{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1", "k2": "v2"}}, + Fns: []testclient.ReactionFunc{ + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &overlappingButSafe().Items[0], nil }, - &api.ReplicationControllerList{ // LIST - Items: []api.ReplicationController{ - { - ObjectMeta: api.ObjectMeta{ - Name: "baz", - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1", "k2": "v2", "k3": "v3"}}, - }, - { - ObjectMeta: api.ObjectMeta{ - Name: "zaz", - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, - }, - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1", "k2": "v2"}}, - }, - }, + // LIST rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, overlappingButSafe(), nil }, }, - - StopError: fmt.Errorf("Detected overlapping controllers for rc foo: baz,zaz, please manage deletion individually with --cascade=false."), - ExpectedActions: []string{"get", "list"}, + StopError: fmt.Errorf("Detected overlapping controllers for rc foo: baz,zaz, please manage deletion individually with --cascade=false."), + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), + }, }, - { Name: "TwoExactMatchRCs", - Objs: []runtime.Object{ - - &api.ReplicationController{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, + Fns: []testclient.ReactionFunc{ + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, - &api.ReplicationControllerList{ // LIST - Items: []api.ReplicationController{ - { - ObjectMeta: api.ObjectMeta{ - Name: "zaz", - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, - }, - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: api.ReplicationControllerSpec{ - Replicas: 0, - Selector: map[string]string{"k1": "v1"}}, - }, - }, + // LIST rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, exactMatches(), nil + }, + // GET rc + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil }, }, - - StopError: nil, - ExpectedActions: []string{"get", "list", "delete"}, + StopError: nil, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), + testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), + testclient.NewDeleteAction("replicationcontrollers", api.NamespaceDefault, name), + }, }, } for _, test := range tests { - fake := testclient.NewSimpleFake(test.Objs...) + toBeReaped.Spec.Replicas = 5 + fake := &testclient.Fake{} + for i, reaction := range test.Fns { + fake.AddReactor(test.ExpectedActions[i].GetVerb(), test.ExpectedActions[i].GetResource(), reaction) + } reaper := ReplicationControllerReaper{fake, time.Millisecond, time.Millisecond} - err := reaper.Stop(ns, name, 0, nil) + err := reaper.Stop(api.NamespaceDefault, name, 0, nil) if !reflect.DeepEqual(err, test.StopError) { - t.Errorf("%s unexpected error: %v", test.Name, err) + t.Errorf("%s: unexpected error: %v", test.Name, err) continue } actions := fake.Actions() - if len(actions) != len(test.ExpectedActions) { - t.Errorf("%s unexpected actions: %v, expected %d actions got %d", test.Name, actions, len(test.ExpectedActions), len(actions)) - continue + if len(test.ExpectedActions) != len(actions) { + t.Errorf("%s: unexpected actions:\n%v\nexpected\n%v\n", test.Name, actions, test.ExpectedActions) } - for i, verb := range test.ExpectedActions { - if actions[i].GetResource() != "replicationcontrollers" { - t.Errorf("%s unexpected action: %+v, expected %s-replicationController", test.Name, actions[i], verb) - } - if actions[i].GetVerb() != verb { - t.Errorf("%s unexpected action: %+v, expected %s-replicationController", test.Name, actions[i], verb) + for i, action := range actions { + testAction := test.ExpectedActions[i] + if !testAction.Matches(action.GetVerb(), action.GetResource()) { + t.Errorf("%s: unexpected action: %#v; expected %v", test.Name, action, testAction) } } } } func TestJobStop(t *testing.T) { - name := "foo" - ns := "default" zero := 0 + one := 1 + toBeReaped := &extensions.Job{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: api.NamespaceDefault, + }, + Spec: extensions.JobSpec{ + Parallelism: &one, + Selector: &extensions.LabelSelector{ + MatchLabels: map[string]string{"k1": "v1"}, + }, + }, + } + reaped := &extensions.Job{ + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: api.NamespaceDefault, + }, + Spec: extensions.JobSpec{ + Parallelism: &zero, + Selector: &extensions.LabelSelector{ + MatchLabels: map[string]string{"k1": "v1"}, + }, + }, + } + tests := []struct { Name string - Objs []runtime.Object + Fns []testclient.ReactionFunc StopError error - ExpectedActions []string + ExpectedActions []testclient.Action }{ { Name: "OnlyOneJob", - Objs: []runtime.Object{ - &extensions.Job{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: extensions.JobSpec{ - Parallelism: &zero, - Selector: &extensions.LabelSelector{ - MatchLabels: map[string]string{"k1": "v1"}, - }, - }, + Fns: []testclient.ReactionFunc{ + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, - &extensions.JobList{ // LIST - Items: []extensions.Job{ - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: extensions.JobSpec{ - Parallelism: &zero, - Selector: &extensions.LabelSelector{ - MatchLabels: map[string]string{"k1": "v1"}, - }, - }, - }, - }, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // UPDATE job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + *toBeReaped.Spec.Parallelism = 0 + return true, toBeReaped, nil + }, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // LIST pods + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.PodList{}, nil + }, + // DELETE job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, }, StopError: nil, - ExpectedActions: []string{"get:jobs", "get:jobs", "update:jobs", - "get:jobs", "get:jobs", "list:pods", "delete:jobs"}, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewUpdateAction("jobs", api.NamespaceDefault, toBeReaped), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewListAction("pods", api.NamespaceDefault, api.ListOptions{}), + testclient.NewDeleteAction("jobs", api.NamespaceDefault, name), + }, }, { Name: "JobWithDeadPods", - Objs: []runtime.Object{ - &extensions.Job{ // GET - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: extensions.JobSpec{ - Parallelism: &zero, - Selector: &extensions.LabelSelector{ - MatchLabels: map[string]string{"k1": "v1"}, - }, - }, + Fns: []testclient.ReactionFunc{ + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, - &extensions.JobList{ // LIST - Items: []extensions.Job{ - { - ObjectMeta: api.ObjectMeta{ - Name: name, - Namespace: ns, - }, - Spec: extensions.JobSpec{ - Parallelism: &zero, - Selector: &extensions.LabelSelector{ - MatchLabels: map[string]string{"k1": "v1"}, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // UPDATE job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + *toBeReaped.Spec.Parallelism = 0 + return true, toBeReaped, nil + }, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil + }, + // LIST pods + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.PodList{ + Items: []api.Pod{ + { + ObjectMeta: api.ObjectMeta{ + Name: "pod1", + Namespace: api.NamespaceDefault, + Labels: map[string]string{"k1": "v1"}, }, }, }, - }, + }, nil }, - &api.PodList{ // LIST - Items: []api.Pod{ - { - ObjectMeta: api.ObjectMeta{ - Name: "pod1", - Namespace: ns, - Labels: map[string]string{"k1": "v1"}, - }, - }, - }, + // DELETE pod + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, nil + }, + // DELETE job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, toBeReaped, nil }, }, StopError: nil, - ExpectedActions: []string{"get:jobs", "get:jobs", "update:jobs", - "get:jobs", "get:jobs", "list:pods", "delete:pods", "delete:jobs"}, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewUpdateAction("jobs", api.NamespaceDefault, toBeReaped), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewListAction("pods", api.NamespaceDefault, api.ListOptions{}), + testclient.NewDeleteAction("pods", api.NamespaceDefault, name), + testclient.NewDeleteAction("jobs", api.NamespaceDefault, name), + }, + }, + { + Name: "JobWithNoParallelism", + Fns: []testclient.ReactionFunc{ + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // GET job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + // LIST pods + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, &api.PodList{}, nil + }, + // DELETE job + func(action testclient.Action) (handled bool, ret runtime.Object, err error) { + return true, reaped, nil + }, + }, + StopError: nil, + ExpectedActions: []testclient.Action{ + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewGetAction("jobs", api.NamespaceDefault, name), + testclient.NewListAction("pods", api.NamespaceDefault, api.ListOptions{}), + testclient.NewDeleteAction("jobs", api.NamespaceDefault, name), + }, }, } for _, test := range tests { - fake := testclient.NewSimpleFake(test.Objs...) + *toBeReaped.Spec.Parallelism = one + fake := &testclient.Fake{} + for i, reaction := range test.Fns { + fake.AddReactor(test.ExpectedActions[i].GetVerb(), test.ExpectedActions[i].GetResource(), reaction) + } reaper := JobReaper{fake, time.Millisecond, time.Millisecond} - err := reaper.Stop(ns, name, 0, nil) + err := reaper.Stop(api.NamespaceDefault, name, 0, nil) if !reflect.DeepEqual(err, test.StopError) { t.Errorf("%s unexpected error: %v", test.Name, err) continue } actions := fake.Actions() - if len(actions) != len(test.ExpectedActions) { - t.Errorf("%s unexpected actions: %v, expected %d actions got %d", test.Name, actions, len(test.ExpectedActions), len(actions)) - continue + if len(test.ExpectedActions) != len(actions) { + t.Errorf("%s: unexpected actions:\n%v\nexpected\n%v\n", test.Name, actions, test.ExpectedActions) } - for i, expAction := range test.ExpectedActions { - action := strings.Split(expAction, ":") - if actions[i].GetVerb() != action[0] { - t.Errorf("%s unexpected verb: %+v, expected %s", test.Name, actions[i], expAction) - } - if actions[i].GetResource() != action[1] { - t.Errorf("%s unexpected resource: %+v, expected %s", test.Name, actions[i], expAction) + for i, action := range actions { + testAction := test.ExpectedActions[i] + if !testAction.Matches(action.GetVerb(), action.GetResource()) { + t.Errorf("%s: unexpected action: %#v; expected %v", test.Name, action, testAction) } } }