From 311a8c2e6f0d2f5e6822d23d19ae62261826463a Mon Sep 17 00:00:00 2001 From: Razvan-Liviu Varzaru Date: Mon, 2 Feb 2026 18:29:46 +0200 Subject: [PATCH] Implementing bb's standard db access pattern for last-N-failed builder MTRLogObserver is deprecated starting with Buildbot 3.3.0. Where possible, without breaking existing builders, we need to move away from using MTR related classes and methods. In this case, subclassing MTR was not even needed for getting the test failures. This patch is implementing the standard db access pattern of Buildbot. See: https://buildbot.readthedocs.io/en/latest/developer/database.html#module-buildbot.db.pool Although the recommended way is to retrieve data through the data api our use case is special enough to make an exception, given that the test_ table schemas will become in-house maintained when we migrate to newer versions of buildbot. **VERY IMPORTANT NOTE**: After removing `MTR` as a parent class I observed that `tests_to_run` start to get populated on `buildbot.dev.mariadb.org` Countless hours later, some debugging code revelead that `test_type` is sent as 'None' to get_tests_for_type ``` 2026-02-02 16:11:40+0000 [-] [FetchTestData] master_branch: type= len=5 repr='10.11' 2026-02-02 16:11:40+0000 [-] [FetchTestData] master_branch tail codepoints: [49, 48, 46, 49, 49] 2026-02-02 16:11:40+0000 [-] [FetchTestData] test_type: type= len=None repr=None ``` The only reason that on `buildbot.mariadb.org`, `tests_run_run` is sometimes populated is because the failures are bumped with the default type as in: ``` tests += yield self.get_tests_for_type( branch, "mtr", limit - len(tests) ) ``` The `mtr` type last appeared on `2025-01-28` as per cross-reference: https://buildbot.mariadb.org/cr/?branch=&revision=&platform=&dt=&bbnum=&typ=MTR&info=&test_name=&test_variant=&info_text=&failure_text=&limit=5# **So basically the `last-N-failed` builder never did what it was supposed to do.** The source of this bug is setting `test_type` before invoking the `__init__` method of the parent class (`MTR`) in: ``` class FetchTestData(MTR): def __init__(self, mtrDbPool, test_type, **kwargs): self.mtrDbPool = mtrDbPool self.test_type = test_type super().__init__(dbpool=mtrDbPool, **kwargs) ``` Some will guess right, `MTR` defines `test_type` as `None` (the default). --- common_factories.py | 55 +++++++++++++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/common_factories.py b/common_factories.py index a95f6acf..5512cb5a 100644 --- a/common_factories.py +++ b/common_factories.py @@ -1,10 +1,14 @@ import os import re +from sqlalchemy import text +from sqlalchemy.exc import OperationalError from twisted.internet import defer +from twisted.python import log from buildbot.plugins import steps, util from buildbot.process import results +from buildbot.process.buildstep import BuildStep from buildbot.process.factory import BuildFactory from buildbot.process.properties import Property from buildbot.steps.mtrlogobserver import MTR @@ -52,27 +56,51 @@ # * run for view protocol and sanitizers # * how to do it for install/upgrade tests? # -class FetchTestData(MTR): - def __init__(self, mtrDbPool, test_type, **kwargs): - self.mtrDbPool = mtrDbPool +class FetchTestData(BuildStep): + def __init__(self, test_type, **kwargs): self.test_type = test_type - super().__init__(dbpool=mtrDbPool, **kwargs) + super().__init__(**kwargs) - @defer.inlineCallbacks def get_tests_for_type(self, branch, typ, limit): scale = 20 - query = f""" + inner_limit = int(limit * scale) + outer_limit = int(limit) + + q = text( + """ select concat(test_name, ',', test_variant) from - (select id, test_name, test_variant - from test_failure join test_run on (test_run_id=id) - where branch='{branch}' and typ='{typ}' - order by test_run_id desc limit {limit*scale}) x + (select id, test_name, test_variant + from test_failure join test_run on (test_run_id=id) + where branch=:branch and typ=:typ + order by test_run_id desc + limit :inner_limit) x group by test_name, test_variant - order by max(id) desc limit {limit} + order by max(id) desc + limit :outer_limit """ - tests = yield self.runQueryWithRetry(query) - return list(t[0] for t in tests) + ) + + def thd(conn): + try: + res = conn.execute( + q, + { + "branch": branch, + "typ": typ, + "inner_limit": inner_limit, + "outer_limit": outer_limit, + }, + ) + except OperationalError as e: + log.err(f"[FetchTestData] DB query failed: {e}") + # An empty list will allow for all configured suites to run + # Better this than an immense crash + return [] + + return [row[0] for row in res.fetchall()] + + return self.master.db.pool.do(thd) # Runs in a Thread and returns a Deferred @defer.inlineCallbacks def run(self): @@ -613,7 +641,6 @@ def getTests(props): f.addStep( FetchTestData( name=f"Get last N failed {typ} tests", - mtrDbPool=mtrDbPool, test_type=typ, ) )