From c119168ff21bc68a4781dd02f6ab16417bc377c0 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 5 Oct 2021 15:08:56 -0700 Subject: [PATCH] Add TCP keepalive to gitlab The gitlab driver uses requests to interact with gitlab's API. By default, requests uses a connection pool to cache and re-use HTTP connections. If the gitlab server is across a network segment with, say, a poor NAT implementation which silently drops idle connections from its map, then Zuul may not notice until it attempts to make a request on one of these connections and receives a read timeout. To avoid this, use TCP keepalives on these HTTP requests by default. This is configured to send a keepalive packet by default every 60 seconds. This should avoid sending too much extra traffic, while improving the reliability for all users. The default of 60 seconds was chosen to match the SSH keepalive value we set for gerrit connections (for much the same reason). If this works well, we can use this approach with other requests-based drivers (and at this point, every driver uses requests to at least some degree). Change-Id: I86ce064c6ec9325e2ae9db516778058050696ef6 --- doc/source/reference/drivers/gitlab.rst | 8 +++++- tests/base.py | 8 +++--- zuul/driver/gitlab/gitlabconnection.py | 11 +++++--- zuul/lib/http.py | 36 +++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 7 deletions(-) create mode 100644 zuul/lib/http.py diff --git a/doc/source/reference/drivers/gitlab.rst b/doc/source/reference/drivers/gitlab.rst index 4bbd21a495..5e1c156685 100644 --- a/doc/source/reference/drivers/gitlab.rst +++ b/doc/source/reference/drivers/gitlab.rst @@ -101,7 +101,7 @@ The supported options in ``zuul.conf`` connections are: .. attr:: cloneurl :default: {baseurl} - + Omit to clone using http(s) or set to ``ssh://git@{server}``. If **api_token_name** is set and **cloneurl** is either omitted or is set without credentials, **cloneurl** will be modified to use credentials @@ -109,6 +109,12 @@ The supported options in ``zuul.conf`` connections are: If **cloneurl** is defined with credentials, it will be used as is, without modification from the driver. + .. attr:: keepalive + :default: 60 + + TCP connection keepalive timeout; ``0`` disables. + + Trigger Configuration --------------------- diff --git a/tests/base.py b/tests/base.py index d0c5fb16ce..c09cd99c97 100644 --- a/tests/base.py +++ b/tests/base.py @@ -1927,7 +1927,7 @@ class FakeGitlabConnection(gitlabconnection.GitlabConnection): connection_config) self.merge_requests = changes_db self.gl_client = FakeGitlabAPIClient( - self.baseurl, self.api_token, merge_requests_db=changes_db) + self.baseurl, self.api_token, 60, merge_requests_db=changes_db) self.rpcclient = rpcclient self.upstream_root = upstream_root self.mr_number = 0 @@ -2024,8 +2024,10 @@ FakeBranch = namedtuple('Branch', ('name', 'protected')) class FakeGitlabAPIClient(gitlabconnection.GitlabAPIClient): log = logging.getLogger("zuul.test.FakeGitlabAPIClient") - def __init__(self, baseurl, api_token, merge_requests_db={}): - super(FakeGitlabAPIClient, self).__init__(baseurl, api_token) + def __init__(self, baseurl, api_token, keepalive, + merge_requests_db={}): + super(FakeGitlabAPIClient, self).__init__( + baseurl, api_token, keepalive) self.merge_requests = merge_requests_db self.fake_repos = defaultdict(lambda: IterableList('name')) self.community_edition = False diff --git a/zuul/driver/gitlab/gitlabconnection.py b/zuul/driver/gitlab/gitlabconnection.py index 11edd1094c..9a6cbc6361 100644 --- a/zuul/driver/gitlab/gitlabconnection.py +++ b/zuul/driver/gitlab/gitlabconnection.py @@ -30,6 +30,7 @@ from typing import List, Optional 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.exceptions import MergeFailure from zuul.model import Branch, Project, Ref, Tag @@ -254,11 +255,12 @@ class GitlabAPIClientException(Exception): class GitlabAPIClient(): log = logging.getLogger("zuul.GitlabAPIClient") - def __init__(self, baseurl, api_token): + def __init__(self, baseurl, api_token, keepalive): self.session = requests.Session() retry = urllib3.util.Retry(total=8, backoff_factor=0.1) - adapter = requests.adapters.HTTPAdapter(max_retries=retry) + adapter = ZuulHTTPAdapter(keepalive=keepalive, + max_retries=retry) self.session.mount(baseurl, adapter) self.baseurl = '%s/api/v4' % baseurl self.api_token = api_token @@ -435,7 +437,10 @@ class GitlabConnection(ZKChangeCacheMixin, CachedBranchConnection): 'api_token_name', '') self.api_token = self.connection_config.get( 'api_token', '') - self.gl_client = GitlabAPIClient(self.baseurl, self.api_token) + self.keepalive = self.connection_config.get('keepalive', 60) + + self.gl_client = GitlabAPIClient(self.baseurl, self.api_token, + self.keepalive) self.sched = None self.source = driver.getSource(self) diff --git a/zuul/lib/http.py b/zuul/lib/http.py new file mode 100644 index 0000000000..1b74e59427 --- /dev/null +++ b/zuul/lib/http.py @@ -0,0 +1,36 @@ +# Copyright 2021 Acme Gating, LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import socket + +from requests.adapters import HTTPAdapter +from urllib3.connection import HTTPConnection + + +class ZuulHTTPAdapter(HTTPAdapter): + "A requests HTTPAdapter class which supports TCP keepalives" + def __init__(self, *args, **kw): + self.keepalive = int(kw.pop('keepalive', 0)) + super().__init__(*args, **kw) + + def init_poolmanager(self, *args, **kw): + if self.keepalive: + ka = self.keepalive + kw['socket_options'] = HTTPConnection.default_socket_options + [ + (socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1), + (socket.IPPROTO_TCP, socket.TCP_KEEPIDLE, ka), + (socket.IPPROTO_TCP, socket.TCP_KEEPINTVL, ka), + (socket.IPPROTO_TCP, socket.TCP_KEEPCNT, 2), + ] + return super().init_poolmanager(*args, **kw)