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

[Raphaël Badin]

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

This works in tandem with https://code.launchpad.net/~rvb/maas/bug-1384001-redux/+merge/239722.

[lp:~rvb/gomaasapi/gomaasapi-bug-1384001]

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)]

My eyes are bleeding, but otherwise it seems okay.

review: Approve

[lp:~rvb/gomaasapi/gomaasapi-bug-1384001]

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)]

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

[Julian Edwards (julian-edwards)]

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

[Andrew Wilkins (axwalk)]

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.

[lp:~rvb/gomaasapi/gomaasapi-bug-1384001]

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

Restore body before issuing request.

[Ian Booth (wallyworld)]

@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.

=== 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 @@
 	"mime/multipart"
 	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
+	"time"
+)
+
+const (
+	// Number of retries performed when the server returns a 503
+	// response with a 'Retry-after' header.  A request will be issued
+	// at most NumberOfRetries + 1 times.
+	NumberOfRetries = 4
+
+	RetryAfterHeaderName = "Retry-After"
 )
 
 // Client represents a way to communicating with a MAAS API instance.
@@ -23,10 +34,11 @@
 
 // ServerError is an http error (or at least, a non-2xx result) received from
 // the server.  It contains the numerical HTTP status code as well as an error
-// string.
+// string and the response's headers.
 type ServerError struct {
 	error
 	StatusCode int
+	Header     http.Header
 }
 
 // readAndClose reads and closes the given ReadCloser.
@@ -44,8 +56,43 @@
 // Client-side errors will return an empty response and a non-nil error.  For
 // server-side errors however (i.e. responses with a non 2XX status code), the
 // returned error will be ServerError and the returned body will reflect the
-// server's response.
+// server's response.  If the server returns a 503 response with a 'Retry-after'
+// header, the request will be transparenty retried.
 func (client Client) dispatchRequest(request *http.Request) ([]byte, error) {
+	// First, store the request's body into a byte[] to be able to restore it
+	// after each request.
+	bodyContent, err := readAndClose(request.Body)
+	if err != nil {
+		return nil, err
+	}
+	for retry := 0; retry < NumberOfRetries; retry++ {
+		// Restore body before issuing request.
+		newBody := ioutil.NopCloser(bytes.NewReader(bodyContent))
+		request.Body = newBody
+		body, err := client.dispatchSingleRequest(request)
+		// If this is a 503 response with a non-void "Retry-After" header: wait
+		// as instructed and retry the request.
+		if err != nil {
+			serverError, ok := err.(ServerError)
+			if ok && serverError.StatusCode == http.StatusServiceUnavailable {
+				retry_time_int, errConv := strconv.Atoi(serverError.Header.Get(RetryAfterHeaderName))
+				if errConv == nil {
+					select {
+					case <-time.After(time.Duration(retry_time_int) * time.Second):
+					}
+					continue
+				}
+			}
+		}
+		return body, err
+	}
+	// Restore body before issuing request.
+	newBody := ioutil.NopCloser(bytes.NewReader(bodyContent))
+	request.Body = newBody
+	return client.dispatchSingleRequest(request)
+}
+
+func (client Client) dispatchSingleRequest(request *http.Request) ([]byte, error) {
 	client.Signer.OAuthSign(request)
 	httpClient := http.Client{}
 	// See https://code.google.com/p/go/issues/detail?id=4677
@@ -62,7 +109,7 @@
 	}
 	if response.StatusCode < 200 || response.StatusCode > 299 {
 		msg := fmt.Errorf("gomaasapi: got error back from server: %v (%v)", response.Status, string(body))
-		return body, ServerError{error: msg, StatusCode: response.StatusCode}
+		return body, ServerError{error: msg, StatusCode: response.StatusCode, Header: response.Header}
 	}
 	return body, nil
 }

=== 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 @@
 	c.Check(string(result), Equals, expectedResult)
 }
 
+func (suite *ClientSuite) TestClientdispatchRequestRetries503(c *C) {
+	URI := "/some/url/?param1=test"
+	server := newFlakyServer(URI, 503, NumberOfRetries)
+	defer server.Close()
+	client, err := NewAnonymousClient(server.URL, "1.0")
+	c.Assert(err, IsNil)
+	content := "content"
+	request, err := http.NewRequest("GET", server.URL+URI, ioutil.NopCloser(strings.NewReader(content)))
+
+	_, err = client.dispatchRequest(request)
+
+	c.Check(err, IsNil)
+	c.Check(*server.nbRequests, Equals, NumberOfRetries+1)
+	expectedRequestsContent := make([][]byte, NumberOfRetries+1)
+	for i := 0; i < NumberOfRetries+1; i++ {
+		expectedRequestsContent[i] = []byte(content)
+	}
+	c.Check(*server.requests, DeepEquals, expectedRequestsContent)
+}
+
+func (suite *ClientSuite) TestClientdispatchRequestDoesntRetry200(c *C) {
+	URI := "/some/url/?param1=test"
+	server := newFlakyServer(URI, 200, 10)
+	defer server.Close()
+	client, err := NewAnonymousClient(server.URL, "1.0")
+	c.Assert(err, IsNil)
+
+	request, err := http.NewRequest("GET", server.URL+URI, nil)
+
+	_, err = client.dispatchRequest(request)
+
+	c.Check(err, IsNil)
+	c.Check(*server.nbRequests, Equals, 1)
+}
+
+func (suite *ClientSuite) TestClientdispatchRequestRetriesIsLimited(c *C) {
+	URI := "/some/url/?param1=test"
+	// Make the server return 503 responses NumberOfRetries + 1 times.
+	server := newFlakyServer(URI, 503, NumberOfRetries+1)
+	defer server.Close()
+	client, err := NewAnonymousClient(server.URL, "1.0")
+	c.Assert(err, IsNil)
+	request, err := http.NewRequest("GET", server.URL+URI, nil)
+
+	_, err = client.dispatchRequest(request)
+
+	c.Check(*server.nbRequests, Equals, NumberOfRetries+1)
+	c.Check(err.(ServerError).StatusCode, Equals, 503)
+}
+
 func (suite *ClientSuite) TestClientDispatchRequestReturnsNonServerError(c *C) {
 	client, err := NewAnonymousClient("/foo", "1.0")
 	c.Assert(err, IsNil)

=== 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 @@
 	server := httptest.NewServer(http.HandlerFunc(handler))
 	return &singleServingServer{server, &requestContent, &requestHeader}
 }
+
+type flakyServer struct {
+	*httptest.Server
+	nbRequests *int
+	requests   *[][]byte
+}
+
+// newFlakyServer creates a "flaky" test http server which will
+// return `nbFlakyResponses` responses with the given code and then a 200 response.
+func newFlakyServer(uri string, code int, nbFlakyResponses int) *flakyServer {
+	nbRequests := 0
+	requests := make([][]byte, nbFlakyResponses+1)
+	handler := func(writer http.ResponseWriter, request *http.Request) {
+		nbRequests += 1
+		body, err := readAndClose(request.Body)
+		if err != nil {
+			panic(err)
+		}
+		requests[nbRequests-1] = body
+		if request.URL.String() != uri {
+			errorMsg := fmt.Sprintf("Error 404: page not found (expected '%v', got '%v').", uri, request.URL.String())
+			http.Error(writer, errorMsg, http.StatusNotFound)
+		} else if nbRequests <= nbFlakyResponses {
+			if code == http.StatusServiceUnavailable {
+				writer.Header().Set("Retry-After", "0")
+			}
+			writer.WriteHeader(code)
+			fmt.Fprint(writer, "flaky")
+		} else {
+			writer.WriteHeader(http.StatusOK)
+			fmt.Fprint(writer, "ok")
+		}
+
+	}
+	server := httptest.NewServer(http.HandlerFunc(handler))
+	return &flakyServer{server, &nbRequests, &requests}
+}