Merge ~cjwatson/launchpad:number-cruncher-transactions into launchpad:master

[Colin Watson]

Abort transaction after each number-cruncher loop

We only read from the database, but if we don't end our transaction from time to time then we can end up with incorrect graphs and very long transactions.

[Thiago F. Pappacena (pappacena)]

Do you think it worth adding a comment on `NumberCruncher` class to warn future travelers that we always abort transaction?

Just in case we ever try to use it somewhere outside numbercruncher.tac, and end up with data not being persisted at database.

review: Approve

9bc4751... by Colin Watson on 2020-11-10

Add comment about transaction handling

[Colin Watson (cjwatson)]

That seems reasonable, thanks. I've updated the docstrings a bit.

diff --git a/lib/lp/services/statsd/numbercruncher.py b/lib/lp/services/statsd/numbercruncher.py
index 0a0552c..85fa2cd 100644
--- a/lib/lp/services/statsd/numbercruncher.py
+++ b/lib/lp/services/statsd/numbercruncher.py
@@ -10,6 +10,7 @@ __all__ = ['NumberCruncher']
 
 import logging
 
+import transaction
 from twisted.application import service
 from twisted.internet import (
     defer,
@@ -68,7 +69,10 @@ class NumberCruncher(service.Service):
         return loop, stopping_deferred
 
     def updateBuilderQueues(self):
-        """Update statsd with the build queue lengths."""
+        """Update statsd with the build queue lengths.
+
+        This aborts the current transaction before returning.
+        """
         self.logger.debug("Updating build queue stats.")
         queue_details = getUtility(IBuilderSet).getBuildQueueSizes()
         for queue_type, contents in queue_details.items():
@@ -79,6 +83,7 @@ class NumberCruncher(service.Service):
                 self.logger.debug("{}: {}".format(gauge_name, value[0]))
                 self.statsd_client.gauge(gauge_name, value[0])
         self.logger.debug("Build queue stats update complete.")
+        transaction.abort()
 
     def _updateBuilderCounts(self):
         """Update statsd with the builder statuses.
@@ -114,9 +119,13 @@ class NumberCruncher(service.Service):
         self.logger.debug("Builder stats update complete.")
 
     def updateBuilderStats(self):
-        """Statistics that require builder knowledge to be updated."""
+        """Statistics that require builder knowledge to be updated.
+
+        This aborts the current transaction before returning.
+        """
         self.builder_factory.update()
         self._updateBuilderCounts()
+        transaction.abort()
 
     def startService(self):
         self.logger.info("Starting number-cruncher service.")
diff --git a/lib/lp/services/statsd/tests/test_numbercruncher.py b/lib/lp/services/statsd/tests/test_numbercruncher.py
index c5a4ff6..ede74e1 100644
--- a/lib/lp/services/statsd/tests/test_numbercruncher.py
+++ b/lib/lp/services/statsd/tests/test_numbercruncher.py
@@ -20,6 +20,7 @@ from lp.buildmaster.enums import BuilderCleanStatus
 from lp.buildmaster.interactor import BuilderSlave
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.tests.mock_slaves import OkSlave
+from lp.services.database.isolation import is_transaction_in_progress
 from lp.services.statsd.numbercruncher import NumberCruncher
 from lp.services.statsd.tests import StatsMixin
 from lp.testing import TestCaseWithFactory
@@ -46,6 +47,7 @@ class TestNumberCruncher(StatsMixin, TestCaseWithFactory):
         manager.builder_factory.update()
         manager.updateBuilderStats()
 
+        self.assertFalse(is_transaction_in_progress())
         self.assertEqual(8, self.stats_client.gauge.call_count)
         for call in self.stats_client.mock.gauge.call_args_list:
             self.assertIn('386', call[0][0])
@@ -61,6 +63,7 @@ class TestNumberCruncher(StatsMixin, TestCaseWithFactory):
         manager.builder_factory.update()
         manager.updateBuilderStats()
 
+        self.assertFalse(is_transaction_in_progress())
         self.assertEqual(12, self.stats_client.gauge.call_count)
         i386_calls = [c for c in self.stats_client.gauge.call_args_list
                       if '386' in c[0][0]]
@@ -80,6 +83,7 @@ class TestNumberCruncher(StatsMixin, TestCaseWithFactory):
         manager.builder_factory.update()
         manager.updateBuilderStats()
 
+        self.assertFalse(is_transaction_in_progress())
         self.assertEqual(12, self.stats_client.gauge.call_count)
         calls = [c[0] for c in self.stats_client.gauge.call_args_list
                  if 'amd64' in c[0][0]]
@@ -113,6 +117,7 @@ class TestNumberCruncher(StatsMixin, TestCaseWithFactory):
         manager.builder_factory.update()
         manager.updateBuilderQueues()
 
+        self.assertFalse(is_transaction_in_progress())
         self.assertEqual(2, self.stats_client.gauge.call_count)
         self.assertThat(
             [x[0] for x in self.stats_client.gauge.call_args_list],