From 312ccad3c1ac2e660ce8d267a513ae381eda9b98 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Wed, 15 Apr 2015 14:28:59 -0400 Subject: [PATCH 1/2] Use narrowly scoped interfaces for client access Use custom narrowly scoped interfaces for client access from the RollingUpdater and Resizer. This allows for more flexible downstream integration and unit testing without imposing a burden to implement the entire client.Interface for just a handful of methods. --- pkg/kubectl/cmd/rollingupdate.go | 2 +- pkg/kubectl/cmd/util/factory.go | 2 +- pkg/kubectl/resize.go | 35 ++++++++++++++---- pkg/kubectl/resize_test.go | 6 ++-- pkg/kubectl/rolling_updater.go | 55 +++++++++++++++++++++++------ pkg/kubectl/rolling_updater_test.go | 4 +-- pkg/kubectl/stop.go | 2 +- 7 files changed, 81 insertions(+), 25 deletions(-) diff --git a/pkg/kubectl/cmd/rollingupdate.go b/pkg/kubectl/cmd/rollingupdate.go index 7b81a729585..32a65c4ccbd 100644 --- a/pkg/kubectl/cmd/rollingupdate.go +++ b/pkg/kubectl/cmd/rollingupdate.go @@ -111,7 +111,7 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg return err } - updater := kubectl.NewRollingUpdater(newRc.Namespace, client) + updater := kubectl.NewRollingUpdater(newRc.Namespace, &kubectl.RealRollingUpdaterClient{client}) // fetch rc oldRc, err := client.ReplicationControllers(newRc.Namespace).Get(oldName) diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index e9c4c4a7d1f..2cd314940c0 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -197,7 +197,7 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { if err != nil { return nil, err } - return kubectl.ResizerFor(mapping.Kind, client) + return kubectl.ResizerFor(mapping.Kind, &kubectl.RealResizerClient{client}) }, Reaper: func(mapping *meta.RESTMapping) (kubectl.Reaper, error) { client, err := clients.ClientForVersion(mapping.APIVersion) diff --git a/pkg/kubectl/resize.go b/pkg/kubectl/resize.go index 229768e5c85..ca82e65d84b 100644 --- a/pkg/kubectl/resize.go +++ b/pkg/kubectl/resize.go @@ -89,7 +89,7 @@ type Resizer interface { ResizeSimple(namespace, name string, preconditions *ResizePrecondition, newSize uint) (string, error) } -func ResizerFor(kind string, c client.Interface) (Resizer, error) { +func ResizerFor(kind string, c ResizerClient) (Resizer, error) { switch kind { case "ReplicationController": return &ReplicationControllerResizer{c}, nil @@ -98,7 +98,7 @@ func ResizerFor(kind string, c client.Interface) (Resizer, error) { } type ReplicationControllerResizer struct { - client.Interface + c ResizerClient } type RetryParams struct { @@ -122,8 +122,7 @@ func ResizeCondition(r Resizer, precondition *ResizePrecondition, namespace, nam } func (resizer *ReplicationControllerResizer) ResizeSimple(namespace, name string, preconditions *ResizePrecondition, newSize uint) (string, error) { - rc := resizer.ReplicationControllers(namespace) - controller, err := rc.Get(name) + controller, err := resizer.c.GetReplicationController(namespace, name) if err != nil { return "", ControllerResizeError{ControllerResizeGetFailure, "Unknown", err} } @@ -134,7 +133,7 @@ func (resizer *ReplicationControllerResizer) ResizeSimple(namespace, name string } controller.Spec.Replicas = int(newSize) // TODO: do retry on 409 errors here? - if _, err := rc.Update(controller); err != nil { + if _, err := resizer.c.UpdateReplicationController(namespace, controller); err != nil { return "", ControllerResizeError{ControllerResizeUpdateFailure, controller.ResourceVersion, err} } // TODO: do a better job of printing objects here. @@ -159,9 +158,33 @@ func (resizer *ReplicationControllerResizer) Resize(namespace, name string, newS if waitForReplicas != nil { rc := &api.ReplicationController{ObjectMeta: api.ObjectMeta{Namespace: namespace, Name: name}} if err := wait.Poll(waitForReplicas.interval, waitForReplicas.timeout, - client.ControllerHasDesiredReplicas(resizer, rc)); err != nil { + resizer.c.ControllerHasDesiredReplicas(rc)); err != nil { return err } } return nil } + +// ResizerClient abstracts access to ReplicationControllers. +type ResizerClient interface { + GetReplicationController(namespace, name string) (*api.ReplicationController, error) + UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) + ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc +} + +// RealResizerClient is a ResizerClient which uses a Kube client. +type RealResizerClient struct { + Client client.Interface +} + +func (c *RealResizerClient) GetReplicationController(namespace, name string) (*api.ReplicationController, error) { + return c.Client.ReplicationControllers(namespace).Get(name) +} + +func (c *RealResizerClient) UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { + return c.Client.ReplicationControllers(namespace).Update(rc) +} + +func (c *RealResizerClient) ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc { + return client.ControllerHasDesiredReplicas(c.Client, rc) +} diff --git a/pkg/kubectl/resize_test.go b/pkg/kubectl/resize_test.go index 50c711168d0..5c2d482af35 100644 --- a/pkg/kubectl/resize_test.go +++ b/pkg/kubectl/resize_test.go @@ -43,7 +43,7 @@ func (c *ErrorReplicationControllerClient) ReplicationControllers(namespace stri func TestReplicationControllerResizeRetry(t *testing.T) { fake := &ErrorReplicationControllerClient{Fake: testclient.Fake{}} - resizer := ReplicationControllerResizer{fake} + resizer := ReplicationControllerResizer{&RealResizerClient{fake}} preconditions := ResizePrecondition{-1, ""} count := uint(3) name := "foo" @@ -67,7 +67,7 @@ func TestReplicationControllerResizeRetry(t *testing.T) { func TestReplicationControllerResize(t *testing.T) { fake := &testclient.Fake{} - resizer := ReplicationControllerResizer{fake} + resizer := ReplicationControllerResizer{&RealResizerClient{fake}} preconditions := ResizePrecondition{-1, ""} count := uint(3) name := "foo" @@ -90,7 +90,7 @@ func TestReplicationControllerResizeFailsPreconditions(t *testing.T) { Replicas: 10, }, }) - resizer := ReplicationControllerResizer{fake} + resizer := ReplicationControllerResizer{&RealResizerClient{fake}} preconditions := ResizePrecondition{2, ""} count := uint(3) name := "foo" diff --git a/pkg/kubectl/rolling_updater.go b/pkg/kubectl/rolling_updater.go index f350aaf22a0..0c4a901e8f7 100644 --- a/pkg/kubectl/rolling_updater.go +++ b/pkg/kubectl/rolling_updater.go @@ -31,13 +31,13 @@ import ( // fault-tolerant way. type RollingUpdater struct { // Client interface for creating and updating controllers - c client.Interface + c RollingUpdaterClient // Namespace for resources ns string } // NewRollingUpdater creates a RollingUpdater from a client -func NewRollingUpdater(namespace string, c client.Interface) *RollingUpdater { +func NewRollingUpdater(namespace string, c RollingUpdaterClient) *RollingUpdater { return &RollingUpdater{ c, namespace, @@ -95,7 +95,7 @@ func (r *RollingUpdater) Update(out io.Writer, oldRc, newRc *api.ReplicationCont newRc.ObjectMeta.Annotations[desiredReplicasAnnotation] = fmt.Sprintf("%d", desired) newRc.ObjectMeta.Annotations[sourceIdAnnotation] = sourceId newRc.Spec.Replicas = 0 - newRc, err = r.c.ReplicationControllers(r.ns).Create(newRc) + newRc, err = r.c.CreateReplicationController(r.ns, newRc) if err != nil { return err } @@ -147,7 +147,7 @@ func (r *RollingUpdater) Update(out io.Writer, oldRc, newRc *api.ReplicationCont } } // Clean up annotations - if newRc, err = r.c.ReplicationControllers(r.ns).Get(newName); err != nil { + if newRc, err = r.c.GetReplicationController(r.ns, newName); err != nil { return err } delete(newRc.ObjectMeta.Annotations, sourceIdAnnotation) @@ -158,11 +158,11 @@ func (r *RollingUpdater) Update(out io.Writer, oldRc, newRc *api.ReplicationCont } // delete old rc fmt.Fprintf(out, "Update succeeded. Deleting %s\n", oldName) - return r.c.ReplicationControllers(r.ns).Delete(oldName) + return r.c.DeleteReplicationController(r.ns, oldName) } func (r *RollingUpdater) getExistingNewRc(sourceId, name string) (rc *api.ReplicationController, existing bool, err error) { - if rc, err = r.c.ReplicationControllers(r.ns).Get(name); err == nil { + if rc, err = r.c.GetReplicationController(r.ns, name); err == nil { existing = true annotations := rc.ObjectMeta.Annotations source := annotations[sourceIdAnnotation] @@ -184,17 +184,50 @@ func (r *RollingUpdater) resizeAndWait(rc *api.ReplicationController, retry *Ret if err := resizer.Resize(rc.Namespace, rc.Name, uint(rc.Spec.Replicas), &ResizePrecondition{-1, ""}, retry, wait); err != nil { return nil, err } - return r.c.ReplicationControllers(r.ns).Get(rc.ObjectMeta.Name) + return r.c.GetReplicationController(r.ns, rc.ObjectMeta.Name) } func (r *RollingUpdater) updateAndWait(rc *api.ReplicationController, interval, timeout time.Duration) (*api.ReplicationController, error) { - rc, err := r.c.ReplicationControllers(r.ns).Update(rc) + rc, err := r.c.UpdateReplicationController(r.ns, rc) if err != nil { return nil, err } - if err = wait.Poll(interval, timeout, - client.ControllerHasDesiredReplicas(r.c, rc)); err != nil { + if err = wait.Poll(interval, timeout, r.c.ControllerHasDesiredReplicas(rc)); err != nil { return nil, err } - return r.c.ReplicationControllers(r.ns).Get(rc.ObjectMeta.Name) + return r.c.GetReplicationController(r.ns, rc.ObjectMeta.Name) +} + +// RollingUpdaterClient abstracts access to ReplicationControllers. +type RollingUpdaterClient interface { + GetReplicationController(namespace, name string) (*api.ReplicationController, error) + UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) + CreateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) + DeleteReplicationController(namespace, name string) error + ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc +} + +// RealRollingUpdaterClient is a RollingUpdaterClient which uses a Kube client. +type RealRollingUpdaterClient struct { + Client client.Interface +} + +func (c *RealRollingUpdaterClient) GetReplicationController(namespace, name string) (*api.ReplicationController, error) { + return c.Client.ReplicationControllers(namespace).Get(name) +} + +func (c *RealRollingUpdaterClient) UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { + return c.Client.ReplicationControllers(namespace).Update(rc) +} + +func (c *RealRollingUpdaterClient) CreateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { + return c.Client.ReplicationControllers(namespace).Create(rc) +} + +func (c *RealRollingUpdaterClient) DeleteReplicationController(namespace, name string) error { + return c.Client.ReplicationControllers(namespace).Delete(name) +} + +func (c *RealRollingUpdaterClient) ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc { + return client.ControllerHasDesiredReplicas(c.Client, rc) } diff --git a/pkg/kubectl/rolling_updater_test.go b/pkg/kubectl/rolling_updater_test.go index a13297e13ae..2c05b79002c 100644 --- a/pkg/kubectl/rolling_updater_test.go +++ b/pkg/kubectl/rolling_updater_test.go @@ -253,7 +253,7 @@ Update succeeded. Deleting foo-v1 for _, test := range tests { updater := RollingUpdater{ - fakeClientFor("default", test.responses), + &RealRollingUpdaterClient{fakeClientFor("default", test.responses)}, "default", } var buffer bytes.Buffer @@ -296,7 +296,7 @@ Update succeeded. Deleting foo-v1 {newRc(3, 3), nil}, {newRc(3, 3), nil}, } - updater := RollingUpdater{fakeClientFor("default", responses), "default"} + updater := RollingUpdater{&RealRollingUpdaterClient{fakeClientFor("default", responses)}, "default"} var buffer bytes.Buffer if err := updater.Update(&buffer, rc, rcExisting, 0, time.Millisecond, time.Millisecond); err != nil { diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 3e7b89f4f44..771b16d6c6b 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -65,7 +65,7 @@ type objInterface interface { func (reaper *ReplicationControllerReaper) Stop(namespace, name string) (string, error) { rc := reaper.ReplicationControllers(namespace) - resizer, err := ResizerFor("ReplicationController", *reaper) + resizer, err := ResizerFor("ReplicationController", &RealResizerClient{*reaper}) if err != nil { return "", err } From bd7b719944ca8c9f5d2f54b51b760f49a81abc91 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Wed, 15 Apr 2015 16:50:08 -0400 Subject: [PATCH 2/2] Make default impls private --- pkg/kubectl/cmd/rollingupdate.go | 2 +- pkg/kubectl/cmd/util/factory.go | 2 +- pkg/kubectl/resize.go | 22 ++++++++++++--------- pkg/kubectl/resize_test.go | 6 +++--- pkg/kubectl/rolling_updater.go | 30 ++++++++++++++++------------- pkg/kubectl/rolling_updater_test.go | 4 ++-- pkg/kubectl/stop.go | 2 +- 7 files changed, 38 insertions(+), 30 deletions(-) diff --git a/pkg/kubectl/cmd/rollingupdate.go b/pkg/kubectl/cmd/rollingupdate.go index 32a65c4ccbd..0b0293e506a 100644 --- a/pkg/kubectl/cmd/rollingupdate.go +++ b/pkg/kubectl/cmd/rollingupdate.go @@ -111,7 +111,7 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg return err } - updater := kubectl.NewRollingUpdater(newRc.Namespace, &kubectl.RealRollingUpdaterClient{client}) + updater := kubectl.NewRollingUpdater(newRc.Namespace, kubectl.NewRollingUpdaterClient(client)) // fetch rc oldRc, err := client.ReplicationControllers(newRc.Namespace).Get(oldName) diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 2cd314940c0..0f712a4e867 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -197,7 +197,7 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { if err != nil { return nil, err } - return kubectl.ResizerFor(mapping.Kind, &kubectl.RealResizerClient{client}) + return kubectl.ResizerFor(mapping.Kind, kubectl.NewResizerClient(client)) }, Reaper: func(mapping *meta.RESTMapping) (kubectl.Reaper, error) { client, err := clients.ClientForVersion(mapping.APIVersion) diff --git a/pkg/kubectl/resize.go b/pkg/kubectl/resize.go index ca82e65d84b..f35f48c1ba9 100644 --- a/pkg/kubectl/resize.go +++ b/pkg/kubectl/resize.go @@ -172,19 +172,23 @@ type ResizerClient interface { ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc } -// RealResizerClient is a ResizerClient which uses a Kube client. -type RealResizerClient struct { - Client client.Interface +func NewResizerClient(c client.Interface) ResizerClient { + return &realResizerClient{c} } -func (c *RealResizerClient) GetReplicationController(namespace, name string) (*api.ReplicationController, error) { - return c.Client.ReplicationControllers(namespace).Get(name) +// realResizerClient is a ResizerClient which uses a Kube client. +type realResizerClient struct { + client client.Interface } -func (c *RealResizerClient) UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { - return c.Client.ReplicationControllers(namespace).Update(rc) +func (c *realResizerClient) GetReplicationController(namespace, name string) (*api.ReplicationController, error) { + return c.client.ReplicationControllers(namespace).Get(name) } -func (c *RealResizerClient) ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc { - return client.ControllerHasDesiredReplicas(c.Client, rc) +func (c *realResizerClient) UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { + return c.client.ReplicationControllers(namespace).Update(rc) +} + +func (c *realResizerClient) ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc { + return client.ControllerHasDesiredReplicas(c.client, rc) } diff --git a/pkg/kubectl/resize_test.go b/pkg/kubectl/resize_test.go index 5c2d482af35..3a70751e81a 100644 --- a/pkg/kubectl/resize_test.go +++ b/pkg/kubectl/resize_test.go @@ -43,7 +43,7 @@ func (c *ErrorReplicationControllerClient) ReplicationControllers(namespace stri func TestReplicationControllerResizeRetry(t *testing.T) { fake := &ErrorReplicationControllerClient{Fake: testclient.Fake{}} - resizer := ReplicationControllerResizer{&RealResizerClient{fake}} + resizer := ReplicationControllerResizer{NewResizerClient(fake)} preconditions := ResizePrecondition{-1, ""} count := uint(3) name := "foo" @@ -67,7 +67,7 @@ func TestReplicationControllerResizeRetry(t *testing.T) { func TestReplicationControllerResize(t *testing.T) { fake := &testclient.Fake{} - resizer := ReplicationControllerResizer{&RealResizerClient{fake}} + resizer := ReplicationControllerResizer{NewResizerClient(fake)} preconditions := ResizePrecondition{-1, ""} count := uint(3) name := "foo" @@ -90,7 +90,7 @@ func TestReplicationControllerResizeFailsPreconditions(t *testing.T) { Replicas: 10, }, }) - resizer := ReplicationControllerResizer{&RealResizerClient{fake}} + resizer := ReplicationControllerResizer{NewResizerClient(fake)} preconditions := ResizePrecondition{2, ""} count := uint(3) name := "foo" diff --git a/pkg/kubectl/rolling_updater.go b/pkg/kubectl/rolling_updater.go index 0c4a901e8f7..7172b8663b6 100644 --- a/pkg/kubectl/rolling_updater.go +++ b/pkg/kubectl/rolling_updater.go @@ -207,27 +207,31 @@ type RollingUpdaterClient interface { ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc } -// RealRollingUpdaterClient is a RollingUpdaterClient which uses a Kube client. -type RealRollingUpdaterClient struct { - Client client.Interface +func NewRollingUpdaterClient(c client.Interface) RollingUpdaterClient { + return &realRollingUpdaterClient{c} } -func (c *RealRollingUpdaterClient) GetReplicationController(namespace, name string) (*api.ReplicationController, error) { - return c.Client.ReplicationControllers(namespace).Get(name) +// realRollingUpdaterClient is a RollingUpdaterClient which uses a Kube client. +type realRollingUpdaterClient struct { + client client.Interface } -func (c *RealRollingUpdaterClient) UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { - return c.Client.ReplicationControllers(namespace).Update(rc) +func (c *realRollingUpdaterClient) GetReplicationController(namespace, name string) (*api.ReplicationController, error) { + return c.client.ReplicationControllers(namespace).Get(name) } -func (c *RealRollingUpdaterClient) CreateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { - return c.Client.ReplicationControllers(namespace).Create(rc) +func (c *realRollingUpdaterClient) UpdateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { + return c.client.ReplicationControllers(namespace).Update(rc) } -func (c *RealRollingUpdaterClient) DeleteReplicationController(namespace, name string) error { - return c.Client.ReplicationControllers(namespace).Delete(name) +func (c *realRollingUpdaterClient) CreateReplicationController(namespace string, rc *api.ReplicationController) (*api.ReplicationController, error) { + return c.client.ReplicationControllers(namespace).Create(rc) } -func (c *RealRollingUpdaterClient) ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc { - return client.ControllerHasDesiredReplicas(c.Client, rc) +func (c *realRollingUpdaterClient) DeleteReplicationController(namespace, name string) error { + return c.client.ReplicationControllers(namespace).Delete(name) +} + +func (c *realRollingUpdaterClient) ControllerHasDesiredReplicas(rc *api.ReplicationController) wait.ConditionFunc { + return client.ControllerHasDesiredReplicas(c.client, rc) } diff --git a/pkg/kubectl/rolling_updater_test.go b/pkg/kubectl/rolling_updater_test.go index 2c05b79002c..144d7059c90 100644 --- a/pkg/kubectl/rolling_updater_test.go +++ b/pkg/kubectl/rolling_updater_test.go @@ -253,7 +253,7 @@ Update succeeded. Deleting foo-v1 for _, test := range tests { updater := RollingUpdater{ - &RealRollingUpdaterClient{fakeClientFor("default", test.responses)}, + NewRollingUpdaterClient(fakeClientFor("default", test.responses)), "default", } var buffer bytes.Buffer @@ -296,7 +296,7 @@ Update succeeded. Deleting foo-v1 {newRc(3, 3), nil}, {newRc(3, 3), nil}, } - updater := RollingUpdater{&RealRollingUpdaterClient{fakeClientFor("default", responses)}, "default"} + updater := RollingUpdater{NewRollingUpdaterClient(fakeClientFor("default", responses)), "default"} var buffer bytes.Buffer if err := updater.Update(&buffer, rc, rcExisting, 0, time.Millisecond, time.Millisecond); err != nil { diff --git a/pkg/kubectl/stop.go b/pkg/kubectl/stop.go index 771b16d6c6b..ba1d69d8442 100644 --- a/pkg/kubectl/stop.go +++ b/pkg/kubectl/stop.go @@ -65,7 +65,7 @@ type objInterface interface { func (reaper *ReplicationControllerReaper) Stop(namespace, name string) (string, error) { rc := reaper.ReplicationControllers(namespace) - resizer, err := ResizerFor("ReplicationController", &RealResizerClient{*reaper}) + resizer, err := ResizerFor("ReplicationController", NewResizerClient(*reaper)) if err != nil { return "", err }