From cce5771bb4e6da38a0ecc9de27ea2c12d807ba52 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Sat, 2 Nov 2024 09:19:20 +0200 Subject: [PATCH 1/4] scripts: Fix error handling in copydb script --- master/buildbot/scripts/copydb.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/master/buildbot/scripts/copydb.py b/master/buildbot/scripts/copydb.py index 9f3e6a8a4e59..215f9bd0d085 100644 --- a/master/buildbot/scripts/copydb.py +++ b/master/buildbot/scripts/copydb.py @@ -55,12 +55,19 @@ def print_log(*args, **kwargs): ): config_file = base.getConfigFileFromTac(config['basedir']) + if not config_file: + return 1 + with base.captureErrors( config_module.ConfigErrors, f"Unable to load '{config_file}' from '{config['basedir']}':" ): master_src_cfg = base.loadConfig(config, config_file) master_dst_cfg = base.loadConfig(config, config_file) - master_dst_cfg.db["db_url"] = config["destination_url"] + + if not master_src_cfg or not master_dst_cfg: + return 1 + + master_dst_cfg.db["db_url"] = config["destination_url"] print_log(f"Copying database ({master_src_cfg.db['db_url']}) to ({config['destination_url']})") From 622b02f61e7ff500bd972d35ba67b40e7e9ea97e Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Sat, 2 Nov 2024 09:19:21 +0200 Subject: [PATCH 2/4] test: Add a way to specify integration test basedir --- master/buildbot/test/util/integration.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/master/buildbot/test/util/integration.py b/master/buildbot/test/util/integration.py index 0211ba98cd52..d789a91947e4 100644 --- a/master/buildbot/test/util/integration.py +++ b/master/buildbot/test/util/integration.py @@ -63,16 +63,15 @@ def __init__(self): self.is_master_shutdown = False @async_to_deferred - async def create_master(self, case, reactor, config_dict): + async def create_master(self, case, reactor, config_dict, basedir=None): """ Create a started ``BuildMaster`` with the given configuration. """ - basedir = FilePath(case.mktemp()) - basedir.createDirectory() + if basedir is None: + basedir = case.mktemp() + os.makedirs(basedir, exist_ok=True) config_dict['buildbotNetUsageData'] = None - self.master = BuildMaster( - basedir.path, reactor=reactor, config_loader=DictLoader(config_dict) - ) + self.master = BuildMaster(basedir, reactor=reactor, config_loader=DictLoader(config_dict)) if 'db_url' not in config_dict: config_dict['db_url'] = 'sqlite://' @@ -278,6 +277,7 @@ async def setup_master( reactor, config_dict, proto='null', + basedir=None, start_worker=True, **worker_kwargs, ): @@ -308,7 +308,7 @@ async def setup_master( ] config_dict['protocols'] = config_protocols - await self.create_master(case, reactor, config_dict) + await self.create_master(case, reactor, config_dict, basedir=basedir) self.master_config_dict = config_dict case.assertFalse(stop.called, "startService tried to stop the reactor; check logs") @@ -393,10 +393,16 @@ class RunMasterBase(unittest.TestCase): skip = "buildbot-worker package is not installed" @defer.inlineCallbacks - def setup_master(self, config_dict, startWorker=True, **worker_kwargs): + def setup_master(self, config_dict, startWorker=True, basedir=None, **worker_kwargs): self.tested_master = TestedRealMaster() yield self.tested_master.setup_master( - self, reactor, config_dict, proto=self.proto, start_worker=startWorker, **worker_kwargs + self, + reactor, + config_dict, + proto=self.proto, + basedir=basedir, + start_worker=startWorker, + **worker_kwargs, ) self.master = self.tested_master.master self.master_config_dict = self.tested_master.master_config_dict From 0a3c6a645461d1eb65fb78960b030e67fcab4fec Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Sat, 2 Nov 2024 09:19:22 +0200 Subject: [PATCH 3/4] scripts: Upgrade copydb script to sqlalchemy 2.0 The script did not have any tests, thus when Buildbot migrated to sqlalchemy 2.0 the script got broken. --- master/buildbot/scripts/copydb.py | 2 +- newsfragments/copydb.bugfix | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 newsfragments/copydb.bugfix diff --git a/master/buildbot/scripts/copydb.py b/master/buildbot/scripts/copydb.py index 215f9bd0d085..cbb7eebdd632 100644 --- a/master/buildbot/scripts/copydb.py +++ b/master/buildbot/scripts/copydb.py @@ -156,7 +156,7 @@ def thd_write(conn): rows_queue.task_done() def thd_read(conn): - q = sa.select([sa.sql.func.count()]).select_from(table) + q = sa.select(sa.sql.func.count()).select_from(table) total_count[0] = conn.execute(q).scalar() result = conn.execute(sa.select(table)) diff --git a/newsfragments/copydb.bugfix b/newsfragments/copydb.bugfix new file mode 100644 index 000000000000..2baa2dbe56bd --- /dev/null +++ b/newsfragments/copydb.bugfix @@ -0,0 +1 @@ +Fixed ``copydb`` script when SQLAlchemy 2.x is used. From ce7610c5b3e0c369f8fdd320e1a3ed7da6a922d5 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Sat, 2 Nov 2024 09:19:23 +0200 Subject: [PATCH 4/4] test: Add unit and integration tests for copydb script --- .../buildbot/test/unit/scripts/test_copydb.py | 176 ++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 master/buildbot/test/unit/scripts/test_copydb.py diff --git a/master/buildbot/test/unit/scripts/test_copydb.py b/master/buildbot/test/unit/scripts/test_copydb.py new file mode 100644 index 000000000000..b61fed3e0b29 --- /dev/null +++ b/master/buildbot/test/unit/scripts/test_copydb.py @@ -0,0 +1,176 @@ +# This file is part of Buildbot. Buildbot is free software: you can +# redistribute it and/or modify it under the terms of the GNU General Public +# License as published by the Free Software Foundation, version 2. +# +# This program is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS +# FOR A PARTICULAR PURPOSE. See the GNU General Public License for more +# details. +# +# You should have received a copy of the GNU General Public License along with +# this program; if not, write to the Free Software Foundation, Inc., 51 +# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# +# Copyright Buildbot Team Members + +import os +import platform +import textwrap + +from twisted.internet import defer +from twisted.trial import unittest + +from buildbot.config import BuilderConfig +from buildbot.plugins import schedulers +from buildbot.process.factory import BuildFactory +from buildbot.scripts import copydb +from buildbot.steps.master import MasterShellCommand +from buildbot.test.reactor import TestReactorMixin +from buildbot.test.util import db +from buildbot.test.util import dirs +from buildbot.test.util import misc +from buildbot.test.util.integration import RunMasterBase +from buildbot.util.twisted import async_to_deferred + + +def get_script_config(destination_url='sqlite://', **kwargs): + config = { + "quiet": False, + "basedir": os.path.abspath('basedir'), + 'destination_url': destination_url, + } + config.update(kwargs) + return config + + +def write_buildbot_tac(path): + with open(path, "w", encoding='utf-8') as f: + f.write( + textwrap.dedent(""" + from twisted.application import service + application = service.Application('buildmaster') + """) + ) + + +class TestCopyDb(misc.StdoutAssertionsMixin, dirs.DirsMixin, TestReactorMixin, unittest.TestCase): + def setUp(self): + self.setup_test_reactor(auto_tear_down=False) + self.setUpDirs('basedir') + write_buildbot_tac(os.path.join('basedir', 'buildbot.tac')) + self.setUpStdoutAssertions() + + @defer.inlineCallbacks + def tearDown(self): + self.tearDownDirs() + yield self.tear_down_test_reactor() + + def create_master_cfg(self, db_url='sqlite://', extraconfig=""): + with open(os.path.join('basedir', 'master.cfg'), "w", encoding='utf-8') as f: + f.write( + textwrap.dedent(f""" + from buildbot.plugins import * + c = BuildmasterConfig = dict() + c['db_url'] = {db_url!r} + c['buildbotNetUsageData'] = None + c['multiMaster'] = True # don't complain for no builders + {extraconfig} + """) + ) + + @async_to_deferred + async def test_not_basedir(self): + res = await copydb._copy_database_in_reactor(get_script_config(basedir='doesntexist')) + self.assertEqual(res, 1) + tac_path = os.path.join('doesntexist', 'buildbot.tac') + self.assertInStdout(f'error reading \'{tac_path}\': No such file or directory') + + @async_to_deferred + async def test_bad_config(self): + res = await copydb._copy_database_in_reactor(get_script_config(basedir='basedir')) + self.assertEqual(res, 1) + master_cfg_path = os.path.abspath(os.path.join('basedir', 'master.cfg')) + self.assertInStdout(f'configuration file \'{master_cfg_path}\' does not exist') + + @async_to_deferred + async def test_bad_config2(self): + self.create_master_cfg(extraconfig="++++ # syntaxerror") + res = await copydb._copy_database_in_reactor(get_script_config(basedir='basedir')) + self.assertEqual(res, 1) + self.assertInStdout("encountered a SyntaxError while parsing config file:") + # config logs an error via log.err, we must eat it or trial will + # complain + self.flushLoggedErrors() + + +class TestCopyDbRealDb(misc.StdoutAssertionsMixin, RunMasterBase, dirs.DirsMixin, TestReactorMixin): + INITIAL_DB_URL = 'sqlite:///tmp.sqlite' + + def setUp(self): + self.setUpDirs('basedir') + # self.setUpStdoutAssertions() # comment out to see stdout from script + write_buildbot_tac(os.path.join('basedir', 'buildbot.tac')) + + def tearDown(self): + self.tearDownDirs() + + @defer.inlineCallbacks + def create_master_config(self): + f = BuildFactory() + cmd = "dir" if platform.system() in ("Windows", "Microsoft") else "ls" + f.addStep(MasterShellCommand(cmd)) + + config_dict = { + 'builders': [ + BuilderConfig( + name="testy", + workernames=["local1"], + factory=f, + ), + ], + 'schedulers': [schedulers.ForceScheduler(name="force", builderNames=["testy"])], + # Disable checks about missing scheduler. + 'multiMaster': True, + 'db_url': self.INITIAL_DB_URL, + } + yield self.setup_master(config_dict, basedir='basedir') + builder_id = yield self.master.data.updates.findBuilderId('testy') + + return builder_id + + def create_master_config_file(self, db_url): + with open(os.path.join('basedir', 'master.cfg'), "w", encoding='utf-8') as f: + f.write( + textwrap.dedent(f""" + from buildbot.plugins import * + c = BuildmasterConfig = dict() + c['db_url'] = {db_url!r} + c['buildbotNetUsageData'] = None + c['multiMaster'] = True # don't complain for no builders + """) + ) + + def resolve_db_url(self): + # test may use mysql or pg if configured in env + envkey = "BUILDBOT_TEST_DB_URL" + if envkey not in os.environ or os.environ[envkey] == 'sqlite://': + return "sqlite:///" + os.path.abspath(os.path.join("basedir", "dest.sqlite")) + return os.environ[envkey] + + @async_to_deferred + async def test_full(self): + await self.create_master_config() + + await self.doForceBuild() + await self.doForceBuild() + await self.doForceBuild() + + await self.tested_master.shutdown() + + self.create_master_config_file(self.INITIAL_DB_URL) + + dest_db_url = db.resolve_test_index_in_db_url(self.resolve_db_url()) + + script_config = get_script_config(destination_url=dest_db_url) + res = await copydb._copy_database_in_reactor(script_config) + self.assertEqual(res, 0)