Merge lp:~rvb/gomaasapi/gomaasapi-bug-1384001 into lp:gomaasapi

Proposed by Raphaël Badin on 2014-10-27
Status: Merged
Approved by: Ian Booth on 2014-10-27
Approved revision: 63
Merged at revision: 58
Proposed branch: lp:~rvb/gomaasapi/gomaasapi-bug-1384001
Merge into: lp:gomaasapi
Diff against target: 195 lines (+137/-3)
3 files modified
client.go (+50/-3)
client_test.go (+50/-0)
testing.go (+37/-0)
To merge this branch: bzr merge lp:~rvb/gomaasapi/gomaasapi-bug-1384001
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve on 2014-10-27
Juju Engineering 2014-10-27 Pending
Review via email: mp+239738@code.launchpad.net

Commit message

Retry requests that result in a 503 response with the Retry-After header set.

Description of the change

To post a comment you must log in.
60. By Raphaël Badin on 2014-10-27

Lint fixes.

61. By Raphaël Badin on 2014-10-27

Update comment.

Gavin Panella (allenap) wrote :

My eyes are bleeding, but otherwise it seems okay.

review: Approve
62. By Raphaël Badin on 2014-10-27

Review fixes.

63. By Raphaël Badin on 2014-10-27

Retry more.

Julian Edwards (julian-edwards) wrote :

Small point of order - MAAS doesn't set the Retry-After header (yet).

Julian Edwards (julian-edwards) wrote :

Ah my bad, saw the maas change you made after I had written that.

Andrew Wilkins (axwalk) wrote :

I think it'd be good to have an option to handle this outside of gomaasapi as well. In Juju, the provisioner runs in a single goroutine (there are good reasons for that) and so retrying a single request will block others. It'd be good to just reschedule the provisioning for the failed one.

For other operations (e.g. releasing all nodes) I think this behaviour would be fine.

64. By Raphaël Badin on 2014-10-28

Restore body before issuing request.

Ian Booth (wallyworld) wrote :

@Andrew,

The new behaviour in here now matches what goose does. When Openstack's rate limit is exceeded, a 403 response comes back and goose will retry after the time set by the retry-after header. This is independent of what request is being made. I think this is ok given that machine provisioning generally takes a while anyway, and adding in a few retries hopefully won't materially affect the total time. Not that there's not scope to discuss if this could be improved across the board.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'client.go'
--- client.go 2013-10-16 23:56:55 +0000
+++ client.go 2014-10-28 08:33:05 +0000
@@ -11,7 +11,18 @@
11 "mime/multipart"11 "mime/multipart"
12 "net/http"12 "net/http"
13 "net/url"13 "net/url"
14 "strconv"
14 "strings"15 "strings"
16 "time"
17)
18
19const (
20 // Number of retries performed when the server returns a 503
21 // response with a 'Retry-after' header. A request will be issued
22 // at most NumberOfRetries + 1 times.
23 NumberOfRetries = 4
24
25 RetryAfterHeaderName = "Retry-After"
15)26)
1627
17// Client represents a way to communicating with a MAAS API instance.28// Client represents a way to communicating with a MAAS API instance.
@@ -23,10 +34,11 @@
2334
24// ServerError is an http error (or at least, a non-2xx result) received from35// ServerError is an http error (or at least, a non-2xx result) received from
25// the server. It contains the numerical HTTP status code as well as an error36// the server. It contains the numerical HTTP status code as well as an error
26// string.37// string and the response's headers.
27type ServerError struct {38type ServerError struct {
28 error39 error
29 StatusCode int40 StatusCode int
41 Header http.Header
30}42}
3143
32// readAndClose reads and closes the given ReadCloser.44// readAndClose reads and closes the given ReadCloser.
@@ -44,8 +56,43 @@
44// Client-side errors will return an empty response and a non-nil error. For56// Client-side errors will return an empty response and a non-nil error. For
45// server-side errors however (i.e. responses with a non 2XX status code), the57// server-side errors however (i.e. responses with a non 2XX status code), the
46// returned error will be ServerError and the returned body will reflect the58// returned error will be ServerError and the returned body will reflect the
47// server's response.59// server's response. If the server returns a 503 response with a 'Retry-after'
60// header, the request will be transparenty retried.
48func (client Client) dispatchRequest(request *http.Request) ([]byte, error) {61func (client Client) dispatchRequest(request *http.Request) ([]byte, error) {
62 // First, store the request's body into a byte[] to be able to restore it
63 // after each request.
64 bodyContent, err := readAndClose(request.Body)
65 if err != nil {
66 return nil, err
67 }
68 for retry := 0; retry < NumberOfRetries; retry++ {
69 // Restore body before issuing request.
70 newBody := ioutil.NopCloser(bytes.NewReader(bodyContent))
71 request.Body = newBody
72 body, err := client.dispatchSingleRequest(request)
73 // If this is a 503 response with a non-void "Retry-After" header: wait
74 // as instructed and retry the request.
75 if err != nil {
76 serverError, ok := err.(ServerError)
77 if ok && serverError.StatusCode == http.StatusServiceUnavailable {
78 retry_time_int, errConv := strconv.Atoi(serverError.Header.Get(RetryAfterHeaderName))
79 if errConv == nil {
80 select {
81 case <-time.After(time.Duration(retry_time_int) * time.Second):
82 }
83 continue
84 }
85 }
86 }
87 return body, err
88 }
89 // Restore body before issuing request.
90 newBody := ioutil.NopCloser(bytes.NewReader(bodyContent))
91 request.Body = newBody
92 return client.dispatchSingleRequest(request)
93}
94
95func (client Client) dispatchSingleRequest(request *http.Request) ([]byte, error) {
49 client.Signer.OAuthSign(request)96 client.Signer.OAuthSign(request)
50 httpClient := http.Client{}97 httpClient := http.Client{}
51 // See https://code.google.com/p/go/issues/detail?id=467798 // See https://code.google.com/p/go/issues/detail?id=4677
@@ -62,7 +109,7 @@
62 }109 }
63 if response.StatusCode < 200 || response.StatusCode > 299 {110 if response.StatusCode < 200 || response.StatusCode > 299 {
64 msg := fmt.Errorf("gomaasapi: got error back from server: %v (%v)", response.Status, string(body))111 msg := fmt.Errorf("gomaasapi: got error back from server: %v (%v)", response.Status, string(body))
65 return body, ServerError{error: msg, StatusCode: response.StatusCode}112 return body, ServerError{error: msg, StatusCode: response.StatusCode, Header: response.Header}
66 }113 }
67 return body, nil114 return body, nil
68}115}
69116
=== modified file 'client_test.go'
--- client_test.go 2013-09-02 12:18:41 +0000
+++ client_test.go 2014-10-28 08:33:05 +0000
@@ -50,6 +50,56 @@
50 c.Check(string(result), Equals, expectedResult)50 c.Check(string(result), Equals, expectedResult)
51}51}
5252
53func (suite *ClientSuite) TestClientdispatchRequestRetries503(c *C) {
54 URI := "/some/url/?param1=test"
55 server := newFlakyServer(URI, 503, NumberOfRetries)
56 defer server.Close()
57 client, err := NewAnonymousClient(server.URL, "1.0")
58 c.Assert(err, IsNil)
59 content := "content"
60 request, err := http.NewRequest("GET", server.URL+URI, ioutil.NopCloser(strings.NewReader(content)))
61
62 _, err = client.dispatchRequest(request)
63
64 c.Check(err, IsNil)
65 c.Check(*server.nbRequests, Equals, NumberOfRetries+1)
66 expectedRequestsContent := make([][]byte, NumberOfRetries+1)
67 for i := 0; i < NumberOfRetries+1; i++ {
68 expectedRequestsContent[i] = []byte(content)
69 }
70 c.Check(*server.requests, DeepEquals, expectedRequestsContent)
71}
72
73func (suite *ClientSuite) TestClientdispatchRequestDoesntRetry200(c *C) {
74 URI := "/some/url/?param1=test"
75 server := newFlakyServer(URI, 200, 10)
76 defer server.Close()
77 client, err := NewAnonymousClient(server.URL, "1.0")
78 c.Assert(err, IsNil)
79
80 request, err := http.NewRequest("GET", server.URL+URI, nil)
81
82 _, err = client.dispatchRequest(request)
83
84 c.Check(err, IsNil)
85 c.Check(*server.nbRequests, Equals, 1)
86}
87
88func (suite *ClientSuite) TestClientdispatchRequestRetriesIsLimited(c *C) {
89 URI := "/some/url/?param1=test"
90 // Make the server return 503 responses NumberOfRetries + 1 times.
91 server := newFlakyServer(URI, 503, NumberOfRetries+1)
92 defer server.Close()
93 client, err := NewAnonymousClient(server.URL, "1.0")
94 c.Assert(err, IsNil)
95 request, err := http.NewRequest("GET", server.URL+URI, nil)
96
97 _, err = client.dispatchRequest(request)
98
99 c.Check(*server.nbRequests, Equals, NumberOfRetries+1)
100 c.Check(err.(ServerError).StatusCode, Equals, 503)
101}
102
53func (suite *ClientSuite) TestClientDispatchRequestReturnsNonServerError(c *C) {103func (suite *ClientSuite) TestClientDispatchRequestReturnsNonServerError(c *C) {
54 client, err := NewAnonymousClient("/foo", "1.0")104 client, err := NewAnonymousClient("/foo", "1.0")
55 c.Assert(err, IsNil)105 c.Assert(err, IsNil)
56106
=== modified file 'testing.go'
--- testing.go 2013-07-30 08:11:08 +0000
+++ testing.go 2014-10-28 08:33:05 +0000
@@ -43,3 +43,40 @@
43 server := httptest.NewServer(http.HandlerFunc(handler))43 server := httptest.NewServer(http.HandlerFunc(handler))
44 return &singleServingServer{server, &requestContent, &requestHeader}44 return &singleServingServer{server, &requestContent, &requestHeader}
45}45}
46
47type flakyServer struct {
48 *httptest.Server
49 nbRequests *int
50 requests *[][]byte
51}
52
53// newFlakyServer creates a "flaky" test http server which will
54// return `nbFlakyResponses` responses with the given code and then a 200 response.
55func newFlakyServer(uri string, code int, nbFlakyResponses int) *flakyServer {
56 nbRequests := 0
57 requests := make([][]byte, nbFlakyResponses+1)
58 handler := func(writer http.ResponseWriter, request *http.Request) {
59 nbRequests += 1
60 body, err := readAndClose(request.Body)
61 if err != nil {
62 panic(err)
63 }
64 requests[nbRequests-1] = body
65 if request.URL.String() != uri {
66 errorMsg := fmt.Sprintf("Error 404: page not found (expected '%v', got '%v').", uri, request.URL.String())
67 http.Error(writer, errorMsg, http.StatusNotFound)
68 } else if nbRequests <= nbFlakyResponses {
69 if code == http.StatusServiceUnavailable {
70 writer.Header().Set("Retry-After", "0")
71 }
72 writer.WriteHeader(code)
73 fmt.Fprint(writer, "flaky")
74 } else {
75 writer.WriteHeader(http.StatusOK)
76 fmt.Fprint(writer, "ok")
77 }
78
79 }
80 server := httptest.NewServer(http.HandlerFunc(handler))
81 return &flakyServer{server, &nbRequests, &requests}
82}

Subscribers

People subscribed via source and target branches

to all changes: