launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25207
[Merge] ~twom/launchpad:stats-queues-in-buildd into launchpad:master
Tom Wardill has proposed merging ~twom/launchpad:stats-queues-in-buildd into launchpad:master.
Commit message:
Add counter for failed builders
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/389655
Increment a counter when we judge a builder as failed.
Also restructure the stats client retrieval to be a singleton via a Zope Utility.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:stats-queues-in-buildd into launchpad:master.
diff --git a/lib/lp/buildmaster/manager.py b/lib/lp/buildmaster/manager.py
index 3ace0d8..9643b34 100644
--- a/lib/lp/buildmaster/manager.py
+++ b/lib/lp/buildmaster/manager.py
@@ -61,7 +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
+from lp.services.statsd.interfaces.lp_statsd_client import ILPStatsdClient
BUILDD_MANAGER_LOG_NAME = "slave-scanner"
@@ -454,6 +454,8 @@ class SlaveScanner:
self._cached_build_cookie = None
self._cached_build_queue = None
+ self.statsd_client = getUtility(ILPStatsdClient).getClient()
+
def startCycle(self):
"""Scan the builder and dispatch to it or deal with failures."""
self.loop = LoopingCall(self.singleCycle)
@@ -523,6 +525,7 @@ class SlaveScanner:
if builder.current_build is not None:
builder.current_build.gotFailure()
recover_failure(self.logger, vitals, builder, retry, failure.value)
+ self.statsd_client.incr('builders.judged_failed')
transaction.commit()
except Exception:
# Catastrophic code failure! Not much we can do.
@@ -706,7 +709,7 @@ class BuilddManager(service.Service):
self.logger = self._setupLogger()
self.current_builders = []
self.pending_logtails = {}
- self.statsd_client = get_statsd_client()
+ self.statsd_client = getUtility(ILPStatsdClient).getClient()
def _setupLogger(self):
"""Set up a 'slave-scanner' logger that redirects to twisted.
diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
index 06b4e2d..c202b4c 100644
--- a/lib/lp/buildmaster/tests/test_manager.py
+++ b/lib/lp/buildmaster/tests/test_manager.py
@@ -13,7 +13,7 @@ import os
import signal
import time
-from fixtures import MockPatch
+import mock
from six.moves import xmlrpc_client
from testtools.matchers import (
Equals,
@@ -78,6 +78,7 @@ from lp.buildmaster.tests.test_interactor import (
from lp.registry.interfaces.distribution import IDistributionSet
from lp.services.config import config
from lp.services.log.logger import BufferLogger
+from lp.services.statsd.interfaces.lp_statsd_client import ILPStatsdClient
from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
from lp.soyuz.model.binarypackagebuildbehaviour import (
BinaryPackageBuildBehaviour,
@@ -92,6 +93,7 @@ from lp.testing import (
from lp.testing.dbuser import switch_dbuser
from lp.testing.factory import LaunchpadObjectFactory
from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import (
LaunchpadScriptLayer,
LaunchpadZopelessLayer,
@@ -101,7 +103,19 @@ from lp.testing.matchers import HasQueryCount
from lp.testing.sampledata import BOB_THE_BUILDER_NAME
-class TestSlaveScannerScan(TestCaseWithFactory):
+class StatsMixin:
+
+ def setUpStats(self):
+ # Mock the utility class, then return a known value
+ # from getClient(), so we can assert against the call counts and args.
+ utility_class = mock.Mock()
+ self.stats_client = mock.Mock()
+ utility_class.getClient.return_value = self.stats_client
+ self.useFixture(
+ ZopeUtilityFixture(utility_class, ILPStatsdClient))
+
+
+class TestSlaveScannerScan(StatsMixin, TestCaseWithFactory):
"""Tests `SlaveScanner.scan` method.
This method uses the old framework for scanning and dispatching builds.
@@ -123,6 +137,7 @@ class TestSlaveScannerScan(TestCaseWithFactory):
self.test_publisher.setUpDefaultDistroSeries(hoary)
self.test_publisher.addFakeChroots(db_only=True)
self.addCleanup(shut_down_default_process_pool)
+ self.setUpStats()
def _resetBuilder(self, builder):
"""Reset the given builder and its job."""
@@ -528,6 +543,20 @@ class TestSlaveScannerScan(TestCaseWithFactory):
self.assertFalse(builder.builderok)
@defer.inlineCallbacks
+ def test_scanFailed_increments_counter(self):
+ def failing_scan():
+ return defer.fail(Exception("fake exception"))
+ scanner = self._getScanner()
+ scanner.scan = failing_scan
+ builder = getUtility(IBuilderSet)[scanner.builder_name]
+ builder.failure_count = BUILDER_FAILURE_THRESHOLD
+ builder.currentjob.reset()
+ transaction.commit()
+
+ yield scanner.singleCycle()
+ self.assertEqual(1, self.stats_client.incr.call_count)
+
+ @defer.inlineCallbacks
def test_fail_to_resume_leaves_it_dirty(self):
# If an attempt to resume a slave fails, its failure count is
# incremented and it is left DIRTY.
@@ -893,6 +922,7 @@ class FakeBuilddManager:
class TestSlaveScannerWithoutDB(TestCase):
+ layer = ZopelessDatabaseLayer
run_tests_with = AsynchronousDeferredRunTest
def setUp(self):
@@ -1619,17 +1649,14 @@ class TestBuilddManagerScript(TestCaseWithFactory):
"Twistd's log file was rotated by twistd.")
-class TestStats(TestCaseWithFactory):
+class TestStats(StatsMixin, 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()
+ self.setUpStats()
def test_single_processor(self):
builder = self.factory.makeBuilder()
diff --git a/lib/lp/services/configure.zcml b/lib/lp/services/configure.zcml
index f23faea..bdde7df 100644
--- a/lib/lp/services/configure.zcml
+++ b/lib/lp/services/configure.zcml
@@ -29,6 +29,7 @@
<include package=".signing" />
<include package=".sitesearch" />
<include package=".statistics" />
+ <include package=".statsd" />
<include package=".temporaryblobstorage" />
<include package=".verification" />
<include package=".webhooks" />
diff --git a/lib/lp/services/statsd/__init__.py b/lib/lp/services/statsd/__init__.py
index 22c839f..e69de29 100644
--- a/lib/lp/services/statsd/__init__.py
+++ b/lib/lp/services/statsd/__init__.py
@@ -1,35 +0,0 @@
-# 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/configure.zcml b/lib/lp/services/statsd/configure.zcml
new file mode 100644
index 0000000..761f5f4
--- /dev/null
+++ b/lib/lp/services/statsd/configure.zcml
@@ -0,0 +1,17 @@
+<!-- Copyright 2015-2020 Canonical Ltd. This software is licensed under the
+ GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+<configure
+ xmlns="http://namespaces.zope.org/zope";
+ xmlns:i18n="http://namespaces.zope.org/i18n";
+ xmlns:lp="http://namespaces.canonical.com/lp";
+ xmlns:webservice="http://namespaces.canonical.com/webservice";
+ i18n_domain="launchpad">
+
+ <!-- utility, rather than securedutility as we don't want the child
+ objects (the statsd clients) to be wrapped in the proxy -->
+ <utility
+ factory="lp.services.statsd.model.lp_statsd_client.LPStatsdClient"
+ provides="lp.services.statsd.interfaces.lp_statsd_client.ILPStatsdClient">
+ </utility>
+</configure>
diff --git a/lib/lp/services/statsd/interfaces/__init__.py b/lib/lp/services/statsd/interfaces/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lib/lp/services/statsd/interfaces/__init__.py
diff --git a/lib/lp/services/statsd/interfaces/lp_statsd_client.py b/lib/lp/services/statsd/interfaces/lp_statsd_client.py
new file mode 100644
index 0000000..b7b085b
--- /dev/null
+++ b/lib/lp/services/statsd/interfaces/lp_statsd_client.py
@@ -0,0 +1,19 @@
+# Copyright 2020 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Interfaces for configuring and retrieving a statsd client."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = ['ILPStatsdClient']
+
+
+from zope.interface import Interface
+
+
+class ILPStatsdClient(Interface):
+ """Methods for retrieving a statsd client using Launchpad config."""
+
+ def getClient():
+ """Return an appropriately configured statsd client."""
diff --git a/lib/lp/services/statsd/model/__init__.py b/lib/lp/services/statsd/model/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/lib/lp/services/statsd/model/__init__.py
diff --git a/lib/lp/services/statsd/model/lp_statsd_client.py b/lib/lp/services/statsd/model/lp_statsd_client.py
new file mode 100644
index 0000000..2f14544
--- /dev/null
+++ b/lib/lp/services/statsd/model/lp_statsd_client.py
@@ -0,0 +1,48 @@
+# 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__ = ['LPStatsdClient']
+
+
+from statsd import StatsClient
+from zope.interface import implementer
+
+from lp.services.config import config
+from lp.services.statsd.interfaces.lp_statsd_client import ILPStatsdClient
+
+
+client = None
+
+
+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
+
+
+@implementer(ILPStatsdClient)
+class LPStatsdClient:
+ """See `ILPStatsdClient`."""
+
+ client = None
+
+ def getClient(self):
+ if not self.client:
+ if config.statsd.host:
+ self.client = StatsClient(
+ host=config.statsd.host,
+ port=config.statsd.port,
+ prefix=config.statsd.prefix)
+ else:
+ self.client = UnconfiguredStatsdClient()
+ return self.client
diff --git a/lib/lp/services/statsd/tests/test_lp_statsd.py b/lib/lp/services/statsd/tests/test_lp_statsd.py
index 691e93d..0f51faa 100644
--- a/lib/lp/services/statsd/tests/test_lp_statsd.py
+++ b/lib/lp/services/statsd/tests/test_lp_statsd.py
@@ -8,33 +8,42 @@ from __future__ import absolute_import, print_function, unicode_literals
__metaclass__ = type
from statsd import StatsClient
+from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from lp.services.config import config
-from lp.services.statsd import (
- get_statsd_client,
+from lp.services.statsd.interfaces.lp_statsd_client import ILPStatsdClient
+from lp.services.statsd.model.lp_statsd_client import (
+ LPStatsdClient,
UnconfiguredStatsdClient,
)
from lp.testing import TestCase
-from lp.testing.layers import BaseLayer
+from lp.testing.layers import ZopelessLayer
class TestClientConfiguration(TestCase):
- layer = BaseLayer
+ layer = ZopelessLayer
+
+ def test_accessible_via_utility(self):
+ """Test that we can access the class via a zope utility."""
+ config.push(
+ 'statsd_test',
+ "[statsd]\nhost: 127.0.01\nport: 9999\nprefix: test\n")
+ client = getUtility(ILPStatsdClient).getClient()
+ self.assertEqual(type(client), StatsClient)
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)
+ client = LPStatsdClient().getClient()
+ 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)
+ client = LPStatsdClient().getClient()
+ self.assertEqual(type(client), StatsClient)
