launchpad-reviewers team mailing list archive

[Colin Watson <mp+447085@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:twisted-start-feature-controller-earlier into launchpad:master.

Commit message:
Start feature controller earlier in Twisted services

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

After migrating buildd-manager to a charmed deployment, it quite frequently seems to start up in a bad state, where all its database operations fail with errors like "permission denied for relation builder".

I can reproduce this on staging, but only sometimes, and unreliably: it seems to be a race condition of some kind.  My best guess is that it has something to do with the different database connection arrangements for charmed deployments, where we switch to the desired database role after connecting.  As such, I guessed that setting up the feature controller earlier in the service startup sequence might help, since that makes a database connection, and it would make some sense that it might matter whether this happens before or after we start a bunch of threads; and indeed when I try that I don't seem to be able to reproduce the problem on staging any more.

This isn't entirely satisfactory, and I'd like to understand the problem better, but perhaps this will do for now.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:twisted-start-feature-controller-earlier into launchpad:master.

diff --git a/daemons/buildd-manager.tac b/daemons/buildd-manager.tac
index 234c976..876ecae 100644
--- a/daemons/buildd-manager.tac
+++ b/daemons/buildd-manager.tac
@@ -23,6 +23,11 @@ dbconfig.override(dbuser="buildd_manager", isolation_level="read_committed")
 # Should be removed from callsites verified to not need it.
 set_immediate_mail_delivery(True)
 
+# Allow use of feature flags.  Do this before setting up the Twisted
+# application, in order to ensure that we switch to the correct database
+# role before starting any threads.
+setup_feature_controller("buildd-manager")
+
 # ampoule uses five file descriptors per subprocess (i.e.
 # 5 * config.builddmaster.download_connections); we also need at least three
 # per active builder for resuming virtualized builders or making XML-RPC
@@ -46,6 +51,3 @@ readyservice.ReadyService().setServiceParent(application)
 # Service for scanning buildd workers.
 service = BuilddManager()
 service.setServiceParent(application)
-
-# Allow use of feature flags.
-setup_feature_controller("buildd-manager")
diff --git a/daemons/librarian.tac b/daemons/librarian.tac
index 062ef54..4522b9c 100644
--- a/daemons/librarian.tac
+++ b/daemons/librarian.tac
@@ -45,6 +45,11 @@ execute_zcml_for_scripts(
     scriptzcmlfilename="librarian.zcml", setup_interaction=False
 )
 
+# Allow use of feature flags.  Do this before setting up the Twisted
+# application, in order to ensure that we switch to the correct database
+# role before starting any threads.
+setup_feature_controller("librarian")
+
 if os.environ.get("LP_TEST_INSTANCE"):
     # Running in ephemeral mode: get the root dir from the environment and
     # dynamically allocate ports.
@@ -121,9 +126,6 @@ logfile = options.get("logfile")
 observer = set_up_oops_reporting("librarian", "librarian", logfile)
 application.addComponent(observer, ignoreClass=1)
 
-# Allow use of feature flags.
-setup_feature_controller("librarian")
-
 
 # Setup a signal handler to dump the process' memory upon 'kill -44'.
 def sigdumpmem_handler(signum, frame):
diff --git a/daemons/numbercruncher.tac b/daemons/numbercruncher.tac
index 8efe410..f396f32 100644
--- a/daemons/numbercruncher.tac
+++ b/daemons/numbercruncher.tac
@@ -16,6 +16,11 @@ from lp.services.twistedsupport.loggingsupport import RotatableFileLogObserver
 
 execute_zcml_for_scripts()
 
+# Allow use of feature flags.  Do this before setting up the Twisted
+# application, in order to ensure that we switch to the correct database
+# role before starting any threads.
+setup_feature_controller("number-cruncher")
+
 options = ServerOptions()
 options.parseOptions()
 
@@ -31,6 +36,3 @@ readyservice.ReadyService().setServiceParent(application)
 # Service for updating statsd receivers.
 service = NumberCruncher()
 service.setServiceParent(application)
-
-# Allow use of feature flags.
-setup_feature_controller("number-cruncher")