software/erp5: set server timeout in haproxy to match publisher-timeout

Instead of having an hardcoded timeout that users will hit anyway even
if they increase publisher timeout, set this timeout value to be
slightly higher than publisher timeout. This way publisher-timeout can
be used to allow longer requests and it's generally more consistent.

software/erp5/test/test/test_balancer.py

@@ -170,6 +170,7 @@ class BalancerTestCase(ERP5InstanceTestCase):
         'ssl': {
             'caucase-url': cls.getManagedResource("caucase", CaucaseService).url,
         },
+        'timeout-dict': {},
         'family-path-routing-dict': {},
         'path-routing-list': [],
       }
@@ -186,18 +187,51 @@ class BalancerTestCase(ERP5InstanceTestCase):
 
 
 class SlowHTTPServer(ManagedHTTPServer):
-  """An HTTP Server which reply after 2 seconds.
+  """An HTTP Server which reply after a timeout.
+
+  Timeout is 2 seconds by default, and can be specified in the path of the URL
   """
   class RequestHandler(BaseHTTPRequestHandler):
     def do_GET(self):
       # type: () -> None
       self.send_response(200)
       self.send_header("Content-Type", "text/plain")
-      time.sleep(2)
+      timeout = 2
+      try:
+        timeout = int(self.path[1:])
+      except ValueError:
+        pass
+      time.sleep(timeout)
       self.end_headers()
       self.wfile.write(b"OK\n")
 
-    log_message = logging.getLogger(__name__ + '.SlowHandler').info
+    log_message = logging.getLogger(__name__ + '.SlowHTTPServer').info
+
+
+class TestTimeout(BalancerTestCase, CrontabMixin):
+  __partition_reference__ = 't'
+  @classmethod
+  def _getInstanceParameterDict(cls):
+    # type: () -> dict
+    parameter_dict = super(TestTimeout, cls)._getInstanceParameterDict()
+    # use a slow server instead
+    parameter_dict['dummy_http_server'] = [[cls.getManagedResource("slow_web_server", SlowHTTPServer).netloc, 1, False]]
+    # and set timeout of 1 second
+    parameter_dict['timeout-dict'] = {'default': 1}
+    return parameter_dict
+
+  def test_timeout(self):
+    # type: () -> None
+    self.assertEqual(
+      requests.get(
+          six.moves.urllib.parse.urljoin(self.default_balancer_url, '/1'),
+          verify=False).status_code,
+      requests.codes.ok)
+    self.assertEqual(
+      requests.get(
+          six.moves.urllib.parse.urljoin(self.default_balancer_url, '/5'),
+          verify=False).status_code,
+      requests.codes.gateway_timeout)
 
 
 class TestLog(BalancerTestCase, CrontabMixin):

stack/erp5/buildout.hash.cfg

@@ -74,7 +74,7 @@ md5sum = 9a7f7888ba4183c9d900e862074f3baf
 
 [template-erp5]
 filename = instance-erp5.cfg.in
-md5sum = fcecee930aa05a1a739f31b28f6e4769
+md5sum = 3d8f3a440b7423c3b947c6ea4d775c6e
 
 [template-zeo]
 filename = instance-zeo.cfg.in
@@ -90,11 +90,11 @@ md5sum = c3e3f8cd985407931b705d15bdedc8d9
 
 [template-balancer]
 filename = instance-balancer.cfg.in
-md5sum = 4491b256241cb4bbdfafb22ae95a3eba
+md5sum = d51d718ab85ae57726e6f0bf81c4bfeb
 
 [template-haproxy-cfg]
 filename = haproxy.cfg.in
-md5sum = d2d98ed3fafce764991b72371e3e09d5
+md5sum = 1645ef8990ab2b50f91a4c02f0cf8882
 
 [template-rsyslogd-cfg]
 filename = rsyslogd.cfg.in

stack/erp5/haproxy.cfg.in

@@ -48,6 +48,7 @@
  #            ( 8000, # port int
  #              'https', # proto str
  #               True, # ssl_required bool
+ #               None,  # timeout (in seconds) int | None
  #               [  # backends
  #                  '10.0.0.10:8001', # netloc str
  #                   1, # max_connection_count int
@@ -59,6 +60,7 @@
  #            ( 8002, # port int
  #              'https', # proto str
  #               False, # ssl_required bool
+ #               None,  # timeout (in seconds) int | None
  #               [  # backends
  #                  '10.0.0.10:8003', # netloc str
  #                   1, # max_connection_count int
@@ -135,7 +137,6 @@ defaults
 
   timeout connect 10s
   timeout queue 60s
-  timeout server 305s
   timeout client 305s
 
   option http-server-close
@@ -150,7 +151,7 @@ defaults
 {% set family_path_routing_dict = parameter_dict['family-path-routing-dict'] %}
 {% set path_routing_list = parameter_dict['path-routing-list'] %}
 
-{% for name, (port, _, certificate_authentication, backend_list) in sorted(six.iteritems(parameter_dict['backend-dict'])) -%}
+{% for name, (port, _, certificate_authentication, timeout, backend_list) in sorted(six.iteritems(parameter_dict['backend-dict'])) -%}
 listen family_{{ name }}
 {%-  if parameter_dict.get('ca-cert') -%}
 {%-    set ssl_auth = ' ca-file ' ~ parameter_dict['ca-cert'] ~ ' verify' ~ ( ' required' if certificate_authentication else ' optional' ) ~ ' crl-file ' ~ parameter_dict['crl'] %}
@@ -162,6 +163,15 @@ listen family_{{ name }}
   cookie SERVERID rewrite
   http-request set-header X-Balancer-Current-Cookie SERVERID
 
+{% if timeout %}
+  {#
+    Apply a slightly longer timeout than the zope timeout so that clients can see the
+    TimeoutReachedError from zope, that is a bit more informative than the 504 error
+    page from haproxy.
+  #}
+  timeout server {{ timeout + 3 }}s
+{%-  endif %}
+
   # remove X-Forwarded-For unless client presented a verified certificate
   acl client_cert_verified ssl_c_used ssl_c_verify 0
   http-request del-header X-Forwarded-For unless client_cert_verified

stack/erp5/instance-balancer.cfg.in

@@ -177,7 +177,7 @@ update-command = ${:command}
 {%   else %}
 {%     set external_scheme = 'https' -%}
 {%   endif -%}
-{%   do haproxy_dict.__setitem__(family_name, (haproxy_port, external_scheme, slapparameter_dict['ssl-authentication-dict'].get(family_name, False), zope_family_address_list)) -%}
+{%   do haproxy_dict.__setitem__(family_name, (haproxy_port, external_scheme, slapparameter_dict['ssl-authentication-dict'].get(family_name, False), slapparameter_dict['timeout-dict'].get(family_name, None), zope_family_address_list)) -%}
 {% endfor -%}
 
 [haproxy-cfg-parameter-dict]
@@ -309,7 +309,7 @@ config-port = {{ next(six.itervalues(haproxy_dict))[0] }}
 
 [{{ section('publish') }}]
 recipe = slapos.cookbook:publish.serialised
-{% for family_name, (port, scheme, _, _) in haproxy_dict.items() -%}
+{% for family_name, (port, scheme, _, _, _) in haproxy_dict.items() -%}
 {{   family_name ~ '-v6' }} = {% if ipv6_set %}{{ scheme ~ '://[' ~ ipv6 ~ ']:' ~ port }}{% endif %}
 {{   family_name }} = {{ scheme ~ '://' ~ ipv4 ~ ':' ~ port }}
 {% endfor -%}

stack/erp5/instance-erp5.cfg.in

@@ -271,6 +271,7 @@ software-type = zope
 {% set zope_family_name_list = [] -%}
 {% set zope_backend_path_dict = {} -%}
 {% set ssl_authentication_dict = {} -%}
+{% set balancer_timeout_dict = {} -%}
 {% set jupyter_zope_family_default = [] -%}
 {% for custom_name, zope_parameter_dict in six.iteritems(zope_partition_dict) -%}
 {%   set partition_name = 'zope-' ~ custom_name -%}
@@ -288,6 +289,7 @@ software-type = zope
 {%   do zope_backend_path_dict.__setitem__(zope_family, backend_path) -%}
 {%   do ssl_authentication_dict.__setitem__(zope_family, zope_parameter_dict.get('ssl-authentication', False)) -%}
 {%   set current_zope_family_override_dict = zope_family_override_dict.get(zope_family, {}) -%}
+{%   do balancer_timeout_dict.__setitem__(zope_family, current_zope_family_override_dict.get('publisher-timeout', global_publisher_timeout)) -%}
 [{{ section_name }}]
 <= request-zope-base
 name = {{ partition_name }}
@@ -406,6 +408,7 @@ config-url = ${request-jupyter:connection-url}
     'zope-family-dict': zope_family_parameter_dict,
     'backend-path-dict': zope_backend_path_dict,
     'ssl-authentication-dict': ssl_authentication_dict,
+    'timeout-dict': balancer_timeout_dict,
     'apachedex-promise-threshold': monitor_dict.get('apachedex-promise-threshold', 70),
     'apachedex-configuration': monitor_dict.get(
       'apachedex-configuration',

[Vincent Pelletier]

Trivial change: , None should be unnecessary here, I think.

[Vincent Pelletier]

...actually, do we even need to use get here ? instance-erp5.cfg.in should be providing values for every family, so maybe it could just fail instanciation if zope-family-dict contains more entires than timeout-dict.

[Jérome Perrin]

Thanks, I think I left this default value to .get() accidentally. Also you are right, using .get here for timeout-dict and also for ssl-authentication-dict (which is also created in instance-erp5.cfg.in) is not needed and it may even hide bugs. I'm adding a commit to switch to [] syntax and if test passes I'll merge all this