Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some more operations code cleanup #3921

Merged
merged 1 commit into from
Jan 29, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion cmd/integration/integration.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 0 additions & 2 deletions cmd/kubernetes/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ package main

import (
"fmt"
"time"

kubeletapp "github.com/GoogleCloudPlatform/kubernetes/cmd/kubelet/app"
"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
Expand Down Expand Up @@ -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
}

Expand Down
7 changes: 3 additions & 4 deletions pkg/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -862,7 +862,6 @@ type StatusDetails struct {
const (
StatusSuccess = "Success"
StatusFailure = "Failure"
StatusWorking = "Working"
)

// StatusReason is an enumeration of possible failure causes. Each StatusReason
Expand Down Expand Up @@ -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 (
Expand Down
11 changes: 5 additions & 6 deletions pkg/api/v1beta1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -676,7 +676,6 @@ type StatusDetails struct {
const (
StatusSuccess = "Success"
StatusFailure = "Failure"
StatusWorking = "Working"
)

// StatusReason is an enumeration of possible failure causes. Each StatusReason
Expand Down Expand Up @@ -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 (
Expand Down
11 changes: 5 additions & 6 deletions pkg/api/v1beta2/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -637,7 +637,6 @@ type StatusDetails struct {
const (
StatusSuccess = "Success"
StatusFailure = "Failure"
StatusWorking = "Working"
)

// StatusReason is an enumeration of possible failure causes. Each StatusReason
Expand Down Expand Up @@ -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 (
Expand Down
7 changes: 3 additions & 4 deletions pkg/api/v1beta3/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -872,7 +872,6 @@ type StatusDetails struct {
const (
StatusSuccess = "Success"
StatusFailure = "Failure"
StatusWorking = "Working"
)

// StatusReason is an enumeration of possible failure causes. Each StatusReason
Expand Down Expand Up @@ -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 (
Expand Down
5 changes: 2 additions & 3 deletions pkg/apiserver/apiserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
53 changes: 1 addition & 52 deletions pkg/client/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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=<namespace>" as a query parameter, if false put ns/<namespace> in path
// Query parameter is considered legacy behavior
namespaceInQuery bool
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
Expand Down
90 changes: 0 additions & 90 deletions pkg/client/request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -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 {
Expand Down