Return HTTP 502 from failed phabricator heartbeat checks

Author: mars-f

landoapi/dockerflow.py

@@ -7,9 +7,16 @@
 """
 
 import json
+import logging
 
+import requests
 from flask import Blueprint, current_app, jsonify
 
+from landoapi.phabricator_client import PhabricatorClient, \
+    PhabricatorAPIException
+
+logger = logging.getLogger(__name__)
+
 dockerflow = Blueprint('dockerflow', __name__)
 
 
@@ -21,8 +28,16 @@ def heartbeat():
     and return a 200 iff those services and the app itself are
     performing normally. Return a 5XX if something goes wrong.
     """
-    # TODO check backing services
-    return '', 200
+    phab = PhabricatorClient(api_key='')
+    try:
+        phab.check_connection()
+    except PhabricatorAPIException as exc:
+        logger.warning(
+            'heartbeat: problem, Phabricator API connection', exc_info=exc
+        )
+        return 'heartbeat: problem', 502
+    logger.info('heartbeat: ok, all services are up')
+    return 'heartbeat: ok', 200
 
 
 @dockerflow.route('/__lbheartbeat__')

landoapi/phabricator_client.py

@@ -2,11 +2,14 @@
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
+import logging
 import os
 import requests
 
 from landoapi.utils import extract_rawdiff_id_from_uri
 
+logger = logging.getLogger(__name__)
+
 
 class PhabricatorClient:
     """ A class to interface with Phabricator's Conduit API.
@@ -145,6 +148,21 @@ def get_revision_repo(self, revision):
         """
         return self.get_repo(revision['repositoryPHID'])
 
+    def check_connection(self):
+        """Test the Phabricator API connection with conduit.ping.
+
+        Will return success iff the response has a HTTP status code of 200, the
+        JSON response is a well-formed Phabricator API response, and if there
+        is no connection error (like a hostname lookup error or timeout).
+
+        Raises a PhabricatorAPIException on error.
+        """
+        try:
+            self._GET('/conduit.ping')
+        except (requests.ConnectionError, requests.Timeout) as exc:
+            logging.debug("error calling 'conduit.ping': %s", exc)
+            raise PhabricatorAPIException from exc
+
     def _request(self, url, data=None, params=None, method='GET'):
         data = data if data else {}
         data['api.token'] = self.api_key

tests/conftest.py

@@ -3,6 +3,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 import json
 
+import logging
 import pytest
 import requests_mock
 
@@ -58,7 +59,13 @@ def init_app(self, app):
 
 
 @pytest.fixture
-def app(versionfile, docker_env_vars, disable_migrations):
+def disable_log_output():
+    """Disable Python standard logging output to the console."""
+    logging.disable(logging.CRITICAL)
+
+
+@pytest.fixture
+def app(versionfile, docker_env_vars, disable_migrations, disable_log_output):
     """Needed for pytest-flask."""
     app = create_app(versionfile.strpath)
     return app.app

tests/test_dockerflow.py

@@ -3,6 +3,12 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import json
+import requests
+
+import requests_mock
+
+from tests.canned_responses.phabricator.revisions import CANNED_EMPTY_RESULT
+from tests.utils import phab_url
 
 
 def test_dockerflow_lb_endpoint_returns_200(client):
@@ -18,3 +24,30 @@ def test_dockerflow_version_endpoint_response(client):
 def test_dockerflow_version_matches_disk_contents(client, versionfile):
     response = client.get('/__version__')
     assert response.json == json.load(versionfile.open())
+
+
+def test_heartbeat_returns_200_if_phabricator_api_is_up(client):
+    json_response = CANNED_EMPTY_RESULT.copy()
+    with requests_mock.mock() as m:
+        m.get(phab_url('conduit.ping'), status_code=200, json=json_response)
+
+        response = client.get('/__heartbeat__')
+
+        assert m.called
+        assert response.status_code == 200
+
+
+def test_heartbeat_returns_http_502_if_phabricator_ping_returns_error(client):
+    error_json = {
+        "result": None,
+        "error_code": "ERR-CONDUIT-CORE",
+        "error_info": "BOOM"
+    }
+
+    with requests_mock.mock() as m:
+        m.get(phab_url('conduit.ping'), status_code=500, json=error_json)
+
+        response = client.get('/__heartbeat__')
+
+        assert m.called
+        assert response.status_code == 502

tests/test_phabricator_client.py

@@ -4,7 +4,9 @@
 """
 Tests for the PhabricatorClient
 """
+
 import pytest
+import requests
 import requests_mock
 
 from landoapi.phabricator_client import PhabricatorClient, \
@@ -122,6 +124,57 @@ def test_get_latest_patch_for_revision(phabfactory):
     assert returned_patch == patch
 
 
+def test_check_connection_success():
+    phab = PhabricatorClient(api_key='api-key')
+    success_json = CANNED_EMPTY_RESULT.copy()
+    with requests_mock.mock() as m:
+        m.get(phab_url('conduit.ping'), status_code=200, json=success_json)
+        phab.check_connection()
+        assert m.called
+
+
+def test_raise_exception_if_ping_encounters_connection_error():
+    phab = PhabricatorClient(api_key='api-key')
+    with requests_mock.mock() as m:
+        # Test with the generic ConnectionError, which is a superclass for
+        # other connection error types.
+        m.get(phab_url('conduit.ping'), exc=requests.ConnectionError)
+
+        with pytest.raises(PhabricatorAPIException):
+            phab.check_connection()
+        assert m.called
+
+
+def test_raise_exception_if_api_ping_times_out():
+    phab = PhabricatorClient(api_key='api-key')
+    with requests_mock.mock() as m:
+        # Test with the generic Timeout exception, which all other timeout
+        # exceptions derive from.
+        m.get(phab_url('conduit.ping'), exc=requests.Timeout)
+
+        with pytest.raises(PhabricatorAPIException):
+            phab.check_connection()
+        assert m.called
+
+
+def test_raise_exception_if_api_returns_error_json_response():
+    phab = PhabricatorClient(api_key='api-key')
+    error_json = {
+        "result": None,
+        "error_code": "ERR-CONDUIT-CORE",
+        "error_info": "BOOM"
+    }
+
+    with requests_mock.mock() as m:
+        # Test with the generic Timeout exception, which all other timeout
+        # exceptions derive from.
+        m.get(phab_url('conduit.ping'), status_code=500, json=error_json)
+
+        with pytest.raises(PhabricatorAPIException):
+            phab.check_connection()
+        assert m.called
+
+
 def test_phabricator_exception():
     """ Ensures that the PhabricatorClient converts JSON errors from Phabricator
     into proper exceptions with the error_code and error_message in tact.