Merge lp:~flacoste/launchpad/bug-721324 into lp:launchpad

[Francis J. Lacoste]

[r=bac,benji][bug=721324] Made the haproxy going down status configurable.

This branch makes the result code to return when the app server is going down
configurable through a config variable.

That's for the +haproxy monitoring URL. We were returning 500, but it's
possible that 404 results in better behavior from HAProxy. We have to test to
make sure, so I'm making this result code configurable.

I didn't use a feature flag to control this because we don't want the +haproxy
URL to hit the database. (It gets a special DB policy and a
NullFeatureController.)

Let me know if you have any questions.

[Benji York (benji)]

This branch looks good. Approved (but I'm doing mentored reviews so
Brad will review my review before you get a full approval.)

I only have two small (ignorable) thoughts about the branch:

The comment about the default value being 500 in the docstring could
easily get out of sync with reality. Maybe we should point to the place
the default is configured instead.

The second is that perhaps the test should use an invalid status code
instead. I worry that someone might introduce a bug that would cause a
404 to be generated when requesting /+haproxy instead of the configured
value and the test wouldn't fail. Perhaps the test should configure the
status to be 999 just to be sure we're getting back the status we
configured and not one someone else is setting.

review: Approve (code*)

[Robert Collins (lifeless)]

I like your suggestion about the docstring Benji. There is already a test that 500 is returned by default - so the page is being tested with both 404 and 500 responses, so I think your second suggestion would be redundant.

[Francis J. Lacoste (flacoste)]

Thanks for the review.

I've pushed modifications for both comments and will wait for Brad's approval.

[Brad Crittenden (bac)]

As Robert said, the docstring comment was spot on. I notice there is another docstring for the __call__ method that mentions returning 200 or 500 and I think it should be updated similarly.

review: Approve (code)

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-02-18 00:14:02 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-02-21 18:25:28 +0000
@@ -938,6 +938,13 @@
 # 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
+
 [initialisedistroseries]
 dbuser: initialisedistroseries
 

=== modified file 'lib/canonical/launchpad/webapp/haproxy.py'
--- lib/canonical/launchpad/webapp/haproxy.py	2010-12-17 20:58:27 +0000
+++ lib/canonical/launchpad/webapp/haproxy.py	2011-02-21 18:25:28 +0000
@@ -11,6 +11,8 @@
     'switch_going_down_flag',
     ]
 
+from canonical.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
@@ -46,6 +48,10 @@
     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/canonical/config/schema-lazr.conf).
     """
 
     def __init__(self, context, request):
@@ -53,10 +59,12 @@
         self.request = request
 
     def __call__(self):
-        """Return 200 or 500 depending on the global flag."""
+        """Return 200 or going down status depending on the global flag."""
         global going_down_flag
+
         if going_down_flag:
-            self.request.response.setStatus(500)
+            self.request.response.setStatus(
+                config.haproxy_status_view.going_down_status)
             return u"May day! May day! I'm going down. Stop the flood gate."
         else:
             self.request.response.setStatus(200)

=== modified file 'lib/canonical/launchpad/webapp/tests/test_haproxy.py'
--- lib/canonical/launchpad/webapp/tests/test_haproxy.py	2010-12-17 20:58:27 +0000
+++ lib/canonical/launchpad/webapp/tests/test_haproxy.py	2011-02-21 18:25:28 +0000
@@ -6,6 +6,9 @@
 __metaclass__ = type
 __all__ = []
 
+from textwrap import dedent
+
+from canonical.config import config
 from canonical.testing.layers import FunctionalLayer
 from canonical.launchpad.webapp import haproxy
 from canonical.launchpad.webapp.dbpolicy import (
@@ -15,6 +18,7 @@
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 
 from zope.app.testing.functional import HTTPCaller
+
 from lp.testing import TestCase
 
 
@@ -47,3 +51,14 @@
         self.assertEquals(False, haproxy.going_down_flag)
         haproxy.switch_going_down_flag()
         self.assertEquals(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 = self.http(u'GET /+haproxy HTTP/1.0', handle_errors=False)
+        self.assertEquals(499, result.getStatus())
+