From 62726c4ab8cdecd51c3d8575d8bf6325aec38b24 Mon Sep 17 00:00:00 2001 From: Zach Loafman Date: Fri, 22 Jan 2016 14:52:38 -0800 Subject: [PATCH] Revert "kubectl: Make scaling smarter" --- 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 | 756 +++++++++++---------------- 6 files changed, 335 insertions(+), 585 deletions(-) diff --git a/pkg/client/unversioned/conditions.go b/pkg/client/unversioned/conditions.go index e2a27b6b1b1..4556a643745 100644 --- a/pkg/client/unversioned/conditions.go +++ b/pkg/client/unversioned/conditions.go @@ -112,7 +112,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.Spec.Parallelism != nil && job.Status.Active == *job.Spec.Parallelism { + if 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 8cc4c21c8b6..18e9fedbffd 100644 --- a/pkg/kubectl/cmd/scale.go +++ b/pkg/kubectl/cmd/scale.go @@ -146,10 +146,6 @@ 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 4603e7ac55d..80f07df9201 100644 --- a/pkg/kubectl/scale.go +++ b/pkg/kubectl/scale.go @@ -17,7 +17,6 @@ limitations under the License. package kubectl import ( - goerrors "errors" "fmt" "strconv" "time" @@ -80,13 +79,8 @@ 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 { @@ -118,14 +112,12 @@ func ScaleCondition(r Scaler, precondition *ScalePrecondition, namespace, name s case nil: return true, nil case ScaleError: - switch e.FailureType { - case ScaleUpdateInvalidFailure: - // if it's invalid we shouldn't keep waiting + // if it's invalid we shouldn't keep waiting + if e.FailureType == ScaleUpdateInvalidFailure { return false, err - case ScaleUpdateFailure: + } + if e.FailureType == ScaleUpdateFailure { return false, nil - case AlreadyScaled: - return false, err } } return false, err @@ -157,10 +149,8 @@ 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} @@ -226,9 +216,6 @@ 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) { @@ -292,9 +279,6 @@ 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 2814c699fd2..02d222ecfe5 100644 --- a/pkg/kubectl/scale_test.go +++ b/pkg/kubectl/scale_test.go @@ -132,31 +132,6 @@ 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 @@ -387,32 +362,6 @@ 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 { @@ -677,31 +626,6 @@ 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 36c21903617..11409dfc11a 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -23,6 +23,7 @@ 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" @@ -82,6 +83,30 @@ 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{}) @@ -99,11 +124,6 @@ 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) @@ -161,9 +181,7 @@ 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 { - if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { - return err - } + return err } } if err := rc.Delete(name); err != nil { @@ -172,11 +190,6 @@ 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 { @@ -214,11 +227,6 @@ 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) @@ -240,9 +248,7 @@ 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 { - if scaleErr, ok := err.(ScaleError); !ok || scaleErr.FailureType != AlreadyScaled { - return err - } + return err } // at this point only dead pods are left, that should be removed selector, _ := extensions.LabelSelectorAsSelector(job.Spec.Selector) @@ -270,10 +276,6 @@ 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) @@ -287,10 +289,6 @@ 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 a2dac554a46..2e2b5d343ea 100644 --- a/pkg/kubectl/stop_test.go +++ b/pkg/kubectl/stop_test.go @@ -19,6 +19,7 @@ package kubectl import ( "fmt" "reflect" + "strings" "testing" "time" @@ -30,508 +31,355 @@ 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) { - // 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 + name := "foo" + ns := "default" tests := []struct { Name string - Fns []testclient.ReactionFunc + Objs []runtime.Object StopError error - ExpectedActions []testclient.Action + ExpectedActions []string }{ { Name: "OnlyOneRC", - Fns: []testclient.ReactionFunc{ - // GET rc - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, toBeReaped, nil + Objs: []runtime.Object{ + &api.ReplicationController{ // GET + 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 + &api.ReplicationControllerList{ // LIST + Items: []api.ReplicationController{ + { + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: ns, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 0, + Selector: map[string]string{"k1": "v1"}}, + }, + }, }, }, - 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), - }, + StopError: nil, + ExpectedActions: []string{"get", "list", "get", "update", "get", "get", "delete"}, }, { Name: "NoOverlapping", - Fns: []testclient.ReactionFunc{ - // GET rc - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, toBeReaped, nil + Objs: []runtime.Object{ + &api.ReplicationController{ // GET + 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 + &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"}}, + }, + }, }, }, - 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), - }, + StopError: nil, + ExpectedActions: []string{"get", "list", "get", "update", "get", "get", "delete"}, }, { Name: "OverlappingError", - Fns: []testclient.ReactionFunc{ - // GET rc - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, toBeReaped, nil + Objs: []runtime.Object{ + + &api.ReplicationController{ // GET + 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 + &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"}}, + }, + }, }, }, - 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{}), - }, + StopError: fmt.Errorf("Detected overlapping controllers for rc foo: baz, please manage deletion individually with --cascade=false."), + ExpectedActions: []string{"get", "list"}, }, + { Name: "OverlappingButSafeDelete", - Fns: []testclient.ReactionFunc{ - // GET rc - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, &overlappingButSafe().Items[0], nil + 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"}}, }, - // LIST rc - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, overlappingButSafe(), 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"}}, + }, + }, }, }, - 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{}), - }, + + StopError: fmt.Errorf("Detected overlapping controllers for rc foo: baz,zaz, please manage deletion individually with --cascade=false."), + ExpectedActions: []string{"get", "list"}, }, + { Name: "TwoExactMatchRCs", - Fns: []testclient.ReactionFunc{ - // GET rc - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, toBeReaped, nil - }, - // 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: []testclient.Action{ - testclient.NewGetAction("replicationcontrollers", api.NamespaceDefault, name), - testclient.NewListAction("replicationcontrollers", api.NamespaceDefault, api.ListOptions{}), - testclient.NewDeleteAction("replicationcontrollers", api.NamespaceDefault, name), - }, - }, - } + Objs: []runtime.Object{ - for _, test := range tests { - 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(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(test.ExpectedActions) != len(actions) { - t.Errorf("%s: unexpected actions:\n%v\nexpected\n%v\n", test.Name, actions, test.ExpectedActions) - } - 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) { - 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 - Fns []testclient.ReactionFunc - StopError error - ExpectedActions []testclient.Action - }{ - { - Name: "OnlyOneJob", - Fns: []testclient.ReactionFunc{ - // GET job - func(action testclient.Action) (handled bool, ret runtime.Object, err error) { - return true, toBeReaped, nil + &api.ReplicationController{ // GET + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: ns, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 0, + Selector: 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: []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", - Fns: []testclient.ReactionFunc{ - // 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 - }, - // 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"}, - }, + &api.ReplicationControllerList{ // LIST + Items: []api.ReplicationController{ + { + ObjectMeta: api.ObjectMeta{ + Name: "zaz", + Namespace: ns, }, + Spec: api.ReplicationControllerSpec{ + Replicas: 0, + Selector: map[string]string{"k1": "v1"}}, }, - }, nil - }, - // 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 + { + ObjectMeta: api.ObjectMeta{ + Name: name, + Namespace: ns, + }, + Spec: api.ReplicationControllerSpec{ + Replicas: 0, + Selector: map[string]string{"k1": "v1"}}, + }, + }, }, }, - StopError: nil, - 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), - }, + + StopError: nil, + ExpectedActions: []string{"get", "list", "delete"}, }, } for _, test := range tests { - *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(api.NamespaceDefault, name, 0, nil) + fake := testclient.NewSimpleFake(test.Objs...) + reaper := ReplicationControllerReaper{fake, time.Millisecond, time.Millisecond} + err := reaper.Stop(ns, name, 0, nil) if !reflect.DeepEqual(err, test.StopError) { t.Errorf("%s unexpected error: %v", test.Name, err) continue } actions := fake.Actions() - if len(test.ExpectedActions) != len(actions) { - t.Errorf("%s: unexpected actions:\n%v\nexpected\n%v\n", test.Name, actions, test.ExpectedActions) + 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 } - 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) + 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) + } + } + } +} + +func TestJobStop(t *testing.T) { + name := "foo" + ns := "default" + zero := 0 + tests := []struct { + Name string + Objs []runtime.Object + StopError error + ExpectedActions []string + }{ + { + 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"}, + }, + }, + }, + &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"}, + }, + }, + }, + }, + }, + }, + StopError: nil, + ExpectedActions: []string{"get:jobs", "get:jobs", "update:jobs", + "get:jobs", "get:jobs", "list:pods", "delete:jobs"}, + }, + { + 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"}, + }, + }, + }, + &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"}, + }, + }, + }, + }, + }, + &api.PodList{ // LIST + Items: []api.Pod{ + { + ObjectMeta: api.ObjectMeta{ + Name: "pod1", + Namespace: ns, + Labels: map[string]string{"k1": "v1"}, + }, + }, + }, + }, + }, + StopError: nil, + ExpectedActions: []string{"get:jobs", "get:jobs", "update:jobs", + "get:jobs", "get:jobs", "list:pods", "delete:pods", "delete:jobs"}, + }, + } + + for _, test := range tests { + fake := testclient.NewSimpleFake(test.Objs...) + reaper := JobReaper{fake, time.Millisecond, time.Millisecond} + err := reaper.Stop(ns, 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 + } + 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) } } }