Repository: praekeltfoundation/marathon-acme

Issue:
Only fail when all load balancers can't be reached
We should allow for one/some of the load balancers to be down and carry on when others are happy.

Discussion:

Patch:
diff --git a/marathon_acme/clients.py b/marathon_acme/clients.py
index 8427381..4987d31 100644
--- a/marathon_acme/clients.py
+++ b/marathon_acme/clients.py
@@ -3,7 +3,7 @@ import json
 
 from requests.exceptions import HTTPError
 from treq.client import HTTPClient as treq_HTTPClient
-from twisted.internet.defer import gatherResults
+from twisted.internet.defer import DeferredList
 from twisted.logger import Logger, LogLevel
 from twisted.web.http import OK
 from uritools import uricompose, uridecode, urisplit
@@ -331,18 +331,63 @@ class MarathonLbClient(HTTPClient):
     def __init__(self, endpoints, *args, **kwargs):
         """
         :param endpoints:
-        The list of marathon-lb endpoints. All marathon-lb endpoints will be
-        called at once for any request.
+            The list of marathon-lb endpoints. All marathon-lb endpoints will
+            be called at once for any request.
         """
         super(MarathonLbClient, self).__init__(*args, **kwargs)
         self.endpoints = endpoints
 
     def request(self, *args, **kwargs):
-        requests = []
-        for endpoint in self.endpoints:
-            requests.append(super(MarathonLbClient, self).request(
-                *args, url=endpoint, **kwargs))
-        return gatherResults(requests)
+        return (
+            DeferredList(
+                [self._request(e, *args, **kwargs) for e in self.endpoints],
+                consumeErrors=True)
+            .addCallback(self._check_request_results))
+
+    def _request(self, endpoint, *args, **kwargs):
+        """
+        Perform a request to a specific endpoint. Raise an error if the status
+        code indicates a client or server error.
+        """
+        kwargs['url'] = endpoint
+        return (super(MarathonLbClient, self).request(*args, **kwargs)
+                .addCallback(raise_for_status))
+
+    def _check_request_results(self, results):
+        """
+        Check the result of each request that we made. If a failure occurred,
+        but some requests succeeded, log and count the failures. If all
+        requests failed, raise an error.
+
+        :return:
+            The list of responses, with a None value for any requests that
+            failed.
+        """
+        responses = []
+        failed_endpoints = []
+        for index, result_tuple in enumerate(results):
+            success, result = result_tuple
+            if success:
+                responses.append(result)
+            else:
+                endpoint = self.endpoints[index]
+                self.log.failure(
+                    'Failed to make a request to a marathon-lb instance: '
+                    '{endpoint}', result, LogLevel.error, endpoint=endpoint)
+                responses.append(None)
+                failed_endpoints.append(endpoint)
+
+        if len(failed_endpoints) == len(self.endpoints):
+            raise RuntimeError(
+                'Failed to make a request to all marathon-lb instances')
+
+        if failed_endpoints:
+            self.log.error(
+                'Failed to make a request to {x}/{y} marathon-lb instances: '
+                '{endpoints}', x=len(failed_endpoints), y=len(self.endpoints),
+                endpoints=failed_endpoints)
+
+        return responses
 
     def mlb_signal_hup(self):
         """


Test patch:
diff --git a/marathon_acme/tests/test_clients.py b/marathon_acme/tests/test_clients.py
index 9e95f45..6eb67f9 100644
--- a/marathon_acme/tests/test_clients.py
+++ b/marathon_acme/tests/test_clients.py
@@ -2,7 +2,8 @@ import json
 
 from testtools import ExpectedException, TestCase
 from testtools.assertions import assert_that
-from testtools.matchers import Equals, Is, IsInstance, MatchesStructure
+from testtools.matchers import (
+    Equals, Is, IsInstance, HasLength, MatchesStructure)
 from testtools.twistedsupport import (
     AsynchronousDeferredRunTest, failed, flush_logged_errors)
 from treq.client import HTTPClient as treq_HTTPClient
@@ -824,6 +825,97 @@ class TestMarathonLbClient(TestHTTPClientBase):
         return MarathonLbClient(['http://lb1:9090', 'http://lb2:9090'],
                                 client=client)
 
+    @inlineCallbacks
+    def test_request_success(self):
+        """
+        When a request is made, it is made to all marathon-lb instances and
+        the responses are returned.
+        """
+        d = self.cleanup_d(self.client.request('GET', path='/my-path'))
+
+        for lb in ['lb1', 'lb2']:
+            request = yield self.requests.get()
+            self.assertThat(request, HasRequestProperties(
+                method='GET', url='http://%s:9090/my-path' % (lb,)))
+
+            request.setResponseCode(200)
+            request.finish()
+
+        responses = yield d
+        self.assertThat(responses, HasLength(2))
+        for response in responses:
+            self.assertThat(response.code, Equals(200))
+
+    @inlineCallbacks
+    def test_request_partial_failure(self):
+        """
+        When a request is made and an error status code is returned from some
+        (but not all) of the matathon-lb instances, then the request returns
+        the list of responses with a None value for the unhappy request.
+        """
+        d = self.cleanup_d(self.client.request('GET', path='/my-path'))
+
+        lb1_request = yield self.requests.get()
+        self.assertThat(lb1_request, HasRequestProperties(
+            method='GET', url='http://lb1:9090/my-path'))
+
+        lb2_request = yield self.requests.get()
+        self.assertThat(lb2_request, HasRequestProperties(
+            method='GET', url='http://lb2:9090/my-path'))
+
+        # Fail the first one
+        lb1_request.setResponseCode(500)
+        lb1_request.setHeader('content-type', 'text/plain')
+        lb1_request.write(b'Internal Server Error')
+        lb1_request.finish()
+
+        # ...but succeed the second
+        lb2_request.setResponseCode(200)
+        lb2_request.setHeader('content-type', 'text/plain')
+        lb2_request.write(b'Yes, I work')
+        lb2_request.finish()
+
+        responses = yield d
+        self.assertThat(responses, HasLength(2))
+        lb1_response, lb2_response = responses
+
+        self.assertThat(lb1_response, Is(None))
+        self.assertThat(lb2_response, MatchesStructure(
+            code=Equals(200),
+            headers=HasHeader('content-type', ['text/plain'])
+        ))
+
+        lb2_response_content = yield lb2_response.content()
+        self.assertThat(lb2_response_content, Equals(b'Yes, I work'))
+
+        flush_logged_errors(HTTPError)
+
+    @inlineCallbacks
+    def test_request_failure(self):
+        """
+        When the requests to all the marathon-lb instances have a bad status
+        code then an error should be raised.
+        """
+        d = self.cleanup_d(self.client.request('GET', path='/my-path'))
+
+        for lb in ['lb1', 'lb2']:
+            request = yield self.requests.get()
+            self.assertThat(request, HasRequestProperties(
+                method='GET', url='http://%s:9090/my-path' % (lb,)))
+
+            request.setResponseCode(500)
+            request.setHeader('content-type', 'text/plain')
+            request.write(b'Internal Server Error')
+            request.finish()
+
+        yield wait0()
+        self.assertThat(d, failed(WithErrorTypeAndMessage(
+            RuntimeError,
+            'Failed to make a request to all marathon-lb instances'
+        )))
+
+        flush_logged_errors(HTTPError)
+
     @inlineCallbacks
     def test_mlb_signal_hup(self):
         """