From dc92d3c7a2443b639de22d9d40d853a3bb9e0fe7 Mon Sep 17 00:00:00 2001 From: nikhiljindal Date: Wed, 28 Jan 2015 20:41:37 -0800 Subject: [PATCH] Cleaning up the operations code in client --- cmd/integration/integration.go | 1 - cmd/kubernetes/kubernetes.go | 2 - pkg/api/types.go | 7 +-- pkg/api/v1beta1/types.go | 11 ++-- pkg/api/v1beta2/types.go | 11 ++-- pkg/api/v1beta3/types.go | 7 +-- pkg/apiserver/apiserver_test.go | 5 +- pkg/client/request.go | 53 +--------------- pkg/client/request_test.go | 90 ---------------------------- pkg/client/restclient.go | 34 +---------- pkg/client/restclient_test.go | 21 +++---- pkg/registry/controller/rest.go | 11 ++-- pkg/registry/controller/rest_test.go | 15 ++--- 13 files changed, 38 insertions(+), 230 deletions(-) diff --git a/cmd/integration/integration.go b/cmd/integration/integration.go index c2301923237..d9a41e4ea4b 100644 --- a/cmd/integration/integration.go +++ b/cmd/integration/integration.go @@ -135,7 +135,6 @@ func startComponents(manifestURL string) (apiServerURL string) { } cl := client.NewOrDie(&client.Config{Host: apiServer.URL, Version: testapi.Version()}) - cl.PollPeriod = time.Millisecond * 100 helper, err := master.NewEtcdHelper(etcdClient, "") if err != nil { diff --git a/cmd/kubernetes/kubernetes.go b/cmd/kubernetes/kubernetes.go index 3bc67402891..ad22881dcab 100644 --- a/cmd/kubernetes/kubernetes.go +++ b/cmd/kubernetes/kubernetes.go @@ -22,7 +22,6 @@ package main import ( "fmt" - "time" kubeletapp "github.com/GoogleCloudPlatform/kubernetes/cmd/kubelet/app" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" @@ -61,7 +60,6 @@ func startComponents(etcdClient tools.EtcdClient, cl *client.Client, addr string func newApiClient(addr string, port int) *client.Client { apiServerURL := fmt.Sprintf("http://%s:%d", addr, port) cl := client.NewOrDie(&client.Config{Host: apiServerURL, Version: testapi.Version()}) - cl.PollPeriod = time.Second * 1 return cl } diff --git a/pkg/api/types.go b/pkg/api/types.go index 3e93147ba11..20c8b510473 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -821,12 +821,12 @@ type Status struct { TypeMeta `json:",inline"` ListMeta `json:"metadata,omitempty"` - // One of: "Success", "Failure", "Working" (for operations not yet completed) + // One of: "Success" or "Failure" Status string `json:"status,omitempty"` // A human-readable description of the status of this operation. Message string `json:"message,omitempty"` // A machine-readable description of why this operation is in the - // "Failure" or "Working" status. If this value is empty there + // "Failure" status. If this value is empty there // is no information available. A Reason clarifies an HTTP status // code but does not override it. Reason StatusReason `json:"reason,omitempty"` @@ -862,7 +862,6 @@ type StatusDetails struct { const ( StatusSuccess = "Success" StatusFailure = "Failure" - StatusWorking = "Working" ) // StatusReason is an enumeration of possible failure causes. Each StatusReason @@ -973,7 +972,7 @@ type StatusCause struct { // CauseType is a machine readable value providing more detail about what // occured in a status response. An operation may have multiple causes for a -// status (whether Failure, Success, or Working). +// status (whether Failure or Success). type CauseType string const ( diff --git a/pkg/api/v1beta1/types.go b/pkg/api/v1beta1/types.go index cfc20404b28..99d1460e056 100644 --- a/pkg/api/v1beta1/types.go +++ b/pkg/api/v1beta1/types.go @@ -636,15 +636,15 @@ type Binding struct { // import both. type Status struct { TypeMeta `json:",inline"` - // One of: "Success", "Failure", "Working" (for operations not yet completed) - Status string `json:"status,omitempty" description:"status of the operation; either Working (not yet completed), Success, or Failure"` + // One of: "Success" or "Failure". + Status string `json:"status,omitempty" description:"status of the operation; either Success, or Failure"` // A human-readable description of the status of this operation. Message string `json:"message,omitempty" description:"human-readable description of the status of this operation"` // A machine-readable description of why this operation is in the - // "Failure" or "Working" status. If this value is empty there + // "Failure" status. If this value is empty there // is no information available. A Reason clarifies an HTTP status // code but does not override it. - Reason StatusReason `json:"reason,omitempty" description:"machine-readable description of why this operation is in the 'Failure' or 'Working' status; if this value is empty there is no information available; a reason clarifies an HTTP status code but does not override it"` + Reason StatusReason `json:"reason,omitempty" description:"machine-readable description of why this operation is in the 'Failure' status; if this value is empty there is no information available; a reason clarifies an HTTP status code but does not override it"` // Extended data associated with the reason. Each reason may define its // own extended details. This field is optional and the data returned // is not guaranteed to conform to any schema except that defined by @@ -676,7 +676,6 @@ type StatusDetails struct { const ( StatusSuccess = "Success" StatusFailure = "Failure" - StatusWorking = "Working" ) // StatusReason is an enumeration of possible failure causes. Each StatusReason @@ -739,7 +738,7 @@ type StatusCause struct { // CauseType is a machine readable value providing more detail about what // occured in a status response. An operation may have multiple causes for a -// status (whether Failure, Success, or Working). +// status (whether Failure or Success). type CauseType string const ( diff --git a/pkg/api/v1beta2/types.go b/pkg/api/v1beta2/types.go index f711f2506f2..39c1eb49538 100644 --- a/pkg/api/v1beta2/types.go +++ b/pkg/api/v1beta2/types.go @@ -597,15 +597,15 @@ type Binding struct { // import both. type Status struct { TypeMeta `json:",inline"` - // One of: "Success", "Failure", "Working" (for operations not yet completed) - Status string `json:"status,omitempty" description:"status of the operation; either Working (not yet completed), Success, or Failure"` + // One of: "Success" or "Failure" + Status string `json:"status,omitempty" description:"status of the operation; either Success or Failure"` // A human-readable description of the status of this operation. Message string `json:"message,omitempty" description:"human-readable description of the status of this operation"` // A machine-readable description of why this operation is in the - // "Failure" or "Working" status. If this value is empty there + // "Failure" status. If this value is empty there // is no information available. A Reason clarifies an HTTP status // code but does not override it. - Reason StatusReason `json:"reason,omitempty" description:"machine-readable description of why this operation is in the 'Failure' or 'Working' status; if this value is empty there is no information available; a reason clarifies an HTTP status code but does not override it"` + Reason StatusReason `json:"reason,omitempty" description:"machine-readable description of why this operation is in the 'Failure' status; if this value is empty there is no information available; a reason clarifies an HTTP status code but does not override it"` // Extended data associated with the reason. Each reason may define its // own extended details. This field is optional and the data returned // is not guaranteed to conform to any schema except that defined by @@ -637,7 +637,6 @@ type StatusDetails struct { const ( StatusSuccess = "Success" StatusFailure = "Failure" - StatusWorking = "Working" ) // StatusReason is an enumeration of possible failure causes. Each StatusReason @@ -713,7 +712,7 @@ type StatusCause struct { // CauseType is a machine readable value providing more detail about what // occured in a status response. An operation may have multiple causes for a -// status (whether Failure, Success, or Working). +// status (whether Failure or Success). type CauseType string const ( diff --git a/pkg/api/v1beta3/types.go b/pkg/api/v1beta3/types.go index 0f0a9391fbb..015e546602f 100644 --- a/pkg/api/v1beta3/types.go +++ b/pkg/api/v1beta3/types.go @@ -832,12 +832,12 @@ type Status struct { TypeMeta `json:",inline"` ListMeta `json:"metadata,omitempty"` - // One of: "Success", "Failure", "Working" (for operations not yet completed) + // One of: "Success" or "Failure" Status string `json:"status,omitempty"` // A human-readable description of the status of this operation. Message string `json:"message,omitempty"` // A machine-readable description of why this operation is in the - // "Failure" or "Working" status. If this value is empty there + // "Failure" status. If this value is empty there // is no information available. A Reason clarifies an HTTP status // code but does not override it. Reason StatusReason `json:"reason,omitempty"` @@ -872,7 +872,6 @@ type StatusDetails struct { const ( StatusSuccess = "Success" StatusFailure = "Failure" - StatusWorking = "Working" ) // StatusReason is an enumeration of possible failure causes. Each StatusReason @@ -948,7 +947,7 @@ type StatusCause struct { // CauseType is a machine readable value providing more detail about what // occured in a status response. An operation may have multiple causes for a -// status (whether Failure, Success, or Working). +// status (whether Failure or Success). type CauseType string const ( diff --git a/pkg/apiserver/apiserver_test.go b/pkg/apiserver/apiserver_test.go index 8c4f135b8cb..50840d1aba4 100644 --- a/pkg/apiserver/apiserver_test.go +++ b/pkg/apiserver/apiserver_test.go @@ -74,15 +74,14 @@ func interfacesFor(version string) (*meta.VersionInterfaces, error) { func init() { // Certain API objects are returned regardless of the contents of storage: // api.Status is returned in errors - // api.Operation/api.OperationList are returned by /operations // "internal" version api.Scheme.AddKnownTypes("", &Simple{}, &SimpleList{}, - &api.Status{}, &api.Operation{}, &api.OperationList{}) + &api.Status{}) // "version" version // TODO: Use versioned api objects? api.Scheme.AddKnownTypes(testVersion, &Simple{}, &SimpleList{}, - &api.Status{}, &api.Operation{}, &api.OperationList{}) + &api.Status{}) defMapper := meta.NewDefaultRESTMapper( versions, diff --git a/pkg/client/request.go b/pkg/client/request.go index 953bc5fd7ef..e9b5b50f72c 100644 --- a/pkg/client/request.go +++ b/pkg/client/request.go @@ -42,12 +42,6 @@ import ( // are therefore not allowed to set manually. var specialParams = util.NewStringSet("timeout") -// PollFunc is called when a server operation returns 202 accepted. The name of the -// operation is extracted from the response and passed to this function. Return a -// request to retrieve the result of the operation, or false for the second argument -// if polling should end. -type PollFunc func(name string) (*Request, bool) - // HTTPClient is an interface for testing a request object. type HTTPClient interface { Do(req *http.Request) (*http.Response, error) @@ -86,10 +80,6 @@ type Request struct { baseURL *url.URL codec runtime.Codec - // optional, will be invoked if the server returns a 202 to decide - // whether to poll. - poller PollFunc - // If true, add "?namespace=" as a query parameter, if false put ns/ in path // Query parameter is considered legacy behavior namespaceInQuery bool @@ -305,22 +295,6 @@ func (r *Request) Body(obj interface{}) *Request { return r } -// NoPoll indicates a server "working" response should be returned as an error -func (r *Request) NoPoll() *Request { - return r.Poller(nil) -} - -// Poller indicates this request should use the specified poll function to determine whether -// a server "working" response should be retried. The poller is responsible for waiting or -// outputting messages to the client. -func (r *Request) Poller(poller PollFunc) *Request { - if r.err != nil { - return r - } - r.poller = poller - return r -} - func (r *Request) finalURL() string { p := r.path if r.namespaceSet && !r.namespaceInQuery && len(r.namespace) > 0 { @@ -430,7 +404,7 @@ func (r *Request) Stream() (io.ReadCloser, error) { } // Do formats and executes the request. Returns a Result object for easy response -// processing. Handles polling the server in the event a continuation was sent. +// processing. // // Error type: // * If the request can't be constructed, or an error happened earlier while building its @@ -464,10 +438,6 @@ func (r *Request) Do() Result { } respBody, created, err := r.transformResponse(resp, req) - if poll, ok := r.shouldPoll(err); ok { - r = poll - continue - } // Check to see if we got a 429 Too Many Requests response code. if resp.StatusCode == errors.StatusTooManyRequests { @@ -487,27 +457,6 @@ func (r *Request) Do() Result { } } -// shouldPoll checks the server error for an incomplete operation -// and if found returns a request that would check the response. -// If no polling is necessary or possible, it will return false. -func (r *Request) shouldPoll(err error) (*Request, bool) { - if err == nil || r.poller == nil { - return nil, false - } - apistatus, ok := err.(APIStatus) - if !ok { - return nil, false - } - status := apistatus.Status() - if status.Status != api.StatusWorking { - return nil, false - } - if status.Details == nil || len(status.Details.ID) == 0 { - return nil, false - } - return r.poller(status.Details.ID) -} - // transformResponse converts an API response into a structured API object. func (r *Request) transformResponse(resp *http.Response, req *http.Request) ([]byte, bool, error) { defer resp.Body.Close() diff --git a/pkg/client/request_test.go b/pkg/client/request_test.go index 01ccf8d3cc8..1439625d0ee 100644 --- a/pkg/client/request_test.go +++ b/pkg/client/request_test.go @@ -44,10 +44,6 @@ import ( watchjson "github.com/GoogleCloudPlatform/kubernetes/pkg/watch/json" ) -func skipPolling(name string) (*Request, bool) { - return nil, false -} - func TestRequestWithErrorWontChange(t *testing.T) { original := Request{err: errors.New("test")} r := original @@ -61,9 +57,7 @@ func TestRequestWithErrorWontChange(t *testing.T) { Namespace("new"). Resource("foos"). Name("bars"). - NoPoll(). Body("foo"). - Poller(skipPolling). Timeout(time.Millisecond) if changed != &r { t.Errorf("returned request should point to the same object") @@ -768,90 +762,6 @@ func TestBody(t *testing.T) { } } -func TestSetPoller(t *testing.T) { - c := NewOrDie(&Config{}) - r := c.Get() - if c.PollPeriod == 0 { - t.Errorf("polling should be on by default") - } - if r.poller == nil { - t.Errorf("polling should be on by default") - } - r.NoPoll() - if r.poller != nil { - t.Errorf("'NoPoll' doesn't work") - } -} - -func TestPolling(t *testing.T) { - objects := []runtime.Object{ - &api.Status{Status: api.StatusWorking, Details: &api.StatusDetails{ID: "1234"}}, - &api.Status{Status: api.StatusWorking, Details: &api.StatusDetails{ID: "1234"}}, - &api.Status{Status: api.StatusWorking, Details: &api.StatusDetails{ID: "1234"}}, - &api.Status{Status: api.StatusWorking, Details: &api.StatusDetails{ID: "1234"}}, - &api.Status{Status: api.StatusSuccess}, - } - - callNumber := 0 - testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if callNumber == 0 { - if r.URL.Path != "/api/v1beta1/" { - t.Fatalf("unexpected request URL path %s", r.URL.Path) - } - } else { - if r.URL.Path != "/api/v1beta1/operations/1234" { - t.Fatalf("unexpected request URL path %s", r.URL.Path) - } - } - t.Logf("About to write %d", callNumber) - data, err := v1beta1.Codec.Encode(objects[callNumber]) - if err != nil { - t.Errorf("Unexpected encode error") - } - callNumber++ - w.Write(data) - })) - - c := NewOrDie(&Config{Host: testServer.URL, Version: "v1beta1", Username: "user", Password: "pass"}) - c.PollPeriod = 1 * time.Millisecond - trials := []func(){ - func() { - // Check that we do indeed poll when asked to. - obj, err := c.Get().Do().Get() - if err != nil { - t.Errorf("Unexpected error: %v %#v", err, err) - return - } - if s, ok := obj.(*api.Status); !ok || s.Status != api.StatusSuccess { - t.Errorf("Unexpected return object: %#v", obj) - return - } - if callNumber != len(objects) { - t.Errorf("Unexpected number of calls: %v", callNumber) - } - }, - func() { - // Check that we don't poll when asked not to. - obj, err := c.Get().NoPoll().Do().Get() - if err == nil { - t.Errorf("Unexpected non error: %v", obj) - return - } - if se, ok := err.(APIStatus); !ok || se.Status().Status != api.StatusWorking { - t.Errorf("Unexpected kind of error: %#v", err) - return - } - if callNumber != 1 { - t.Errorf("Unexpected number of calls: %v", callNumber) - } - }, - } - for _, f := range trials { - callNumber = 0 - f() - } -} - func authFromReq(r *http.Request) (*Config, bool) { auth, ok := r.Header["Authorization"] if !ok { diff --git a/pkg/client/restclient.go b/pkg/client/restclient.go index c6d0b26d1d2..a481e83fabd 100644 --- a/pkg/client/restclient.go +++ b/pkg/client/restclient.go @@ -22,8 +22,6 @@ import ( "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" - - "github.com/golang/glog" ) // RESTClient imposes common Kubernetes API conventions on a set of resource paths. @@ -51,12 +49,7 @@ type RESTClient struct { // used. Client HTTPClient - // Set the poll behavior of this client. If not set the DefaultPoll method will - // be called. - Poller PollFunc - - PollPeriod time.Duration - Timeout time.Duration + Timeout time.Duration } // NewRESTClient creates a new RESTClient. This client performs generic REST functions @@ -78,9 +71,6 @@ func NewRESTClient(baseURL *url.URL, apiVersion string, c runtime.Codec, legacyB Codec: c, LegacyBehavior: legacyBehavior, - - // Poll frequently - PollPeriod: time.Second * 2, } } @@ -102,11 +92,7 @@ func (c *RESTClient) Verb(verb string) *Request { // if c.Client != nil { // timeout = c.Client.Timeout // } - poller := c.Poller - if poller == nil { - poller = c.DefaultPoll - } - return NewRequest(c.Client, verb, c.baseURL, c.Codec, c.LegacyBehavior, c.LegacyBehavior).Poller(poller).Timeout(c.Timeout) + return NewRequest(c.Client, verb, c.baseURL, c.Codec, c.LegacyBehavior, c.LegacyBehavior).Timeout(c.Timeout) } // Post begins a POST request. Short for c.Verb("POST"). @@ -129,22 +115,6 @@ func (c *RESTClient) Delete() *Request { return c.Verb("DELETE") } -// PollFor makes a request to do a single poll of the completion of the given operation. -func (c *RESTClient) Operation(name string) *Request { - return c.Get().Resource("operations").Name(name).NoPoll() -} - -// DefaultPoll performs a polling action based on the PollPeriod set on the Client. -func (c *RESTClient) DefaultPoll(name string) (*Request, bool) { - if c.PollPeriod == 0 { - return nil, false - } - glog.Infof("Waiting for completion of operation %s", name) - time.Sleep(c.PollPeriod) - // Make a poll request - return c.Operation(name).Poller(c.DefaultPoll), true -} - // APIVersion returns the APIVersion this RESTClient is expected to use. func (c *RESTClient) APIVersion() string { return c.apiVersion diff --git a/pkg/client/restclient_test.go b/pkg/client/restclient_test.go index 073e1c4bb8c..7f50bd8d371 100644 --- a/pkg/client/restclient_test.go +++ b/pkg/client/restclient_test.go @@ -157,10 +157,10 @@ func TestValidatesHostParameter(t *testing.T) { } func TestDoRequestBearer(t *testing.T) { - status := &api.Status{Status: api.StatusWorking} + status := &api.Status{Status: api.StatusFailure} expectedBody, _ := latest.Codec.Encode(status) fakeHandler := util.FakeHandler{ - StatusCode: 202, + StatusCode: 400, ResponseBody: string(expectedBody), T: t, } @@ -187,11 +187,11 @@ func TestDoRequestBearer(t *testing.T) { } } -func TestDoRequestAccepted(t *testing.T) { - status := &api.Status{Status: api.StatusWorking} +func TestDoRequestWithoutPassword(t *testing.T) { + status := &api.Status{Status: api.StatusFailure} expectedBody, _ := latest.Codec.Encode(status) fakeHandler := util.FakeHandler{ - StatusCode: 202, + StatusCode: 400, ResponseBody: string(expectedBody), T: t, } @@ -228,11 +228,11 @@ func TestDoRequestAccepted(t *testing.T) { fakeHandler.ValidateRequest(t, "/"+testapi.Version()+"/test", "GET", nil) } -func TestDoRequestAcceptedSuccess(t *testing.T) { +func TestDoRequestSuccess(t *testing.T) { status := &api.Status{Status: api.StatusSuccess} expectedBody, _ := latest.Codec.Encode(status) fakeHandler := util.FakeHandler{ - StatusCode: 202, + StatusCode: 200, ResponseBody: string(expectedBody), T: t, } @@ -339,10 +339,3 @@ func TestDoRequestCreated(t *testing.T) { } fakeHandler.ValidateRequest(t, "/"+testapi.Version()+"/test", "GET", nil) } - -func TestDefaultPoll(t *testing.T) { - c := &RESTClient{PollPeriod: 0} - if req, ok := c.DefaultPoll("test"); req != nil || ok { - t.Errorf("expected nil request and not poll") - } -} diff --git a/pkg/registry/controller/rest.go b/pkg/registry/controller/rest.go index 350985f1f2a..5171914af59 100644 --- a/pkg/registry/controller/rest.go +++ b/pkg/registry/controller/rest.go @@ -18,7 +18,6 @@ package controller import ( "fmt" - "time" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" @@ -38,17 +37,15 @@ type PodLister interface { // REST implements apiserver.RESTStorage for the replication controller service. type REST struct { - registry Registry - podLister PodLister - pollPeriod time.Duration + registry Registry + podLister PodLister } // NewREST returns a new apiserver.RESTStorage for the given registry and PodLister. func NewREST(registry Registry, podLister PodLister) *REST { return &REST{ - registry: registry, - podLister: podLister, - pollPeriod: time.Second * 10, + registry: registry, + podLister: podLister, } } diff --git a/pkg/registry/controller/rest_test.go b/pkg/registry/controller/rest_test.go index 585a956c0ce..b084447d470 100644 --- a/pkg/registry/controller/rest_test.go +++ b/pkg/registry/controller/rest_test.go @@ -243,9 +243,8 @@ func TestCreateController(t *testing.T) { }, } storage := REST{ - registry: &mockRegistry, - podLister: &mockPodRegistry, - pollPeriod: time.Millisecond * 1, + registry: &mockRegistry, + podLister: &mockPodRegistry, } controller := &api.ReplicationController{ ObjectMeta: api.ObjectMeta{Name: "test"}, @@ -278,9 +277,8 @@ func TestCreateController(t *testing.T) { func TestControllerStorageValidatesCreate(t *testing.T) { mockRegistry := registrytest.ControllerRegistry{} storage := REST{ - registry: &mockRegistry, - podLister: nil, - pollPeriod: time.Millisecond * 1, + registry: &mockRegistry, + podLister: nil, } failureCases := map[string]api.ReplicationController{ "empty ID": { @@ -309,9 +307,8 @@ func TestControllerStorageValidatesCreate(t *testing.T) { func TestControllerStorageValidatesUpdate(t *testing.T) { mockRegistry := registrytest.ControllerRegistry{} storage := REST{ - registry: &mockRegistry, - podLister: nil, - pollPeriod: time.Millisecond * 1, + registry: &mockRegistry, + podLister: nil, } failureCases := map[string]api.ReplicationController{ "empty ID": {