From 1a0601d29794cec1f735a10208364d11958c41ec Mon Sep 17 00:00:00 2001 From: Ondrej Nosek Date: Aug 03 2023 08:33:29 +0000 Subject: [PATCH 1/3] Prepare the lookaside cache code for retries These changes should not have an impact on the original functionality. JIRA: RHELCMP-11210 Signed-off-by: Ondrej Nosek --- diff --git a/pyrpkg/lookaside.py b/pyrpkg/lookaside.py index 3efcd88..f94ffdb 100644 --- a/pyrpkg/lookaside.py +++ b/pyrpkg/lookaside.py @@ -163,17 +163,17 @@ class CGILookasideCache(object): url = url.encode('utf-8') self.log.debug("Full url: %s", url) + c = pycurl.Curl() + c.setopt(pycurl.URL, url) + c.setopt(pycurl.HTTPHEADER, ['Pragma:']) + c.setopt(pycurl.NOPROGRESS, False) + c.setopt(pycurl.PROGRESSFUNCTION, self.print_progress) + c.setopt(pycurl.OPT_FILETIME, True) + c.setopt(pycurl.LOW_SPEED_LIMIT, 1000) + c.setopt(pycurl.LOW_SPEED_TIME, 300) + c.setopt(pycurl.FOLLOWLOCATION, 1) with open(outfile, 'wb') as f: - c = pycurl.Curl() - c.setopt(pycurl.URL, url) - c.setopt(pycurl.HTTPHEADER, ['Pragma:']) - c.setopt(pycurl.NOPROGRESS, False) - c.setopt(pycurl.PROGRESSFUNCTION, self.print_progress) - c.setopt(pycurl.OPT_FILETIME, True) c.setopt(pycurl.WRITEDATA, f) - c.setopt(pycurl.LOW_SPEED_LIMIT, 1000) - c.setopt(pycurl.LOW_SPEED_TIME, 300) - c.setopt(pycurl.FOLLOWLOCATION, 1) try: c.perform() tstamp = c.getinfo(pycurl.INFO_FILETIME) @@ -254,29 +254,29 @@ class CGILookasideCache(object): ('%ssum' % self.hashtype, hash), ('filename', filename)] - with io.BytesIO() as buf: - c = pycurl.Curl() - c.setopt(pycurl.URL, self.upload_url) - c.setopt(pycurl.WRITEFUNCTION, buf.write) - c.setopt(pycurl.HTTPPOST, post_data) - c.setopt(pycurl.FOLLOWLOCATION, 1) + c = pycurl.Curl() + c.setopt(pycurl.URL, self.upload_url) + c.setopt(pycurl.HTTPPOST, post_data) + c.setopt(pycurl.FOLLOWLOCATION, 1) - if self.client_cert is not None: - if os.path.exists(self.client_cert): - c.setopt(pycurl.SSLCERT, self.client_cert) - else: - self.log.warning("Missing certificate: %s" - % self.client_cert) + if self.client_cert is not None: + if os.path.exists(self.client_cert): + c.setopt(pycurl.SSLCERT, self.client_cert) + else: + self.log.warning("Missing certificate: %s" + % self.client_cert) - if self.ca_cert is not None: - if os.path.exists(self.ca_cert): - c.setopt(pycurl.CAINFO, self.ca_cert) - else: - self.log.warning("Missing certificate: %s", self.ca_cert) + if self.ca_cert is not None: + if os.path.exists(self.ca_cert): + c.setopt(pycurl.CAINFO, self.ca_cert) + else: + self.log.warning("Missing certificate: %s", self.ca_cert) - c.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_GSSNEGOTIATE) - c.setopt(pycurl.USERPWD, ':') + c.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_GSSNEGOTIATE) + c.setopt(pycurl.USERPWD, ':') + with io.BytesIO() as buf: + c.setopt(pycurl.WRITEFUNCTION, buf.write) try: c.perform() status = c.getinfo(pycurl.RESPONSE_CODE) @@ -341,30 +341,30 @@ class CGILookasideCache(object): ('mtime', str(int(os.stat(filepath).st_mtime))), ] - with io.BytesIO() as buf: - c = pycurl.Curl() - c.setopt(pycurl.URL, self.upload_url) - c.setopt(pycurl.NOPROGRESS, False) - c.setopt(pycurl.PROGRESSFUNCTION, self.print_progress) - c.setopt(pycurl.WRITEFUNCTION, buf.write) - c.setopt(pycurl.HTTPPOST, post_data) - c.setopt(pycurl.FOLLOWLOCATION, 1) + c = pycurl.Curl() + c.setopt(pycurl.URL, self.upload_url) + c.setopt(pycurl.NOPROGRESS, False) + c.setopt(pycurl.PROGRESSFUNCTION, self.print_progress) + c.setopt(pycurl.HTTPPOST, post_data) + c.setopt(pycurl.FOLLOWLOCATION, 1) - if self.client_cert is not None: - if os.path.exists(self.client_cert): - c.setopt(pycurl.SSLCERT, self.client_cert) - else: - self.log.warning("Missing certificate: %s", self.client_cert) + if self.client_cert is not None: + if os.path.exists(self.client_cert): + c.setopt(pycurl.SSLCERT, self.client_cert) + else: + self.log.warning("Missing certificate: %s", self.client_cert) - if self.ca_cert is not None: - if os.path.exists(self.ca_cert): - c.setopt(pycurl.CAINFO, self.ca_cert) - else: - self.log.warning("Missing certificate: %s", self.ca_cert) + if self.ca_cert is not None: + if os.path.exists(self.ca_cert): + c.setopt(pycurl.CAINFO, self.ca_cert) + else: + self.log.warning("Missing certificate: %s", self.ca_cert) - c.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_GSSNEGOTIATE) - c.setopt(pycurl.USERPWD, ':') + c.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_GSSNEGOTIATE) + c.setopt(pycurl.USERPWD, ':') + with io.BytesIO() as buf: + c.setopt(pycurl.WRITEFUNCTION, buf.write) try: c.perform() status = c.getinfo(pycurl.RESPONSE_CODE) From 3a96293d2479a75348f424806028c9b640aff31c Mon Sep 17 00:00:00 2001 From: Ondrej Nosek Date: Aug 22 2023 20:19:56 +0000 Subject: [PATCH 2/3] Lookaside cache operations retries Both upload and download network operations might fail and in this case, a retry mechanism was implemented. In case of failure, there is a delay and another attempt(s). Delays are increasing with every attempt. JIRA: RHELCMP-11210 Signed-off-by: Ondrej Nosek --- diff --git a/pyrpkg/lookaside.py b/pyrpkg/lookaside.py index f94ffdb..01eee4a 100644 --- a/pyrpkg/lookaside.py +++ b/pyrpkg/lookaside.py @@ -14,11 +14,13 @@ way it is done by Fedora, RHEL, and other distributions maintainers. """ +import functools import hashlib import io import logging import os import sys +import time import pycurl import six @@ -31,7 +33,7 @@ from .errors import (AlreadyUploadedError, DownloadError, InvalidHashType, class CGILookasideCache(object): """A class to interact with a CGI-based lookaside cache""" def __init__(self, hashtype, download_url, upload_url, - client_cert=None, ca_cert=None): + client_cert=None, ca_cert=None, attempts=None, delay=None): """Constructor :param str hashtype: The hash algorithm to use for uploads. (e.g 'md5') @@ -45,12 +47,18 @@ class CGILookasideCache(object): use for HTTPS connexions. (e.g if the server certificate is self-signed. It defaults to None, in which case the system CA bundle is used. + :param int attempts: repeat network operations after failure. The param + says how many tries to do. None = single attempt / no-retrying + :param int delay: Initial delay between network operation attempts. + Each attempt doubles the previous delay value. In seconds. """ self.hashtype = hashtype self.download_url = download_url self.upload_url = upload_url self.client_cert = client_cert self.ca_cert = ca_cert + self.attempts = attempts if attempts is not None and attempts > 1 else 1 + self.delay_between_attempts = delay if delay is not None and delay >= 0 else 15 self.log = logging.getLogger(__name__) @@ -170,20 +178,13 @@ class CGILookasideCache(object): c.setopt(pycurl.PROGRESSFUNCTION, self.print_progress) c.setopt(pycurl.OPT_FILETIME, True) c.setopt(pycurl.LOW_SPEED_LIMIT, 1000) - c.setopt(pycurl.LOW_SPEED_TIME, 300) + c.setopt(pycurl.LOW_SPEED_TIME, 60) c.setopt(pycurl.FOLLOWLOCATION, 1) - with open(outfile, 'wb') as f: - c.setopt(pycurl.WRITEDATA, f) - try: - c.perform() - tstamp = c.getinfo(pycurl.INFO_FILETIME) - status = c.getinfo(pycurl.RESPONSE_CODE) - - except Exception as e: - raise DownloadError(e) - finally: - c.close() + # call retry method directly instead of @retry decorator - this approach allows passing + # object's internal variables into the retry method + status, tstamp = self.retry(raises=DownloadError)(self.retry_download)(c, outfile) + c.close() # Get back a new line, after displaying the download progress if sys.stdout.isatty(): @@ -220,13 +221,8 @@ class CGILookasideCache(object): c.setopt(pycurl.NOBODY, True) c.setopt(pycurl.FOLLOWLOCATION, 1) - try: - c.perform() - status = c.getinfo(pycurl.RESPONSE_CODE) - except Exception as e: - raise DownloadError(e) - finally: - c.close() + status = self.retry(raises=DownloadError)(self.retry_remote_file_exists_head)(c) + c.close() if status != 200: self.log.debug('Unavailable file \'%s\' at %s' % (filename, url)) @@ -275,19 +271,8 @@ class CGILookasideCache(object): c.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_GSSNEGOTIATE) c.setopt(pycurl.USERPWD, ':') - with io.BytesIO() as buf: - c.setopt(pycurl.WRITEFUNCTION, buf.write) - try: - c.perform() - status = c.getinfo(pycurl.RESPONSE_CODE) - - except Exception as e: - raise UploadError(e) - - finally: - c.close() - - output = buf.getvalue().strip() + status, output = self.retry(raises=UploadError)(self.retry_remote_file_exists)(c) + c.close() if status != 200: self.raise_upload_error(status) @@ -363,19 +348,8 @@ class CGILookasideCache(object): c.setopt(pycurl.HTTPAUTH, pycurl.HTTPAUTH_GSSNEGOTIATE) c.setopt(pycurl.USERPWD, ':') - with io.BytesIO() as buf: - c.setopt(pycurl.WRITEFUNCTION, buf.write) - try: - c.perform() - status = c.getinfo(pycurl.RESPONSE_CODE) - - except Exception as e: - raise UploadError(e) - - finally: - c.close() - - output = buf.getvalue().strip() + status, output = self.retry(raises=UploadError)(self.retry_upload)(c) + c.close() # Get back a new line, after displaying the download progress if sys.stdout.isatty(): @@ -387,3 +361,66 @@ class CGILookasideCache(object): if output: self.log.debug(output) + + def retry_download(self, curl, outfile): + with open(outfile, 'wb') as f: + curl.setopt(pycurl.WRITEDATA, f) + curl.perform() + tstamp = curl.getinfo(pycurl.INFO_FILETIME) + status = curl.getinfo(pycurl.RESPONSE_CODE) + return status, tstamp + + def retry_remote_file_exists_head(self, curl): + curl.perform() + status = curl.getinfo(pycurl.RESPONSE_CODE) + return status + + def retry_remote_file_exists(self, curl): + with io.BytesIO() as buf: + curl.setopt(pycurl.WRITEFUNCTION, buf.write) + curl.perform() + status = curl.getinfo(pycurl.RESPONSE_CODE) + output = buf.getvalue().strip() + return status, output + + def retry_upload(self, curl): + with io.BytesIO() as buf: + curl.setopt(pycurl.WRITEFUNCTION, buf.write) + curl.perform() + status = curl.getinfo(pycurl.RESPONSE_CODE) + output = buf.getvalue().strip() + return status, output + + def retry(self, attempts=None, delay_between_attempts=None, wait_on=pycurl.error, raises=None): + """A decorator that allows to retry a section of code until success or counter elapses + """ + + def wrapper(function): + @functools.wraps(function) + def inner(*args, **kwargs): + + attempts_all = attempts or self.attempts + attempts_left = attempts_all + delay = delay_between_attempts or self.delay_between_attempts + while attempts_left > 0: + try: + return function(*args, **kwargs) + except wait_on as e: + self.log.warn("Network error: %s" % (e)) + attempts_left -= 1 + self.log.debug("Attempt %d/%d has failed." + % (attempts_all - attempts_left, attempts_all)) + if attempts_left: + self.log.info("The operation will be retried in %ds." % (delay)) + time.sleep(delay) + delay *= 2 + self.log.info("Retrying ...") + else: + if raises is None: + raise # This re-raises the last exception. + else: + raise raises(e) + + return inner + + return wrapper diff --git a/tests/test_lookaside.py b/tests/test_lookaside.py index 35d3499..2fe1bdb 100644 --- a/tests/test_lookaside.py +++ b/tests/test_lookaside.py @@ -175,7 +175,7 @@ class CGILookasideCacheTestCase(unittest.TestCase): return 200 if info == pycurl.RESPONSE_CODE else 0 def mock_perform(): - with open(self.filename) as f: + with open(self.filename, "rb") as f: curlopts[pycurl.WRITEDATA].write(f.read()) def mock_setopt(opt, value): @@ -200,7 +200,7 @@ class CGILookasideCacheTestCase(unittest.TestCase): @mock.patch('pyrpkg.lookaside.pycurl.Curl') def test_download_failed(self, mock_curl): curl = mock_curl.return_value - curl.perform.side_effect = Exception( + curl.perform.side_effect = pycurl.error( 'Could not resolve host: example.com') with open(self.filename, 'wb') as f: @@ -219,7 +219,7 @@ class CGILookasideCacheTestCase(unittest.TestCase): return 500 if info == pycurl.RESPONSE_CODE else 0 def mock_perform(): - with open(self.filename) as f: + with open(self.filename, "rb") as f: curlopts[pycurl.WRITEDATA].write(f.read()) def mock_setopt(opt, value): @@ -424,7 +424,7 @@ class CGILookasideCacheTestCase(unittest.TestCase): @mock.patch('pyrpkg.lookaside.pycurl.Curl') def test_remote_file_exists_check_failed(self, mock_curl): curl = mock_curl.return_value - curl.perform.side_effect = Exception( + curl.perform.side_effect = pycurl.error( 'Could not resolve host: example.com') lc = CGILookasideCache('_', '_', '_') @@ -452,7 +452,7 @@ class CGILookasideCacheTestCase(unittest.TestCase): @mock.patch('pyrpkg.lookaside.pycurl.Curl') def test_remote_file_exists_check_unexpected_error(self, mock_curl): def mock_perform(): - curlopts[pycurl.WRITEFUNCTION]('Something unexpected') + curlopts[pycurl.WRITEFUNCTION](b'Something unexpected') def mock_setopt(opt, value): curlopts[opt] = value @@ -590,7 +590,7 @@ class CGILookasideCacheTestCase(unittest.TestCase): @mock.patch('pyrpkg.lookaside.pycurl.Curl') def test_upload_failed(self, mock_curl): curl = mock_curl.return_value - curl.perform.side_effect = Exception( + curl.perform.side_effect = pycurl.error( 'Could not resolve host: example.com') lc = CGILookasideCache('_', '_', '_') From 08cebe5fae426c11b51e645754b87e4ec5737ef3 Mon Sep 17 00:00:00 2001 From: Ondrej Nosek Date: Aug 22 2023 20:24:01 +0000 Subject: [PATCH 3/3] Make lookaside cache retries configurable The number of attempts for lookaside cache network operations is now configurable - there are new keys 'lookaside_attempts' and 'lookaside_delay' in the configuration. The Former expresses a maximum number of attempts to try the operation. '0' or '1' is for a single try (no-retry). The latter means an initial delay between network operation attempts. Each attempt doubles the previous delay value. In seconds. JIRA: RHELCMP-11210 Signed-off-by: Ondrej Nosek --- diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py index f69f2ce..5928d47 100644 --- a/pyrpkg/__init__.py +++ b/pyrpkg/__init__.py @@ -110,7 +110,8 @@ class Commands(object): build_client, user=None, dist=None, target=None, quiet=False, distgit_namespaced=False, realms=None, lookaside_namespaced=False, - git_excludes=None, results_dir='root', allow_pre_generated_srpm=False): + git_excludes=None, results_dir='root', allow_pre_generated_srpm=False, + lookaside_attempts=None, lookaside_delay=None): """Init the object and some configuration details.""" # Path to operate on, most often pwd @@ -242,6 +243,10 @@ class Commands(object): # A Configuration value used in 'import_srpm' command (comes from the Copr team) # If pre-generated srpms are allowed, don't care specfile is processed by rpmautospec self.allow_pre_generated_srpm = allow_pre_generated_srpm + # number of attempts for lookaside network operations + self.lookaside_attempts = lookaside_attempts + # initial delay between network operation attempts. In seconds. + self.lookaside_delay = lookaside_delay # Define properties here # Properties allow us to "lazy load" various attributes, which also means @@ -262,7 +267,8 @@ class Commands(object): """ return CGILookasideCache( self.lookasidehash, self.lookaside, self.lookaside_cgi, - client_cert=self.cert_file, ca_cert=self.ca_cert) + client_cert=self.cert_file, ca_cert=self.ca_cert, + attempts=self.lookaside_attempts, delay=self.lookaside_delay) @property def path(self): diff --git a/pyrpkg/cli.py b/pyrpkg/cli.py index 1bd7979..16298f0 100644 --- a/pyrpkg/cli.py +++ b/pyrpkg/cli.py @@ -261,7 +261,9 @@ class cliClient(object): realms=realms, lookaside_namespaced=la_namespaced, git_excludes=git_excludes, - results_dir=results_dir + results_dir=results_dir, + lookaside_attempts=self.lookaside_attempts, + lookaside_delay=self.lookaside_delay ) if self.args.repo_name: @@ -3087,3 +3089,33 @@ class cliClient(object): def pre_push_check(self): self.cmd.pre_push_check(self.args.ref) + + @property + def lookaside_attempts(self): + """loads parameter 'lookaside_attempts' from the config file + """ + val = None + if self.config.has_option(self.name, 'lookaside_attempts'): + val = self.config.get(self.name, 'lookaside_attempts') + try: + val = int(val) + except Exception: + self.log.error("Error: The config value 'lookaside_attempts' " + "should be an integer.") + val = None + return val + + @property + def lookaside_delay(self): + """loads parameter 'lookaside_delay' from the config file + """ + val = None + if self.config.has_option(self.name, 'lookaside_delay'): + val = self.config.get(self.name, 'lookaside_delay') + try: + val = int(val) + except Exception: + self.log.error("Error: The config value 'lookaside_delay' " + "should be an integer.") + val = None + return val