Merge ~twom/launchpad:stats-in-buildd-manager into launchpad:master

[Tom Wardill]

Add statsd totals for builder stats by processor

It'd be useful to graph the builder states over time, by architecture. Create a statsd client that is configured from settings (with a dummy one to prevent network traffic if not).
Use this in builddmanager in an extra (tunable) loop to prevent impingement on other manager work.

[Ioana Lasc (ilasc)]

looks good to me but i'd get a review from Colin as well.

review: Approve

[Colin Watson (cjwatson)]

review: Approve

bf1b6d3... by Tom Wardill on 2020-08-18

Remove unnecessary empty config

9eea3aa... by Tom Wardill on 2020-08-18

Add startService test, fix client method name

5f433b1... by Tom Wardill on 2020-08-18

Actually include the value, not the name

eeba945... by Tom Wardill on 2020-08-18

Change to use labels/tags for gauge names

aef55cb... by Tom Wardill on 2020-08-19

Change tag format, include more attributes as tags

[Colin Watson (cjwatson)]

review: Approve

diff --git a/constraints.txt b/constraints.txt
index 8a9077d..79ed49e 100644
--- a/constraints.txt
+++ b/constraints.txt
@@ -297,6 +297,7 @@ SimpleTAL==4.3; python_version < "3"
 SimpleTAL==5.2; python_version >= "3"
 soupmatchers==0.4
 soupsieve==1.9
+statsd==3.3.0
 # lp:~launchpad-committers/storm/lp
 storm==0.24+lp416
 subprocess32==3.2.6
diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index 7450a0b..b1d9ccd 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -321,7 +321,7 @@ BuilderVitals = namedtuple(
     'BuilderVitals',
     ('name', 'url', 'processor_names', 'virtualized', 'vm_host',
      'vm_reset_protocol', 'builderok', 'manual', 'build_queue', 'version',
-     'clean_status'))
+     'clean_status', 'active'))
 
 _BQ_UNSPECIFIED = object()
 
@@ -334,7 +334,7 @@ def extract_vitals_from_db(builder, build_queue=_BQ_UNSPECIFIED):
         [processor.name for processor in builder.processors],
         builder.virtualized, builder.vm_host, builder.vm_reset_protocol,
         builder.builderok, builder.manual, build_queue, builder.version,
-        builder.clean_status)
+        builder.clean_status, builder.active)
 
 
 class BuilderInteractor(object):
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index a4b96e3..3ace0d8 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -61,6 +61,7 @@ from lp.services.database.stormexpr import (
     Values,
     )
 from lp.services.propertycache import get_property_cache
+from lp.services.statsd import get_statsd_client
 
 
 BUILDD_MANAGER_LOG_NAME = "slave-scanner"
@@ -691,6 +692,9 @@ class BuilddManager(service.Service):
     # How often to flush logtail updates, in seconds.
     FLUSH_LOGTAILS_INTERVAL = 15
 
+    # How often to update stats, in seconds
+    UPDATE_STATS_INTERVAL = 60
+
     def __init__(self, clock=None, builder_factory=None):
         # Use the clock if provided, it's so that tests can
         # advance it.  Use the reactor by default.
@@ -702,6 +706,7 @@ class BuilddManager(service.Service):
         self.logger = self._setupLogger()
         self.current_builders = []
         self.pending_logtails = {}
+        self.statsd_client = get_statsd_client()
 
     def _setupLogger(self):
         """Set up a 'slave-scanner' logger that redirects to twisted.
@@ -721,6 +726,36 @@ class BuilddManager(service.Service):
         logger.setLevel(level)
         return logger
 
+    def updateStats(self):
+        """Update statsd with the builder statuses."""
+        self.logger.debug("Updating builder stats.")
+        counts_by_processor = {}
+        for builder in self.builder_factory.iterVitals():
+            if not builder.active:
+                continue
+            for processor_name in builder.processor_names:
+                counts = counts_by_processor.setdefault(
+                    "{},virtualized={}".format(
+                        processor_name,
+                        builder.virtualized),
+                    {'cleaning': 0, 'idle': 0, 'disabled': 0, 'building': 0})
+                if not builder.builderok:
+                    counts['disabled'] += 1
+                elif builder.clean_status == BuilderCleanStatus.CLEANING:
+                    counts['cleaning'] += 1
+                elif (builder.build_queue and
+                      builder.build_queue.status == BuildQueueStatus.RUNNING):
+                    counts['building'] += 1
+                elif builder.clean_status == BuilderCleanStatus.CLEAN:
+                    counts['idle'] += 1
+        for processor, counts in counts_by_processor.items():
+            for count_name, count_value in counts.items():
+                gauge_name = "builders.{},arch={}".format(
+                    count_name, processor)
+                self.logger.debug("{}: {}".format(gauge_name, count_value))
+                self.statsd_client.gauge(gauge_name, count_value)
+        self.logger.debug("Builder stats update complete.")
+
     def checkForNewBuilders(self):
         """Add and return any new builders."""
         new_builders = set(
@@ -790,6 +825,9 @@ class BuilddManager(service.Service):
         # Schedule bulk flushes for build queue logtail updates.
         self.flush_logtails_loop, self.flush_logtails_deferred = (
             self._startLoop(self.FLUSH_LOGTAILS_INTERVAL, self.flushLogTails))
+        # Schedule stats updates.
+        self.stats_update_loop, self.stats_update_deferred = (
+            self._startLoop(self.UPDATE_STATS_INTERVAL, self.updateStats))
 
     def stopService(self):
         """Callback for when we need to shut down."""
@@ -798,9 +836,11 @@ class BuilddManager(service.Service):
         deferreds = [slave.stopping_deferred for slave in self.builder_slaves]
         deferreds.append(self.scan_builders_deferred)
         deferreds.append(self.flush_logtails_deferred)
+        deferreds.append(self.stats_update_deferred)
 
         self.flush_logtails_loop.stop()
         self.scan_builders_loop.stop()
+        self.stats_update_loop.stop()
         for slave in self.builder_slaves:
             slave.stopCycle()
 
diff --git a/lib/lp/buildmaster/tests/mock_slaves.py b/lib/lp/buildmaster/tests/mock_slaves.py
index 2794137..2e935e4 100644
--- a/lib/lp/buildmaster/tests/mock_slaves.py
+++ b/lib/lp/buildmaster/tests/mock_slaves.py
@@ -58,7 +58,8 @@ class MockBuilder:
                  processors=None, virtualized=True, vm_host=None,
                  url='http://fake:0000', version=None,
                  clean_status=BuilderCleanStatus.DIRTY,
-                 vm_reset_protocol=BuilderResetProtocol.PROTO_1_1):
+                 vm_reset_protocol=BuilderResetProtocol.PROTO_1_1,
+                 active=True):
         self.currentjob = None
         self.builderok = builderok
         self.manual = manual
@@ -71,6 +72,7 @@ class MockBuilder:
         self.failnotes = None
         self.version = version
         self.clean_status = clean_status
+        self.active = active
 
     def setCleanStatus(self, clean_status):
         self.clean_status = clean_status
diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
index b0d56ed..06b4e2d 100644
--- a/lib/lp/buildmaster/tests/test_manager.py
+++ b/lib/lp/buildmaster/tests/test_manager.py
@@ -13,8 +13,12 @@ import os
 import signal
 import time
 
+from fixtures import MockPatch
 from six.moves import xmlrpc_client
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    MatchesListwise,
+    )
 from testtools.testcase import ExpectedException
 from testtools.twistedsupport import AsynchronousDeferredRunTest
 import transaction
@@ -45,6 +49,7 @@ from lp.buildmaster.interfaces.builder import (
     IBuilderSet,
     )
 from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet
+from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.buildmaster.manager import (
     BuilddManager,
     BUILDER_FAILURE_THRESHOLD,
@@ -1207,6 +1212,22 @@ class TestBuilddManager(TestCase):
         clock.advance(advance)
         self.assertNotEqual(0, manager.flushLogTails.call_count)
 
+    def test_startService_adds_updateStats_loop(self):
+        # When startService is called, the manager will start up a
+        # updateStats loop.
+        self._stub_out_scheduleNextScanCycle()
+        clock = task.Clock()
+        manager = BuilddManager(clock=clock)
+
+        # Replace updateStats() with FakeMethod so we can see if it was
+        # called.
+        manager.updateStats = FakeMethod()
+
+        manager.startService()
+        advance = BuilddManager.UPDATE_STATS_INTERVAL + 1
+        clock.advance(advance)
+        self.assertNotEqual(0, manager.updateStats.call_count)
+
 
 class TestFailureAssessments(TestCaseWithFactory):
 
@@ -1596,3 +1617,71 @@ class TestBuilddManagerScript(TestCaseWithFactory):
         self.assertFalse(
             os.access(rotated_logfilepath, os.F_OK),
             "Twistd's log file was rotated by twistd.")
+
+
+class TestStats(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
+
+    def setUp(self):
+        super(TestStats, self).setUp()
+        self.stats_client = self.useFixture(
+            MockPatch(
+                'lp.buildmaster.manager.get_statsd_client'
+            )).mock()
+
+    def test_single_processor(self):
+        builder = self.factory.makeBuilder()
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
+        transaction.commit()
+        clock = task.Clock()
+        manager = BuilddManager(clock=clock)
+        manager.builder_factory.update()
+        manager.updateStats()
+
+        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])
+
+    def test_multiple_processor(self):
+        builder = self.factory.makeBuilder(
+            processors=[getUtility(IProcessorSet).getByName('amd64')])
+        builder.setCleanStatus(BuilderCleanStatus.CLEAN)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
+        transaction.commit()
+        clock = task.Clock()
+        manager = BuilddManager(clock=clock)
+        manager.builder_factory.update()
+        manager.updateStats()
+
+        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]]
+        amd64_calls = [c for c in self.stats_client.gauge.call_args_list
+                       if 'amd64' in c[0][0]]
+        self.assertEqual(8, len(i386_calls))
+        self.assertEqual(4, len(amd64_calls))
+
+    def test_correct_values(self):
+        builder = self.factory.makeBuilder(
+            processors=[getUtility(IProcessorSet).getByName('amd64')])
+        builder.setCleanStatus(BuilderCleanStatus.CLEANING)
+        self.patch(BuilderSlave, 'makeBuilderSlave', FakeMethod(OkSlave()))
+        transaction.commit()
+        clock = task.Clock()
+        manager = BuilddManager(clock=clock)
+        manager.builder_factory.update()
+        manager.updateStats()
+
+        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]]
+        self.assertThat(
+            calls, MatchesListwise(
+                [Equals(('builders.disabled,arch=amd64,virtualized=True', 0)),
+                 Equals(('builders.building,arch=amd64,virtualized=True', 0)),
+                 Equals(('builders.idle,arch=amd64,virtualized=True', 0)),
+                 Equals(('builders.cleaning,arch=amd64,virtualized=True', 1))
+                 ]))
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 80bc7e2..bb8afcd 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -113,7 +113,6 @@ authentication_endpoint: none
 # authserver.
 authentication_timeout: 15
 
-
 [canonical]
 # datatype: boolean
 show_tracebacks: False
@@ -2022,3 +2021,9 @@ concurrency: 1
 timeout: 86400
 fallback_queue:
 concurrency: 1
+
+# Statsd connection details
+[statsd]
+host: none
+port: none
+prefix: none
diff --git a/lib/lp/services/statsd/__init__.py b/lib/lp/services/statsd/__init__.py
 new file mode 100644
index 0000000..22c839f
--- /dev/null
+++ b/lib/lp/services/statsd/__init__.py
@@ -0,0 +1,35 @@
+# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Statsd client wrapper with Launchpad configuration"""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = ['get_statsd_client']
+
+
+from statsd import StatsClient
+
+from lp.services.config import config
+
+
+class UnconfiguredStatsdClient:
+    """Dummy client for if statsd is not configured in the environment.
+
+    This client will be used if the statsd settings are not available to
+    Launchpad. Prevents unnecessary network traffic.
+    """
+
+    def __getattr__(self, name):
+        return lambda *args, **kwargs: None
+
+
+def get_statsd_client():
+    if config.statsd.host:
+        return StatsClient(
+            host=config.statsd.host,
+            port=config.statsd.port,
+            prefix=config.statsd.prefix)
+    else:
+        return UnconfiguredStatsdClient()
diff --git a/lib/lp/services/statsd/tests/__init__.py b/lib/lp/services/statsd/tests/__init__.py
 new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lib/lp/services/statsd/tests/__init__.py
diff --git a/lib/lp/services/statsd/tests/test_lp_statsd.py b/lib/lp/services/statsd/tests/test_lp_statsd.py
 new file mode 100644
index 0000000..691e93d
--- /dev/null
+++ b/lib/lp/services/statsd/tests/test_lp_statsd.py
@@ -0,0 +1,40 @@
+# Copyright 2020 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the Launchpad statsd client"""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+from statsd import StatsClient
+
+from lp.services.config import config
+from lp.services.statsd import (
+    get_statsd_client,
+    UnconfiguredStatsdClient,
+    )
+from lp.testing import TestCase
+from lp.testing.layers import BaseLayer
+
+
+class TestClientConfiguration(TestCase):
+
+    layer = BaseLayer
+
+    def test_get_correct_instance_unconfigured(self):
+        """Test that we get the correct client, depending on config values."""
+        config.push(
+            'statsd_test',
+            "[statsd]\nhost:")
+        client = get_statsd_client()
+        self.assertEqual(
+            type(client), UnconfiguredStatsdClient)
+
+    def test_get_correct_instance_configured(self):
+        config.push(
+            'statsd_test',
+            "[statsd]\nhost: 127.0.01\nport: 9999\nprefix: test\n")
+        client = get_statsd_client()
+        self.assertEqual(
+            type(client), StatsClient)
diff --git a/setup.py b/setup.py
index db7963e..0f15f7f 100644
--- a/setup.py
+++ b/setup.py
@@ -232,6 +232,7 @@ setup(
         'six',
         'soupmatchers',
         'Sphinx',
+        'statsd',
         'storm',
         # XXX cjwatson 2020-08-07: Temporarily dropped on Python 3 until
         # codeimport can be ported to Breezy.