From ac1bb06c45886a31d6b61bd3f062e18f28900be1 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Mon, 4 Jun 2018 13:55:59 -0700 Subject: [PATCH] Fix zuul-web sql connections A recent change to scope sql connections to tenants was incorrect because it assumed zuul-web would have access to a tenant's layout, but that is only in the zuul-scheduler's memory. Instead, this change causes zuul-web to perform an RPC call to the scheduler to ask which connection to use before performing sql queries. This slipped through testing because the zuul web test fixture used the same connection registry object as the scheduler, and therefore *was* able to access data which normally would not be available to it. This updates the zuul web test fixture to use a separate connection registry. Also, because zuul-web doesn't need any other kinds of connections at the moment, add a facility to allow it to only load sql connections, so that it doesn't do anything with the gerrit or github drivers. Change-Id: Iac6d8c6960f4a31eb2f78511664b704758e41687 --- tests/base.py | 16 +++++++++++++--- tests/unit/test_github_driver.py | 2 +- tests/unit/test_streaming.py | 4 ++-- tests/unit/test_web.py | 2 +- tests/unit/test_web_urls.py | 2 +- zuul/cmd/__init__.py | 4 ++-- zuul/cmd/web.py | 6 +++++- zuul/driver/github/githubconnection.py | 8 ++++++-- zuul/lib/connections.py | 12 +++++++++++- zuul/rpclistener.py | 11 +++++++++++ zuul/web/__init__.py | 10 ++++++++-- 11 files changed, 61 insertions(+), 16 deletions(-) diff --git a/tests/base.py b/tests/base.py index f46fa5279c..36c521cf00 100755 --- a/tests/base.py +++ b/tests/base.py @@ -62,6 +62,8 @@ import tests.fakegithub import zuul.driver.gerrit.gerritsource as gerritsource import zuul.driver.gerrit.gerritconnection as gerritconnection import zuul.driver.github.githubconnection as githubconnection +import zuul.driver.github +import zuul.driver.sql import zuul.scheduler import zuul.executor.server import zuul.executor.client @@ -1904,10 +1906,14 @@ class WebProxyFixture(fixtures.Fixture): class ZuulWebFixture(fixtures.Fixture): - def __init__(self, gearman_server_port, connections, info=None): + def __init__(self, gearman_server_port, config, info=None): super(ZuulWebFixture, self).__init__() self.gearman_server_port = gearman_server_port - self.connections = connections + self.connections = zuul.lib.connections.ConnectionRegistry() + self.connections.configure( + config, + include_drivers=[zuul.driver.sql.SQLDriver, + zuul.driver.github.GithubDriver]) if info is None: self.info = zuul.model.WebInfo() else: @@ -1921,7 +1927,7 @@ class ZuulWebFixture(fixtures.Fixture): info=self.info, connections=self.connections) self.web.start() - self.addCleanup(self.web.stop) + self.addCleanup(self.stop) self.host = 'localhost' # Wait until web server is started @@ -1933,6 +1939,10 @@ class ZuulWebFixture(fixtures.Fixture): except ConnectionRefusedError: pass + def stop(self): + self.web.stop() + self.connections.stop() + class MySQLSchemaFixture(fixtures.Fixture): def setUp(self): diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index acc8010c5d..432df9dcf2 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -827,7 +827,7 @@ class TestGithubWebhook(ZuulTestCase): # Start the web server self.web = self.useFixture( ZuulWebFixture(self.gearman_server.port, - self.connections)) + self.config)) host = '127.0.0.1' # Wait until web server is started diff --git a/tests/unit/test_streaming.py b/tests/unit/test_streaming.py index 30231d1feb..a81750af2a 100644 --- a/tests/unit/test_streaming.py +++ b/tests/unit/test_streaming.py @@ -233,7 +233,7 @@ class TestStreaming(tests.base.AnsibleZuulTestCase): # Start the web server web = self.useFixture( ZuulWebFixture(self.gearman_server.port, - self.connections)) + self.config)) # Start the finger streamer daemon streamer = zuul.lib.log_streamer.LogStreamer( @@ -315,7 +315,7 @@ class TestStreaming(tests.base.AnsibleZuulTestCase): # Start the web server web = self.useFixture( ZuulWebFixture(self.gearman_server.port, - self.connections)) + self.config)) # Start the finger streamer daemon streamer = zuul.lib.log_streamer.LogStreamer( diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index 7fe12ee06f..7169c67826 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -53,7 +53,7 @@ class BaseTestWeb(ZuulTestCase): self.web = self.useFixture( ZuulWebFixture( self.gearman_server.port, - self.connections, + self.config, info=zuul.model.WebInfo.fromConfig(self.zuul_ini_config))) self.executor_server.hold_jobs_in_build = True diff --git a/tests/unit/test_web_urls.py b/tests/unit/test_web_urls.py index 532c7edd6c..fbf4b0f5d5 100644 --- a/tests/unit/test_web_urls.py +++ b/tests/unit/test_web_urls.py @@ -30,7 +30,7 @@ class TestWebURLs(ZuulTestCase): super(TestWebURLs, self).setUp() self.web = self.useFixture( ZuulWebFixture(self.gearman_server.port, - self.connections)) + self.config)) def _get(self, port, uri): url = "http://localhost:{}{}".format(port, uri) diff --git a/zuul/cmd/__init__.py b/zuul/cmd/__init__.py index ee73386f09..6f1babd0b6 100755 --- a/zuul/cmd/__init__.py +++ b/zuul/cmd/__init__.py @@ -150,9 +150,9 @@ class ZuulApp(object): logging_config = logconfig.ServerLoggingConfig(server=section) logging_config.apply() - def configure_connections(self, source_only=False): + def configure_connections(self, source_only=False, include_drivers=None): self.connections = zuul.lib.connections.ConnectionRegistry() - self.connections.configure(self.config, source_only) + self.connections.configure(self.config, source_only, include_drivers) class ZuulDaemonApp(ZuulApp, metaclass=abc.ABCMeta): diff --git a/zuul/cmd/web.py b/zuul/cmd/web.py index 36c4eded18..c1ce80dac7 100755 --- a/zuul/cmd/web.py +++ b/zuul/cmd/web.py @@ -20,6 +20,8 @@ import sys import zuul.cmd import zuul.model import zuul.web +import zuul.driver.sql +import zuul.driver.github from zuul.lib.config import get_default @@ -87,7 +89,9 @@ class WebServer(zuul.cmd.ZuulDaemonApp): self.setup_logging('web', 'log_config') self.log = logging.getLogger("zuul.WebServer") - self.configure_connections() + self.configure_connections( + include_drivers=[zuul.driver.sql.SQLDriver, + zuul.driver.github.GithubDriver]) try: self._run() diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index ab779576f8..002f91988d 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -508,8 +508,12 @@ class GithubConnection(BaseConnection): self.gearman_worker.start() def onStop(self): - self.gearman_worker.stop() - self._stop_event_connector() + # TODO(jeblair): remove this check which is here only so that + # zuul-web can call connections.stop to shut down the sql + # connection. + if hasattr(self, 'gearman_worker'): + self.gearman_worker.stop() + self._stop_event_connector() def _start_event_connector(self): self.github_event_connector = GithubEventConnector(self) diff --git a/zuul/lib/connections.py b/zuul/lib/connections.py index 1320e4cf15..2ab8cf64f5 100644 --- a/zuul/lib/connections.py +++ b/zuul/lib/connections.py @@ -80,7 +80,7 @@ class ConnectionRegistry(object): for driver in self.drivers.values(): driver.stop() - def configure(self, config, source_only=False): + def configure(self, config, source_only=False, include_drivers=None): # Register connections from the config connections = OrderedDict() @@ -109,6 +109,16 @@ class ConnectionRegistry(object): if source_only and not isinstance(driver, SourceInterface): continue + # Zuul web needs only the SQL driver, accomodate that here: + if include_drivers is not None: + found = False + for driver_class in include_drivers: + if isinstance(driver, driver_class): + found = True + break + if not found: + continue + connection = driver.getConnection(con_name, con_config) connections[con_name] = connection diff --git a/zuul/rpclistener.py b/zuul/rpclistener.py index 32d12af1bc..d2d2880578 100644 --- a/zuul/rpclistener.py +++ b/zuul/rpclistener.py @@ -58,6 +58,7 @@ class RPCListener(object): self.worker.registerFunction("zuul:get_running_jobs") self.worker.registerFunction("zuul:get_job_log_stream_address") self.worker.registerFunction("zuul:tenant_list") + self.worker.registerFunction("zuul:tenant_sql_connection") self.worker.registerFunction("zuul:status_get") self.worker.registerFunction("zuul:job_list") self.worker.registerFunction("zuul:key_get") @@ -320,6 +321,16 @@ class RPCListener(object): 'queue': queue_size}) job.sendWorkComplete(json.dumps(output)) + def handle_tenant_sql_connection(self, job): + args = json.loads(job.arguments) + sql_driver = self.sched.connections.drivers['sql'] + conn = sql_driver.tenant_connections.get(args['tenant']) + if conn: + name = conn.connection_name + else: + name = '' + job.sendWorkComplete(json.dumps(name)) + def handle_status_get(self, job): args = json.loads(job.arguments) output = self.sched.formatStatusJSON(args.get("tenant")) diff --git a/zuul/web/__init__.py b/zuul/web/__init__.py index 6e3857000d..a93e38f27d 100755 --- a/zuul/web/__init__.py +++ b/zuul/web/__init__.py @@ -241,9 +241,15 @@ class ZuulWebAPI(object): uuid=None, job_name=None, voting=None, node_name=None, result=None, limit=50, skip=0): sql_driver = self.zuulweb.connections.drivers['sql'] - connection = sql_driver.tenant_connections.get(tenant) - if not connection: + + # Ask the scheduler which sql connection to use for this tenant + job = self.rpc.submitJob('zuul:tenant_sql_connection', + {'tenant': tenant}) + connection_name = json.loads(job.data[0]) + + if not connection_name: raise Exception("Unable to find connection for tenant %s" % tenant) + connection = self.zuulweb.connections.connections[connection_name] args = { 'buildset_filters': {'tenant': [tenant]},