launchpad-reviewers team mailing list archive

[Colin Watson <mp+430774@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:remove-haproxy-status-view into launchpad:master.

Commit message:
Remove /+haproxy view

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/430774

Our haproxy configuration has used Talisker's `/_status/ping` instead for a while.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:remove-haproxy-status-view into launchpad:master.

diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 82fdc73..1de9763 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -917,13 +917,6 @@ public_https: True
 # datatype: int
 port: 11371
 
-[haproxy_status_view]
-# Configuration for the +haproxy monitoring URL
-
-# Result code to return when the app server is going down.
-# datatype: int
-going_down_status: 500
-
 
 [karmacacheupdater]
 # The database user which will be used by this process.
diff --git a/lib/lp/services/database/policy.py b/lib/lp/services/database/policy.py
index 855933e..97ef62d 100644
--- a/lib/lp/services/database/policy.py
+++ b/lib/lp/services/database/policy.py
@@ -225,7 +225,7 @@ def LaunchpadDatabasePolicyFactory(request):
     # to sniff the request this way.  Even though PATH_INFO is always
     # present in real requests, we need to tread carefully (``get``) because
     # of test requests in our automated tests.
-    if request.get("PATH_INFO") in ["/+opstats", "/+haproxy"]:
+    if request.get("PATH_INFO") == "/+opstats":
         return DatabaseBlockedPolicy(request)
     else:
         return LaunchpadDatabasePolicy(request)
diff --git a/lib/lp/services/webapp/configure.zcml b/lib/lp/services/webapp/configure.zcml
index 72a7ba7..702afab 100644
--- a/lib/lp/services/webapp/configure.zcml
+++ b/lib/lp/services/webapp/configure.zcml
@@ -381,14 +381,6 @@
           interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
     </class>
 
-    <!-- Registrations to support +haproxy status url. -->
-    <browser:page
-        for="lp.services.webapp.interfaces.ILaunchpadRoot"
-        name="+haproxy"
-        permission="zope.Public"
-        class="lp.services.webapp.haproxy.HAProxyStatusView"
-        />
-
     <!-- Support Talisker's /_status/check URL. -->
     <browser:page
         for="lp.services.webapp.interfaces.ILaunchpadRoot"
diff --git a/lib/lp/services/webapp/haproxy.py b/lib/lp/services/webapp/haproxy.py
deleted file mode 100644
index 30399e7..0000000
--- a/lib/lp/services/webapp/haproxy.py
+++ /dev/null
@@ -1,70 +0,0 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Implementation of the HAProxy probe URL."""
-
-__all__ = [
-    "HAProxyStatusView",
-    "set_going_down_flag",
-    "switch_going_down_flag",
-]
-
-from lp.services.config import config
-
-# This is the global flag, when this is True, the HAProxy view
-# will return 500, it returns 200 otherwise.
-going_down_flag = False
-
-
-def set_going_down_flag(state):
-    """Sets the going_down_flag to state"""
-    global going_down_flag
-    going_down_flag = state
-
-
-def switch_going_down_flag():
-    """Switch the going down flag.
-
-    This is is registered as a signal handler for HUP.
-    """
-    global going_down_flag
-    going_down_flag = not going_down_flag
-
-
-class HAProxyStatusView:
-    """
-    View implementing the HAProxy probe URL.
-
-    HAProxy doesn't support programmatically taking servers in and our of
-    rotation. It does however uses a probe URL that it scans regularly to see
-    if the server is still alive. The /+haproxy is that URL for us.
-
-    If it times out or returns a non-200 status, it will take the server out
-    of rotation, until the probe works again.
-
-    This allow us to send a signal (HUP) to the app servers when we want to
-    restart them. The probe URL will then return 500 and the app server will
-    be taken out of rotation. Once HAProxy reports that all connections to the
-    app servers are finished, we can restart the server safely.
-
-    The returned result code when the server is going down can be configured
-    through the haproxy_status_view.going_down_status configuration variable.
-    It defaults to 500 (as set in lib/lp/services/config/schema-lazr.conf).
-    """
-
-    def __init__(self, context, request):
-        self.context = context
-        self.request = request
-
-    def __call__(self):
-        """Return 200 or going down status depending on the global flag."""
-        global going_down_flag
-
-        if going_down_flag:
-            self.request.response.setStatus(
-                config.haproxy_status_view.going_down_status
-            )
-            return "May day! May day! I'm going down. Stop the flood gate."
-        else:
-            self.request.response.setStatus(200)
-            return "Everything is groovy. Keep them coming!"
diff --git a/lib/lp/services/webapp/publication.py b/lib/lp/services/webapp/publication.py
index 2e5332d..bbe8b83 100644
--- a/lib/lp/services/webapp/publication.py
+++ b/lib/lp/services/webapp/publication.py
@@ -272,14 +272,14 @@ class LaunchpadBrowserPublication(
         """
         auth_utility = getUtility(IPlacelessAuthUtility)
         principal = None
-        # +opstats and +haproxy are status URLs that must not query the DB at
-        # all.  This is enforced by webapp/dbpolicy.py. If the request is for
-        # one of those two pages, don't even try to authenticate, because it
-        # may fail.  We haven't traversed yet, so we have to sniff the request
-        # this way.  Even though PATH_INFO is always present in real requests,
-        # we need to tread carefully (``get``) because of test requests in our
+        # +opstats is a status URL that must not query the DB at all.  This
+        # is enforced by lp.services.database.policy.  If the request is for
+        # this page, don't even try to authenticate, because it may fail.
+        # We haven't traversed yet, so we have to sniff the request this
+        # way.  Even though PATH_INFO is always present in real requests, we
+        # need to tread carefully (``get``) because of test requests in our
         # automated tests.
-        if request.get("PATH_INFO") not in ["/+opstats", "/+haproxy"]:
+        if request.get("PATH_INFO") != "/+opstats":
             principal = auth_utility.authenticate(request)
         if principal is not None:
             assert principal.person is not None
@@ -421,7 +421,6 @@ class LaunchpadBrowserPublication(
         if pageid in (
             "RootObject:OpStats",
             "RootObject:+opstats",
-            "RootObject:+haproxy",
         ):
             request.features = NullFeatureController()
             features.install_feature_controller(request.features)
diff --git a/lib/lp/services/webapp/tests/test_haproxy.py b/lib/lp/services/webapp/tests/test_haproxy.py
deleted file mode 100644
index abc135a..0000000
--- a/lib/lp/services/webapp/tests/test_haproxy.py
+++ /dev/null
@@ -1,74 +0,0 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Test the haproxy integration view."""
-
-__all__ = []
-
-from textwrap import dedent
-
-from lp.services.config import config
-from lp.services.database.policy import (
-    DatabaseBlockedPolicy,
-    LaunchpadDatabasePolicyFactory,
-)
-from lp.services.webapp import haproxy
-from lp.services.webapp.servers import LaunchpadTestRequest
-from lp.testing import TestCase
-from lp.testing.layers import FunctionalLayer
-from lp.testing.pages import http
-
-
-class HAProxyIntegrationTest(TestCase):
-    layer = FunctionalLayer
-
-    def setUp(self):
-        TestCase.setUp(self)
-        self.original_flag = haproxy.going_down_flag
-        self.addCleanup(haproxy.set_going_down_flag, self.original_flag)
-
-    def test_HAProxyStatusView_all_good_returns_200(self):
-        result = http("GET /+haproxy HTTP/1.0", handle_errors=False)
-        self.assertEqual(200, result.getStatus())
-
-    def test_authenticated_HAProxyStatusView_works(self):
-        # We don't use authenticated requests, but this keeps us from
-        # generating oopses.
-        result = http(
-            "GET /+haproxy HTTP/1.0\n"
-            "Authorization: Basic Zm9vLmJhckBjYW5vbmljYWwuY29tOnRlc3Q=\n",
-            handle_errors=False,
-        )
-        self.assertEqual(200, result.getStatus())
-
-    def test_HAProxyStatusView_going_down_returns_500(self):
-        haproxy.set_going_down_flag(True)
-        result = http("GET /+haproxy HTTP/1.0", handle_errors=False)
-        self.assertEqual(500, result.getStatus())
-
-    def test_haproxy_url_uses_DatabaseBlocked_policy(self):
-        request = LaunchpadTestRequest(environ={"PATH_INFO": "/+haproxy"})
-        policy = LaunchpadDatabasePolicyFactory(request)
-        self.assertIsInstance(policy, DatabaseBlockedPolicy)
-
-    def test_switch_going_down_flag(self):
-        haproxy.set_going_down_flag(True)
-        haproxy.switch_going_down_flag()
-        self.assertEqual(False, haproxy.going_down_flag)
-        haproxy.switch_going_down_flag()
-        self.assertEqual(True, haproxy.going_down_flag)
-
-    def test_HAProxyStatusView_status_code_is_configurable(self):
-        config.push(
-            "change_haproxy_status_code",
-            dedent(
-                """
-            [haproxy_status_view]
-            going_down_status: 499
-            """
-            ),
-        )
-        self.addCleanup(config.pop, "change_haproxy_status_code")
-        haproxy.set_going_down_flag(True)
-        result = http("GET /+haproxy HTTP/1.0", handle_errors=False)
-        self.assertEqual(499, result.getStatus())