From 46e48d7b97ed9de0f426ab890c8317872b7fc2a2 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Sat, 12 May 2018 17:33:56 -0700 Subject: [PATCH] Move SQL web handler to driver The only rest API endpoint that uses sql queries is /api/tenant/{tenant}/builds. There's no connection in there, which means it doesn't make sense for that to be attached to a sql connection (which is currently the case). Moreover, it doesn't make sense for *every* tenant's endpoint to be attached to the *same* connection. In other words, the current situation only allows for a single sql connection system-wide, even if someone is using different connections per tenant. Moving the handler for the endpoint into the sql driver means that it can dispatch the query to the appropriate connection for a given tenant (since a tenant is always implied by the REST endpoint). Moreover, the *rest* of the system actually allows multiple connections within a single tenant, and we should support that here, but I don't immediately have a solution of how to handle pagination across queries that span multiple connections. This is an improvement in that it is now tenant-scoped, but it's not ideal. This also removes the (undocumented!) sql_connection_name config file option. It also uses the tenant name from the path to constructe the query so that it always includes the correct tenant (this eliminates the inadvertant ability for one tenant to query another tenant's builds). The internal API here isn't great, but it will get cleaned up in the next patch which converts to cherrypy. Change-Id: Ie1f19f0b392d4c010ef43dc6220ff1c8667f5a4a --- etc/zuul.conf-sample | 1 - tests/base.py | 6 ++- tests/unit/test_github_driver.py | 3 +- tests/unit/test_streaming.py | 6 ++- tests/unit/test_web.py | 3 +- tests/unit/test_web_urls.py | 13 ++--- zuul/cmd/web.py | 1 + zuul/driver/sql/__init__.py | 63 ++++++++++++++++++++++++ zuul/driver/sql/sqlconnection.py | 82 ++------------------------------ zuul/web/__init__.py | 6 +++ zuul/web/handler.py | 7 --- 11 files changed, 94 insertions(+), 97 deletions(-) diff --git a/etc/zuul.conf-sample b/etc/zuul.conf-sample index 72c53fd86a..de9cf00473 100644 --- a/etc/zuul.conf-sample +++ b/etc/zuul.conf-sample @@ -38,7 +38,6 @@ trusted_rw_paths=/opt/zuul-logs listen_address=127.0.0.1 port=9000 static_cache_expiry=0 -;sql_connection_name=mydatabase status_url=https://zuul.example.com/status [connection gerrit] diff --git a/tests/base.py b/tests/base.py index 3356500634..76e91a7673 100755 --- a/tests/base.py +++ b/tests/base.py @@ -1867,16 +1867,18 @@ class WebProxyFixture(fixtures.Fixture): class ZuulWebFixture(fixtures.Fixture): - def __init__(self, gearman_server_port): + def __init__(self, gearman_server_port, connections): super(ZuulWebFixture, self).__init__() self.gearman_server_port = gearman_server_port + self.connections = connections def _setUp(self): # Start the web server self.web = zuul.web.ZuulWeb( listen_address='127.0.0.1', listen_port=0, gear_server='127.0.0.1', gear_port=self.gearman_server_port, - info=zuul.model.WebInfo()) + info=zuul.model.WebInfo(), + _connections=self.connections) loop = asyncio.new_event_loop() loop.set_debug(True) ws_thread = threading.Thread(target=self.web.run, args=(loop,)) diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 288137bd7b..09b20fd1be 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -789,7 +789,8 @@ class TestGithubWebhook(ZuulTestCase): self.web = zuul.web.ZuulWeb( listen_address='127.0.0.1', listen_port=0, gear_server='127.0.0.1', gear_port=self.gearman_server.port, - connections=[self.fake_github]) + connections=[self.fake_github], + _connections=self.connections) loop = asyncio.new_event_loop() loop.set_debug(True) ws_thread = threading.Thread(target=self.web.run, args=(loop,)) diff --git a/tests/unit/test_streaming.py b/tests/unit/test_streaming.py index b90ef015a7..56b3488811 100644 --- a/tests/unit/test_streaming.py +++ b/tests/unit/test_streaming.py @@ -282,7 +282,8 @@ class TestStreaming(tests.base.AnsibleZuulTestCase): web_server = zuul.web.ZuulWeb( listen_address='::', listen_port=0, gear_server='127.0.0.1', gear_port=self.gearman_server.port, - static_path=tempfile.gettempdir()) + static_path=tempfile.gettempdir(), + _connections=self.connections) loop = asyncio.new_event_loop() loop.set_debug(True) ws_thread = threading.Thread(target=web_server.run, args=(loop,)) @@ -372,7 +373,8 @@ class TestStreaming(tests.base.AnsibleZuulTestCase): web_server = zuul.web.ZuulWeb( listen_address='::', listen_port=0, gear_server='127.0.0.1', gear_port=self.gearman_server.port, - static_path=tempfile.gettempdir()) + static_path=tempfile.gettempdir(), + _connections=self.connections) loop = asyncio.new_event_loop() loop.set_debug(True) ws_thread = threading.Thread(target=web_server.run, args=(loop,)) diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index a157db2f47..33c3a8e544 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -63,7 +63,8 @@ class BaseTestWeb(ZuulTestCase): listen_address='127.0.0.1', listen_port=0, gear_server='127.0.0.1', gear_port=self.gearman_server.port, info=zuul.model.WebInfo.fromConfig(self.zuul_ini_config), - connections=self.connections.connections.values() + connections=self.connections.connections.values(), + _connections=self.connections ) loop = asyncio.new_event_loop() loop.set_debug(True) diff --git a/tests/unit/test_web_urls.py b/tests/unit/test_web_urls.py index 8e372fb79e..532c7edd6c 100644 --- a/tests/unit/test_web_urls.py +++ b/tests/unit/test_web_urls.py @@ -23,13 +23,14 @@ from tests.base import ZuulTestCase, WebProxyFixture from tests.base import ZuulWebFixture -class TestWebURLs(object): +class TestWebURLs(ZuulTestCase): tenant_config_file = 'config/single-tenant/main.yaml' def setUp(self): super(TestWebURLs, self).setUp() self.web = self.useFixture( - ZuulWebFixture(self.gearman_server.port)) + ZuulWebFixture(self.gearman_server.port, + self.connections)) def _get(self, port, uri): url = "http://localhost:{}{}".format(port, uri) @@ -60,7 +61,7 @@ class TestWebURLs(object): self._get(self.port, link) -class TestDirect(TestWebURLs, ZuulTestCase): +class TestDirect(TestWebURLs): # Test directly accessing the zuul-web server with no proxy def setUp(self): super(TestDirect, self).setUp() @@ -70,7 +71,7 @@ class TestDirect(TestWebURLs, ZuulTestCase): self._crawl('/t/tenant-one/status.html') -class TestWhiteLabel(TestWebURLs, ZuulTestCase): +class TestWhiteLabel(TestWebURLs): # Test a zuul-web behind a whitelabel proxy (i.e., what # zuul.openstack.org does). def setUp(self): @@ -85,7 +86,7 @@ class TestWhiteLabel(TestWebURLs, ZuulTestCase): self._crawl('/status.html') -class TestWhiteLabelAPI(TestWebURLs, ZuulTestCase): +class TestWhiteLabelAPI(TestWebURLs): # Test a zuul-web behind a whitelabel proxy (i.e., what # zuul.openstack.org does). def setUp(self): @@ -103,7 +104,7 @@ class TestWhiteLabelAPI(TestWebURLs, ZuulTestCase): self.assertEqual('tenant-one', info['info']['tenant']) -class TestSuburl(TestWebURLs, ZuulTestCase): +class TestSuburl(TestWebURLs): # Test a zuul-web mounted on a suburl (i.e., what software factory # does). def setUp(self): diff --git a/zuul/cmd/web.py b/zuul/cmd/web.py index cfaa2b8f02..19057a74a9 100755 --- a/zuul/cmd/web.py +++ b/zuul/cmd/web.py @@ -55,6 +55,7 @@ class WebServer(zuul.cmd.ZuulDaemonApp): params['ssl_cert'] = get_default(self.config, 'gearman', 'ssl_cert') params['ssl_ca'] = get_default(self.config, 'gearman', 'ssl_ca') + params['_connections'] = self.connections params['connections'] = [] # Validate config here before we spin up the ZuulWeb object for conn_name, connection in self.connections.connections.items(): diff --git a/zuul/driver/sql/__init__.py b/zuul/driver/sql/__init__.py index 3748e47746..2340e10825 100644 --- a/zuul/driver/sql/__init__.py +++ b/zuul/driver/sql/__init__.py @@ -12,6 +12,10 @@ # License for the specific language governing permissions and limitations # under the License. +import logging +from aiohttp import web +import urllib.parse + from zuul.driver import Driver, ConnectionInterface, ReporterInterface from zuul.driver.sql import sqlconnection from zuul.driver.sql import sqlreporter @@ -19,6 +23,28 @@ from zuul.driver.sql import sqlreporter class SQLDriver(Driver, ConnectionInterface, ReporterInterface): name = 'sql' + log = logging.getLogger("zuul.SQLDriver") + + def __init__(self): + self.tenant_connections = {} + + def reconfigure(self, tenant): + # NOTE(corvus): This stores the connection of the first + # reporter seen for each tenant; we should figure out how to + # support multiple connections for a tenant (how do we deal + # with pagination of queries across multiple connections), or + # otherwise, require there only be one connection in a tenant. + if tenant.name in self.tenant_connections: + del self.tenant_connections[tenant.name] + for pipeline in tenant.layout.pipelines.values(): + reporters = (pipeline.start_actions + pipeline.success_actions + + pipeline.failure_actions + + pipeline.merge_failure_actions) + for reporter in reporters: + if not isinstance(reporter, sqlreporter.SQLReporter): + continue + self.tenant_connections[tenant.name] = reporter.connection + return def registerScheduler(self, scheduler): self.sched = scheduler @@ -31,3 +57,40 @@ class SQLDriver(Driver, ConnectionInterface, ReporterInterface): def getReporterSchema(self): return sqlreporter.getSchema() + + # TODO(corvus): these are temporary, remove after cherrypy conversion + def setEventLoop(self, event_loop): + self.event_loop = event_loop + + async def handleRequest(self, request): + tenant_name = request.match_info["tenant"] + connection = self.tenant_connections.get(tenant_name) + if not connection: + return + try: + args = { + 'buildset_filters': {}, + 'build_filters': {}, + 'limit': 50, + 'skip': 0, + 'tenant': tenant_name, + } + for k, v in urllib.parse.parse_qsl(request.rel_url.query_string): + if k in ("project", "pipeline", "change", "branch", + "patchset", "ref", "newrev"): + args['buildset_filters'].setdefault(k, []).append(v) + elif k in ("uuid", "job_name", "voting", "node_name", + "result"): + args['build_filters'].setdefault(k, []).append(v) + elif k in ("limit", "skip"): + args[k] = int(v) + else: + raise ValueError("Unknown parameter %s" % k) + data = await connection.get_builds(args, self.event_loop) + resp = web.json_response(data) + resp.headers['Access-Control-Allow-Origin'] = '*' + except Exception as e: + self.log.exception("Jobs exception:") + resp = web.json_response({'error_description': 'Internal error'}, + status=500) + return resp diff --git a/zuul/driver/sql/sqlconnection.py b/zuul/driver/sql/sqlconnection.py index cdd0c5adca..06ba0bb8c0 100644 --- a/zuul/driver/sql/sqlconnection.py +++ b/zuul/driver/sql/sqlconnection.py @@ -15,19 +15,15 @@ import asyncio import logging -from aiohttp import web import alembic import alembic.command import alembic.config import sqlalchemy as sa import sqlalchemy.pool from sqlalchemy.sql import select -import urllib.parse import voluptuous from zuul.connection import BaseConnection -from zuul.lib.config import get_default -from zuul.web.handler import BaseTenantWebHandler BUILDSET_TABLE = 'zuul_buildset' BUILD_TABLE = 'zuul_build' @@ -127,53 +123,13 @@ class SQLConnection(BaseConnection): return zuul_buildset_table, zuul_build_table - def getWebHandlers(self, zuul_web, info): - info.capabilities.job_history = True - return [ - SqlWebHandler(self, zuul_web, 'GET', 'builds'), - ] - - def validateWebConfig(self, config, connections): - sql_conn_name = get_default(config, 'web', 'sql_connection_name') - if sql_conn_name: - # The config wants a specific sql connection. Check the whole - # list of connections to make sure it can be satisfied. - sql_conn = connections.connections.get(sql_conn_name) - if not sql_conn: - raise Exception( - "Couldn't find sql connection '%s'" % sql_conn_name) - if self.connection_name == sql_conn.connection_name: - return True - else: - # Check to see if there is more than one connection - conn_objects = [c for c in connections.connections.values() - if isinstance(c, SQLConnection)] - if len(conn_objects) > 1: - raise Exception("Multiple sql connection found, " - "set the sql_connection_name option " - "in zuul.conf [web] section") - return True - def onStop(self): self.log.debug("Stopping SQL connection %s" % self.connection_name) self.engine.dispose() - -class SqlWebHandler(BaseTenantWebHandler): - log = logging.getLogger("zuul.web.SqlHandler") - filters = ("project", "pipeline", "change", "branch", "patchset", "ref", - "result", "uuid", "job_name", "voting", "node_name", "newrev") - - def __init__(self, connection, zuul_web, method, path): - super(SqlWebHandler, self).__init__( - connection=connection, zuul_web=zuul_web, method=method, path=path) - - def setEventLoop(self, event_loop): - self.event_loop = event_loop - def query(self, args): - build = self.connection.zuul_build_table - buildset = self.connection.zuul_buildset_table + build = self.zuul_build_table + buildset = self.zuul_buildset_table query = select([ buildset.c.project, buildset.c.branch, @@ -201,12 +157,12 @@ class SqlWebHandler(BaseTenantWebHandler): return query.limit(args['limit']).offset(args['skip']).order_by( build.c.id.desc()) - async def get_builds(self, args): + async def get_builds(self, args, event_loop): """Return a list of build""" builds = [] - with self.connection.engine.begin() as conn: + with self.engine.begin() as conn: query = self.query(args) - query_task = self.event_loop.run_in_executor( + query_task = event_loop.run_in_executor( None, conn.execute, query @@ -229,34 +185,6 @@ class SqlWebHandler(BaseTenantWebHandler): builds.append(build) return builds - async def handleRequest(self, request): - try: - args = { - 'buildset_filters': {}, - 'build_filters': {}, - 'limit': 50, - 'skip': 0, - } - for k, v in urllib.parse.parse_qsl(request.rel_url.query_string): - if k in ("tenant", "project", "pipeline", "change", "branch", - "patchset", "ref", "newrev"): - args['buildset_filters'].setdefault(k, []).append(v) - elif k in ("uuid", "job_name", "voting", "node_name", - "result"): - args['build_filters'].setdefault(k, []).append(v) - elif k in ("limit", "skip"): - args[k] = int(v) - else: - raise ValueError("Unknown parameter %s" % k) - data = await self.get_builds(args) - resp = web.json_response(data) - resp.headers['Access-Control-Allow-Origin'] = '*' - except Exception as e: - self.log.exception("Jobs exception:") - resp = web.json_response({'error_description': 'Internal error'}, - status=500) - return resp - def getSchema(): sql_connection = voluptuous.Any(str, voluptuous.Schema(dict)) diff --git a/zuul/web/__init__.py b/zuul/web/__init__.py index f634fa9988..fcbbe136d2 100755 --- a/zuul/web/__init__.py +++ b/zuul/web/__init__.py @@ -265,6 +265,7 @@ class ZuulWeb(object): ssl_key=None, ssl_cert=None, ssl_ca=None, static_cache_expiry=3600, connections=None, + _connections=None, info=None, static_path=None): self.start_time = time.time() @@ -287,6 +288,7 @@ class ZuulWeb(object): for connection in connections: self._connection_handlers.extend( connection.getWebHandlers(self, self.info)) + self.connections = _connections self._plugin_routes.extend(self._connection_handlers) async def _handleWebsocket(self, request): @@ -349,6 +351,7 @@ class ZuulWeb(object): thread event loop is used. This should be supplied if ZuulWeb is run within a separate (non-main) thread. """ + sql_driver = self.connections.drivers['sql'] routes = [ ('GET', '/api/info', self._handleRootInfo), ('GET', '/api/tenants', self._handleTenantsRequest), @@ -361,6 +364,8 @@ class ZuulWeb(object): self._handleWebsocket), ('GET', '/api/tenant/{tenant}/key/{project:.*}.pub', self._handleKeyRequest), + ('GET', '/api/tenant/{tenant}/builds', + sql_driver.handleRequest), ] static_routes = [ @@ -385,6 +390,7 @@ class ZuulWeb(object): self.event_loop = loop self.log_streaming_handler.setEventLoop(loop) self.gearman_handler.setEventLoop(loop) + sql_driver.setEventLoop(loop) for handler in self._connection_handlers: if hasattr(handler, 'setEventLoop'): diff --git a/zuul/web/handler.py b/zuul/web/handler.py index e30c0f5a48..ff631b2b85 100644 --- a/zuul/web/handler.py +++ b/zuul/web/handler.py @@ -31,13 +31,6 @@ class BaseWebHandler(object, metaclass=abc.ABCMeta): """Process a web request.""" -class BaseTenantWebHandler(BaseWebHandler): - - def __init__(self, connection, zuul_web, method, path): - super(BaseTenantWebHandler, self).__init__( - connection, zuul_web, method, '/api/tenant/{tenant}/' + path) - - class BaseDriverWebHandler(BaseWebHandler): def __init__(self, connection, zuul_web, method, path):