launchpad-reviewers team mailing list archive

[Colin Watson <mp+440349@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:charm-appserver-rolling-restart into launchpad:master.

Commit message:
charm: Use rolling restart for launchpad-appserver

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

As long as we have more than one appserver unit, this ensures that the application remains responsive during upgrades, using the coordinator layer to deal with communication between units.

With gunicorn >= 20.0, waiting for the application to be responsive on each unit turns out to be a matter of setting `preload_app = True` (which causes the application to be loaded before the arbiter signals readiness, and also saves a fair amount of memory) and using `Type=notify` in the systemd service.  I dropped the old soft-restart mechanism when only `*-lazr.conf` files are changed, since it doesn't quite work with application preloading and it wasn't all that much of a win anyway.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:charm-appserver-rolling-restart into launchpad:master.

diff --git a/charm/launchpad-appserver/charmcraft.yaml b/charm/launchpad-appserver/charmcraft.yaml
index b35779b..613dc0b 100644
--- a/charm/launchpad-appserver/charmcraft.yaml
+++ b/charm/launchpad-appserver/charmcraft.yaml
@@ -45,10 +45,23 @@ parts:
       - layers
     prime:
       - "-layers"
+  layer-coordinator:
+    source: https://git.launchpad.net/layer-coordinator
+    source-commit: "fa27fc93e0b08000963e83a6bfe49812d890dfcf"
+    source-submodules: []
+    source-type: git
+    plugin: dump
+    organize:
+      "*": layers/layer/coordinator/
+    stage:
+      - layers
+    prime:
+      - "-layers"
   launchpad-appserver:
     after:
       - charm-wheels
       - launchpad-layers
+      - layer-coordinator
     source: .
     plugin: reactive
     build-snaps: [charm/2.x/stable]
diff --git a/charm/launchpad-appserver/layer.yaml b/charm/launchpad-appserver/layer.yaml
index dae6f8a..87a5643 100644
--- a/charm/launchpad-appserver/layer.yaml
+++ b/charm/launchpad-appserver/layer.yaml
@@ -1,5 +1,6 @@
 includes:
   - layer:launchpad-base
+  - layer:coordinator
   - interface:memcache
 repo: https://git.launchpad.net/launchpad
 options:
diff --git a/charm/launchpad-appserver/reactive/launchpad-appserver.py b/charm/launchpad-appserver/reactive/launchpad-appserver.py
index 99ef7f6..1c32282 100644
--- a/charm/launchpad-appserver/reactive/launchpad-appserver.py
+++ b/charm/launchpad-appserver/reactive/launchpad-appserver.py
@@ -6,6 +6,7 @@ import subprocess
 from multiprocessing import cpu_count
 
 from charmhelpers.core import hookenv, host, templating
+from charms.coordinator import acquire
 from charms.launchpad.base import (
     config_file_path,
     configure_cron,
@@ -24,6 +25,7 @@ from charms.reactive import (
     set_flag,
     set_state,
     when,
+    when_none,
     when_not,
 )
 from ols import base, postgres
@@ -64,6 +66,7 @@ def configure_gunicorn(config):
     templating.render(
         "launchpad.service.j2", "/lib/systemd/system/launchpad.service", config
     )
+    subprocess.run(["systemctl", "daemon-reload"], check=True)
     host.add_user_to_group("syslog", base.user())
     templating.render("rsyslog.j2", "/etc/rsyslog.d/22-launchpad.conf", config)
 
@@ -78,13 +81,6 @@ def configure_logrotate(config):
     )
 
 
-def restart(soft=False):
-    if soft:
-        reload_or_restart("launchpad")
-    else:
-        host.service_restart("launchpad")
-
-
 def config_files():
     files = []
     files.extend(lazr_config_files())
@@ -100,7 +96,7 @@ def config_files():
     "session-db.master.available",
     "memcache.available",
 )
-@when_not("service.configured")
+@when_none("coordinator.requested.restart", "service.configured")
 def configure():
     session_db = endpoint_from_flag("session-db.master.available")
     memcache = endpoint_from_flag("memcache.available")
@@ -133,19 +129,21 @@ def configure():
     configure_logrotate(config)
     configure_cron(config, "crontab.j2")
 
-    restart_type = None
     if helpers.any_file_changed(
         [base.version_info_path(), "/lib/systemd/system/launchpad.service"]
+        + config_files()
     ):
-        restart_type = "hard"
-    elif helpers.any_file_changed(config_files()):
-        restart_type = "soft"
-    if restart_type is None:
-        hookenv.log("Not restarting, since no config files were changed")
+        hookenv.log("Config files changed; waiting for restart lock")
+        acquire("restart")
     else:
-        hookenv.log(f"Config files changed; performing {restart_type} restart")
-        restart(soft=(restart_type == "soft"))
+        hookenv.log("Not restarting, since no config files were changed")
+        set_state("service.configured")
+
 
+@when("coordinator.granted.restart")
+def restart():
+    hookenv.log("Restarting application server")
+    host.service_restart("launchpad")
     set_state("service.configured")
 
 
diff --git a/charm/launchpad-appserver/templates/gunicorn.conf.py.j2 b/charm/launchpad-appserver/templates/gunicorn.conf.py.j2
index efd8a1e..09d8866 100644
--- a/charm/launchpad-appserver/templates/gunicorn.conf.py.j2
+++ b/charm/launchpad-appserver/templates/gunicorn.conf.py.j2
@@ -1,5 +1,7 @@
 bind = [":{{ port_main }}", ":{{ port_xmlrpc }}"]
 workers = {{ wsgi_workers }}
+# Incompatible with `reload = True`, but that's fine for a Juju deployment.
+preload_app = True
 # This is set relatively low to work around memory leaks on Python 3.
 max_requests = 2000
 log_level = "DEBUG"
diff --git a/charm/launchpad-appserver/templates/launchpad.service.j2 b/charm/launchpad-appserver/templates/launchpad.service.j2
index fb5c372..c9290f4 100644
--- a/charm/launchpad-appserver/templates/launchpad.service.j2
+++ b/charm/launchpad-appserver/templates/launchpad.service.j2
@@ -4,6 +4,7 @@ After=network.target
 ConditionPathExists=!{{ code_dir }}/maintenance.txt
 
 [Service]
+Type=notify
 User=launchpad
 Group=launchpad
 WorkingDirectory={{ code_dir }}