From 6c751892e8d92620f159c045bf9096405b6b1a61 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 8 Oct 2021 09:13:12 -0700 Subject: [PATCH] Add gitlab disable_connection_pool option Under adverse network conditions, it may be necessary to disable connection pooling entirely in order to achive reliable interaction with the remote server. Add an option for that. Change-Id: I336fab1197a8fb6e038db24277b6279013224bb0 --- doc/source/reference/drivers/gitlab.rst | 7 +++ ...able-connection-pool-0ad02ef8028d4994.yaml | 6 +++ .../fixtures/zuul-gitlab-driver-no-pool.conf | 48 +++++++++++++++++++ tests/unit/test_gitlab_driver.py | 41 ++++++++++++++++ zuul/driver/gitlab/gitlabconnection.py | 35 ++++++++++---- zuul/lib/config.py | 7 +++ 6 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 releasenotes/notes/gitlab-disable-connection-pool-0ad02ef8028d4994.yaml create mode 100644 tests/fixtures/zuul-gitlab-driver-no-pool.conf diff --git a/doc/source/reference/drivers/gitlab.rst b/doc/source/reference/drivers/gitlab.rst index 5e1c156685..c747bbbd3f 100644 --- a/doc/source/reference/drivers/gitlab.rst +++ b/doc/source/reference/drivers/gitlab.rst @@ -114,6 +114,13 @@ The supported options in ``zuul.conf`` connections are: TCP connection keepalive timeout; ``0`` disables. + .. attr:: disable_connection_pool + :default: false + + Connection pooling improves performance and resource usage under + normal circumstances, but in adverse network conditions it can + be problematic. Set this to ``true`` to disable. + Trigger Configuration diff --git a/releasenotes/notes/gitlab-disable-connection-pool-0ad02ef8028d4994.yaml b/releasenotes/notes/gitlab-disable-connection-pool-0ad02ef8028d4994.yaml new file mode 100644 index 0000000000..4aadd4ee30 --- /dev/null +++ b/releasenotes/notes/gitlab-disable-connection-pool-0ad02ef8028d4994.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + It is now possible to disable connection pooling in the Gitlab + driver with :attr:`.disable_connection_pool`. + This option may be useful under adverse network conditions. diff --git a/tests/fixtures/zuul-gitlab-driver-no-pool.conf b/tests/fixtures/zuul-gitlab-driver-no-pool.conf new file mode 100644 index 0000000000..7cf788e1ac --- /dev/null +++ b/tests/fixtures/zuul-gitlab-driver-no-pool.conf @@ -0,0 +1,48 @@ +[gearman] +server=127.0.0.1 + +[web] +status_url=http://zuul.example.com/status/#{change.number},{change.patchset} + +[merger] +git_dir=/tmp/zuul-test/git +git_user_email=zuul@example.com +git_user_name=zuul + +[executor] +git_dir=/tmp/zuul-test/executor-git + +[connection gitlab] +driver=gitlab +server=gitlab +api_token=0000000000000000000000000000000000000000 +disable_connection_pool=true + +[database] +dburi=$MYSQL_FIXTURE_DBURI$ + +[connection gitlab2] +driver=gitlab +server=gitlabtwo +api_token=2222 +cloneurl=http://myusername:2222@gitlab + +[connection gitlab3] +driver=gitlab +server=gitlabthree +api_token_name=tokenname3 +api_token=3333 +cloneurl=http://myusername:2222@gitlabthree + +[connection gitlab4] +driver=gitlab +server=gitlabfour +api_token_name=tokenname4 +api_token=444 + +[connection gitlab5] +driver=gitlab +server=gitlabfive +api_token_name=tokenname5 +api_token=555 +cloneurl=http://gitlabfivvve diff --git a/tests/unit/test_gitlab_driver.py b/tests/unit/test_gitlab_driver.py index d395216a84..1444f9a9be 100644 --- a/tests/unit/test_gitlab_driver.py +++ b/tests/unit/test_gitlab_driver.py @@ -1089,3 +1089,44 @@ class TestGitlabUnprotectedBranches(ZuulTestCase): old_sha=new_sha, new_sha='0' * 40, removed_files=['zuul.yaml']) + + +class TestGitlabDriverNoPool(ZuulTestCase): + config_file = 'zuul-gitlab-driver-no-pool.conf' + + @simple_layout('layouts/basic-gitlab.yaml', driver='gitlab') + def test_merge_request_opened(self): + + description = "This is the\nMR description." + A = self.fake_gitlab.openFakeMergeRequest( + 'org/project', 'master', 'A', description=description) + self.fake_gitlab.emitEvent( + A.getMergeRequestOpenedEvent(), project='org/project') + self.waitUntilSettled() + + self.assertEqual('SUCCESS', + self.getJobFromHistory('project-test1').result) + + self.assertEqual('SUCCESS', + self.getJobFromHistory('project-test2').result) + + job = self.getJobFromHistory('project-test2') + zuulvars = job.parameters['zuul'] + self.assertEqual(str(A.number), zuulvars['change']) + self.assertEqual(str(A.sha), zuulvars['patchset']) + self.assertEqual('master', zuulvars['branch']) + self.assertEquals(f'{self.fake_gitlab._test_baseurl}/' + 'org/project/merge_requests/1', + zuulvars['items'][0]['change_url']) + self.assertEqual(zuulvars["message"], strings.b64encode(description)) + self.assertEqual(2, len(self.history)) + self.assertEqual(2, len(A.notes)) + self.assertEqual( + A.notes[0]['body'], "Starting check jobs.") + self.assertThat( + A.notes[1]['body'], + MatchesRegex(r'.*project-test1.*SUCCESS.*', re.DOTALL)) + self.assertThat( + A.notes[1]['body'], + MatchesRegex(r'.*project-test2.*SUCCESS.*', re.DOTALL)) + self.assertTrue(A.approved) diff --git a/zuul/driver/gitlab/gitlabconnection.py b/zuul/driver/gitlab/gitlabconnection.py index 7589474c8c..d409815622 100644 --- a/zuul/driver/gitlab/gitlabconnection.py +++ b/zuul/driver/gitlab/gitlabconnection.py @@ -32,6 +32,7 @@ from zuul.connection import CachedBranchConnection, ZKChangeCacheMixin from zuul.web.handler import BaseWebController from zuul.lib.http import ZuulHTTPAdapter from zuul.lib.logutil import get_annotated_logger +from zuul.lib.config import any_to_bool from zuul.exceptions import MergeFailure from zuul.model import Branch, Project, Ref, Tag from zuul.driver.gitlab.gitlabmodel import GitlabTriggerEvent, MergeRequest @@ -255,18 +256,34 @@ class GitlabAPIClientException(Exception): class GitlabAPIClient(): log = logging.getLogger("zuul.GitlabAPIClient") - def __init__(self, baseurl, api_token, keepalive): - self.session = requests.Session() - retry = urllib3.util.Retry(total=8, - backoff_factor=0.1) - adapter = ZuulHTTPAdapter(keepalive=keepalive, - max_retries=retry) - self.session.mount(baseurl, adapter) + def __init__(self, baseurl, api_token, keepalive, disable_pool): + self._session = None + self._orig_baseurl = baseurl self.baseurl = '%s/api/v4' % baseurl self.api_token = api_token + self.keepalive = keepalive + self.disable_pool = disable_pool self.headers = {'Authorization': 'Bearer %s' % ( self.api_token)} + if not self.disable_pool: + self._session = self._makeSession() + + def _makeSession(self): + session = requests.Session() + retry = urllib3.util.Retry(total=8, + backoff_factor=0.1) + adapter = ZuulHTTPAdapter(keepalive=self.keepalive, + max_retries=retry) + session.mount(self._orig_baseurl, adapter) + return session + + @property + def session(self): + if self.disable_pool: + return self._makeSession() + return self._session + def _manage_error(self, data, code, url, verb, zuul_event_id=None): if code < 400: return @@ -442,9 +459,11 @@ class GitlabConnection(ZKChangeCacheMixin, CachedBranchConnection): self.api_token = self.connection_config.get( 'api_token', '') self.keepalive = self.connection_config.get('keepalive', 60) + self.disable_pool = any_to_bool(self.connection_config.get( + 'disable_connection_pool', False)) self.gl_client = GitlabAPIClient(self.baseurl, self.api_token, - self.keepalive) + self.keepalive, self.disable_pool) self.sched = None self.source = driver.getSource(self) diff --git a/zuul/lib/config.py b/zuul/lib/config.py index 867e8a789e..a8bf2ead2d 100644 --- a/zuul/lib/config.py +++ b/zuul/lib/config.py @@ -28,3 +28,10 @@ def get_default(config, section, option, default=None, expand_user=False): if expand_user and value: return os.path.expanduser(value) return value + + +def any_to_bool(val): + val = str(val) + if val.lower() in ('1', 'on', 'yes', 'true'): + return True + return False