Skip to content

Commit 5db69c7

Browse files
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` although sent correctly to get_tests_for_type is then replaced with None by the parent class `__init__`. ``` 2026-02-02 16:11:40+0000 [-] [FetchTestData] master_branch: type=<class 'str'> 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=<class 'NoneType'> 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).
1 parent d98e784 commit 5db69c7

1 file changed

Lines changed: 41 additions & 14 deletions

File tree

common_factories.py

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
import os
22
import re
33

4+
from sqlalchemy import text
5+
from sqlalchemy.exc import OperationalError
46
from twisted.internet import defer
7+
from twisted.python import log
58

69
from buildbot.plugins import steps, util
710
from buildbot.process import results
11+
from buildbot.process.buildstep import BuildStep
812
from buildbot.process.factory import BuildFactory
913
from buildbot.process.properties import Property
1014
from buildbot.steps.mtrlogobserver import MTR
@@ -52,27 +56,51 @@
5256
# * run for view protocol and sanitizers
5357
# * how to do it for install/upgrade tests?
5458
#
55-
class FetchTestData(MTR):
56-
def __init__(self, mtrDbPool, test_type, **kwargs):
57-
self.mtrDbPool = mtrDbPool
59+
class FetchTestData(BuildStep):
60+
def __init__(self, test_type, **kwargs):
5861
self.test_type = test_type
59-
super().__init__(dbpool=mtrDbPool, **kwargs)
62+
super().__init__(**kwargs)
6063

61-
@defer.inlineCallbacks
6264
def get_tests_for_type(self, branch, typ, limit):
6365
scale = 20
64-
query = f"""
66+
inner_limit = int(limit * scale)
67+
outer_limit = int(limit)
68+
69+
q = text(
70+
"""
6571
select concat(test_name, ',', test_variant)
6672
from
67-
(select id, test_name, test_variant
68-
from test_failure join test_run on (test_run_id=id)
69-
where branch='{branch}' and typ='{typ}'
70-
order by test_run_id desc limit {limit*scale}) x
73+
(select id, test_name, test_variant
74+
from test_failure join test_run on (test_run_id=id)
75+
where branch=:branch and typ=:typ
76+
order by test_run_id desc
77+
limit :inner_limit) x
7178
group by test_name, test_variant
72-
order by max(id) desc limit {limit}
79+
order by max(id) desc
80+
limit :outer_limit
7381
"""
74-
tests = yield self.runQueryWithRetry(query)
75-
return list(t[0] for t in tests)
82+
)
83+
84+
def thd(conn):
85+
try:
86+
res = conn.execute(
87+
q,
88+
{
89+
"branch": branch,
90+
"typ": typ,
91+
"inner_limit": inner_limit,
92+
"outer_limit": outer_limit,
93+
},
94+
)
95+
except OperationalError as e:
96+
log.err(f"[FetchTestData] DB query failed: {e}")
97+
# An empty list will allow for all configured suites to run
98+
# Better this than an immense crash
99+
return []
100+
101+
return [row[0] for row in res.fetchall()]
102+
103+
return self.master.db.pool.do(thd) # Runs in a Thread and returns a Deferred
76104

77105
@defer.inlineCallbacks
78106
def run(self):
@@ -613,7 +641,6 @@ def getTests(props):
613641
f.addStep(
614642
FetchTestData(
615643
name=f"Get last N failed {typ} tests",
616-
mtrDbPool=mtrDbPool,
617644
test_type=typ,
618645
)
619646
)

0 commit comments

Comments
 (0)