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
1=== modified file 'client.go'
2--- client.go 2013-10-16 23:56:55 +0000
3+++ client.go 2014-10-28 08:33:05 +0000
4@@ -11,7 +11,18 @@
5 "mime/multipart"
6 "net/http"
7 "net/url"
8+ "strconv"
9 "strings"
10+ "time"
11+)
12+
13+const (
14+ // Number of retries performed when the server returns a 503
15+ // response with a 'Retry-after' header. A request will be issued
16+ // at most NumberOfRetries + 1 times.
17+ NumberOfRetries = 4
18+
19+ RetryAfterHeaderName = "Retry-After"
20 )
21
22 // Client represents a way to communicating with a MAAS API instance.
23@@ -23,10 +34,11 @@
24
25 // ServerError is an http error (or at least, a non-2xx result) received from
26 // the server. It contains the numerical HTTP status code as well as an error
27-// string.
28+// string and the response's headers.
29 type ServerError struct {
30 error
31 StatusCode int
32+ Header http.Header
33 }
34
35 // readAndClose reads and closes the given ReadCloser.
36@@ -44,8 +56,43 @@
37 // Client-side errors will return an empty response and a non-nil error. For
38 // server-side errors however (i.e. responses with a non 2XX status code), the
39 // returned error will be ServerError and the returned body will reflect the
40-// server's response.
41+// server's response. If the server returns a 503 response with a 'Retry-after'
42+// header, the request will be transparenty retried.
43 func (client Client) dispatchRequest(request *http.Request) ([]byte, error) {
44+ // First, store the request's body into a byte[] to be able to restore it
45+ // after each request.
46+ bodyContent, err := readAndClose(request.Body)
47+ if err != nil {
48+ return nil, err
49+ }
50+ for retry := 0; retry < NumberOfRetries; retry++ {
51+ // Restore body before issuing request.
52+ newBody := ioutil.NopCloser(bytes.NewReader(bodyContent))
53+ request.Body = newBody
54+ body, err := client.dispatchSingleRequest(request)
55+ // If this is a 503 response with a non-void "Retry-After" header: wait
56+ // as instructed and retry the request.
57+ if err != nil {
58+ serverError, ok := err.(ServerError)
59+ if ok && serverError.StatusCode == http.StatusServiceUnavailable {
60+ retry_time_int, errConv := strconv.Atoi(serverError.Header.Get(RetryAfterHeaderName))
61+ if errConv == nil {
62+ select {
63+ case <-time.After(time.Duration(retry_time_int) * time.Second):
64+ }
65+ continue
66+ }
67+ }
68+ }
69+ return body, err
70+ }
71+ // Restore body before issuing request.
72+ newBody := ioutil.NopCloser(bytes.NewReader(bodyContent))
73+ request.Body = newBody
74+ return client.dispatchSingleRequest(request)
75+}
76+
77+func (client Client) dispatchSingleRequest(request *http.Request) ([]byte, error) {
78 client.Signer.OAuthSign(request)
79 httpClient := http.Client{}
80 // See https://code.google.com/p/go/issues/detail?id=4677
81@@ -62,7 +109,7 @@
82 }
83 if response.StatusCode < 200 || response.StatusCode > 299 {
84 msg := fmt.Errorf("gomaasapi: got error back from server: %v (%v)", response.Status, string(body))
85- return body, ServerError{error: msg, StatusCode: response.StatusCode}
86+ return body, ServerError{error: msg, StatusCode: response.StatusCode, Header: response.Header}
87 }
88 return body, nil
89 }
90
91=== modified file 'client_test.go'
92--- client_test.go 2013-09-02 12:18:41 +0000
93+++ client_test.go 2014-10-28 08:33:05 +0000
94@@ -50,6 +50,56 @@
95 c.Check(string(result), Equals, expectedResult)
96 }
97
98+func (suite *ClientSuite) TestClientdispatchRequestRetries503(c *C) {
99+ URI := "/some/url/?param1=test"
100+ server := newFlakyServer(URI, 503, NumberOfRetries)
101+ defer server.Close()
102+ client, err := NewAnonymousClient(server.URL, "1.0")
103+ c.Assert(err, IsNil)
104+ content := "content"
105+ request, err := http.NewRequest("GET", server.URL+URI, ioutil.NopCloser(strings.NewReader(content)))
106+
107+ _, err = client.dispatchRequest(request)
108+
109+ c.Check(err, IsNil)
110+ c.Check(*server.nbRequests, Equals, NumberOfRetries+1)
111+ expectedRequestsContent := make([][]byte, NumberOfRetries+1)
112+ for i := 0; i < NumberOfRetries+1; i++ {
113+ expectedRequestsContent[i] = []byte(content)
114+ }
115+ c.Check(*server.requests, DeepEquals, expectedRequestsContent)
116+}
117+
118+func (suite *ClientSuite) TestClientdispatchRequestDoesntRetry200(c *C) {
119+ URI := "/some/url/?param1=test"
120+ server := newFlakyServer(URI, 200, 10)
121+ defer server.Close()
122+ client, err := NewAnonymousClient(server.URL, "1.0")
123+ c.Assert(err, IsNil)
124+
125+ request, err := http.NewRequest("GET", server.URL+URI, nil)
126+
127+ _, err = client.dispatchRequest(request)
128+
129+ c.Check(err, IsNil)
130+ c.Check(*server.nbRequests, Equals, 1)
131+}
132+
133+func (suite *ClientSuite) TestClientdispatchRequestRetriesIsLimited(c *C) {
134+ URI := "/some/url/?param1=test"
135+ // Make the server return 503 responses NumberOfRetries + 1 times.
136+ server := newFlakyServer(URI, 503, NumberOfRetries+1)
137+ defer server.Close()
138+ client, err := NewAnonymousClient(server.URL, "1.0")
139+ c.Assert(err, IsNil)
140+ request, err := http.NewRequest("GET", server.URL+URI, nil)
141+
142+ _, err = client.dispatchRequest(request)
143+
144+ c.Check(*server.nbRequests, Equals, NumberOfRetries+1)
145+ c.Check(err.(ServerError).StatusCode, Equals, 503)
146+}
147+
148 func (suite *ClientSuite) TestClientDispatchRequestReturnsNonServerError(c *C) {
149 client, err := NewAnonymousClient("/foo", "1.0")
150 c.Assert(err, IsNil)
151
152=== modified file 'testing.go'
153--- testing.go 2013-07-30 08:11:08 +0000
154+++ testing.go 2014-10-28 08:33:05 +0000
155@@ -43,3 +43,40 @@
156 server := httptest.NewServer(http.HandlerFunc(handler))
157 return &singleServingServer{server, &requestContent, &requestHeader}
158 }
159+
160+type flakyServer struct {
161+ *httptest.Server
162+ nbRequests *int
163+ requests *[][]byte
164+}
165+
166+// newFlakyServer creates a "flaky" test http server which will
167+// return `nbFlakyResponses` responses with the given code and then a 200 response.
168+func newFlakyServer(uri string, code int, nbFlakyResponses int) *flakyServer {
169+ nbRequests := 0
170+ requests := make([][]byte, nbFlakyResponses+1)
171+ handler := func(writer http.ResponseWriter, request *http.Request) {
172+ nbRequests += 1
173+ body, err := readAndClose(request.Body)
174+ if err != nil {
175+ panic(err)
176+ }
177+ requests[nbRequests-1] = body
178+ if request.URL.String() != uri {
179+ errorMsg := fmt.Sprintf("Error 404: page not found (expected '%v', got '%v').", uri, request.URL.String())
180+ http.Error(writer, errorMsg, http.StatusNotFound)
181+ } else if nbRequests <= nbFlakyResponses {
182+ if code == http.StatusServiceUnavailable {
183+ writer.Header().Set("Retry-After", "0")
184+ }
185+ writer.WriteHeader(code)
186+ fmt.Fprint(writer, "flaky")
187+ } else {
188+ writer.WriteHeader(http.StatusOK)
189+ fmt.Fprint(writer, "ok")
190+ }
191+
192+ }
193+ server := httptest.NewServer(http.HandlerFunc(handler))
194+ return &flakyServer{server, &nbRequests, &requests}
195+}

Subscribers

People subscribed via source and target branches

to all changes: