Skip to content

Commit e78482d

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` is always sent as `None` to `get_tests_for_type` ``` 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 e78482d

1 file changed

Lines changed: 42 additions & 16 deletions

File tree

common_factories.py

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,16 @@
11
import os
22
import re
33

4-
from twisted.internet import defer
5-
64
from buildbot.plugins import steps, util
75
from buildbot.process import results
6+
from buildbot.process.buildstep import BuildStep
87
from buildbot.process.factory import BuildFactory
98
from buildbot.process.properties import Property
109
from buildbot.steps.mtrlogobserver import MTR
10+
from sqlalchemy import text
11+
from sqlalchemy.exc import OperationalError
12+
from twisted.internet import defer
13+
from twisted.python import log
1114

1215
# Local
1316
from constants import MTR_ENV, SAVED_PACKAGE_BRANCHES, TEST_TYPE_TO_MTR_ARG
@@ -52,27 +55,51 @@
5255
# * run for view protocol and sanitizers
5356
# * how to do it for install/upgrade tests?
5457
#
55-
class FetchTestData(MTR):
56-
def __init__(self, mtrDbPool, test_type, **kwargs):
57-
self.mtrDbPool = mtrDbPool
58+
class FetchTestData(BuildStep):
59+
def __init__(self, test_type, **kwargs):
5860
self.test_type = test_type
59-
super().__init__(dbpool=mtrDbPool, **kwargs)
61+
super().__init__(**kwargs)
6062

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

77104
@defer.inlineCallbacks
78105
def run(self):
@@ -613,7 +640,6 @@ def getTests(props):
613640
f.addStep(
614641
FetchTestData(
615642
name=f"Get last N failed {typ} tests",
616-
mtrDbPool=mtrDbPool,
617643
test_type=typ,
618644
)
619645
)

0 commit comments

Comments
 (0)