PR#2186: distgit: frontend: failed import to distgit caused a package to fail - copr/copr

copr / copr

#2186 distgit: frontend: failed import to distgit caused a package to fail

Closed 2 years ago by praiskup. Opened 2 years ago by schlupov.

copr/ schlupov/copr distgit_import into main

distgit: frontend: failed import to distgit caused a package to fail

Silvie Chlupova • 2 years ago

c9a7550

common/copr_common/request.py

file modified

+4 -6

		`@@ -18,13 +18,11 @@`
		`# Prolong the sleep time before asking frontend again`
		`SLEEP_INCREMENT_TIME = 5`

		`- # Reasonable timeout for requests that block the client`
		`- TIMEOUT = 2*60`
		`-`
		`- def __init__(self, auth=None, log=None, try_indefinitely=False):`
		`+ def __init__(self, auth=None, log=None, try_indefinitely=False, timeout=2 * 60):`
		`self.auth = auth`
		`self.log = log`
		`self.try_indefinitely = try_indefinitely`
		`+ self.timeout = timeout`

		`def get(self, url, **kwargs):`
		`"""`
		`@@ -60,7 +58,7 @@`
		`req_args.update(kwargs)`
		`req_args["auth"] = auth`
		`req_args["headers"] = headers`
		`- req_args["timeout"] = self.TIMEOUT / 5`
		`+ req_args["timeout"] = self.timeout / 5`
		`method = method.lower()`
		`if method in ['post', 'put']:`
		`req_args['data'] = json.dumps(data)`
		`@@ -95,7 +93,7 @@`
		`"""`
		`sleep = self.SLEEP_INCREMENT_TIME`
		`start = time.time()`
		`- stop = start + self.TIMEOUT`
		`+ stop = start + self.timeout`

		`i = 0`
		`while True:`

common/python-copr-common.spec

file modified

+1 -1

		`@@ -16,7 +16,7 @@`
		`%endif`

		`Name: python-copr-common`
		`- Version: 0.14`
		`+ Version: 0.14.1.dev`
		`Release: 1%{?dist}`
		`Summary: Python code used by Copr`

dist-git/copr-dist-git.spec

file modified

+1 -1

		`@@ -1,4 +1,4 @@`
		`- %global copr_common_version 0.13.1.dev`
		`+ %global copr_common_version 0.14.1.dev`

		`Name: copr-dist-git`
		`Version: 0.54`

dist-git/copr_dist_git/helpers.py

file modified

+6 -7

		`@@ -5,13 +5,12 @@`

		`from oslo_concurrency import lockutils`
		`from setproctitle import getproctitle, setproctitle`
		`- from .exceptions import FileDownloadException, RunCommandException, SrpmQueryException`
		`+ from copr_common.request import SafeRequest, RequestError`
		`+ from munch import Munch`
		`+ from .exceptions import FileDownloadException, RunCommandException`

		`from contextlib import contextmanager`
		`from configparser import ConfigParser`
		`- from munch import Munch`
		`- from requests import get`
		`- from functools import wraps`

		`log = logging.getLogger(__name__)`
		`LOCK_PATH = "/var/lock/copr-dist-git"`
		`@@ -159,9 +158,9 @@`
		`"""`
		`log.debug("Downloading {0}".format(url))`
		`try:`
		`- log.info(url)`
		`- r = get(url, stream=True, verify=False)`
		`- except Exception as e:`
		`+ request = SafeRequest(log=log, timeout=10 * 60)`
		`+ r = request.send(url, method='get')`
		`+ except RequestError as e:`
		`raise FileDownloadException(str(e))`

		`if 200 <= r.status_code < 400:`

schlupov commented 2 years ago

Fixes: #2103

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

csdiff-pylint : FAILURE in 2m 25s

rebased onto 559ea754a4f17e60288fb5b25cb60750ca9b7883

2 years ago

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

csdiff-pylint : FAILURE in 1m 48s

rebased onto 7d79fb8ce3317f09a06bbcebf04dcde731361575

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 38s

praiskup commented on line 25 of dist-git/copr_dist_git/helpers.py 2 years ago

This probably deserves the try_indefinitely option.

praiskup commented on line 25 of dist-git/copr_dist_git/helpers.py 2 years ago

Or rather a longer timeout than just 2 minutes, I think.

praiskup commented on line 26 of dist-git/copr_dist_git/helpers.py 2 years ago

Thinking about this ... isn't it actually better to bubble the exception up? That way we don't post back to the Frontend at all, and we can re-take the task by another worker in the future.

praiskup commented on line 7 of frontend/coprs_frontend/coprs/views/backend_ns/backend_general.py 2 years ago

This is weird. We actually failed to import the package, but we still continue (we just warn). I would probably avoid this at all...

praiskup commented on line 26 of dist-git/copr_dist_git/helpers.py 2 years ago

Wrong, scratch this. We never did this. This import error logic a big mess, and it isn't to be fixed in this PR.

praiskup commented 2 years ago

IMO we should add the SafeRequest(try_indefinitely=true) for all the requests to frontend. It is pity if we successfully import the file, but we fail to report back to frontend.

For the Backend SRPM download, I'm convinced that we should at least prolong the timeout to say 10 minutes (multiple attempts for extraordinarily large files, not sure... WDYT?)

rebased onto 8a6c37c66a95ca1cd8686c26467c506e62872d7e

2 years ago

rebased onto 8d9cf31d9677c07f817ae5ad38a2f4eb0b9fe8fb

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 5m 29s

praiskup commented on line 6 of common/copr_common/request.py 2 years ago

Please don't set this globally - we need a new init() argument which will override this

praiskup commented on line 23 of dist-git/copr_dist_git/helpers.py 2 years ago

The verify=False before was a weird choice btw.

rebased onto 6936d2842a3db175a66eadb8338466db957cbcfa

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 34s

praiskup commented 2 years ago

Can you just formally bump the common version to the "dev" variant, together with "%copr_common_version in copr-dist-git.spec? Otherwise looks good.

rebased onto c9a7550

2 years ago

zuul commented 2 years ago

Build succeeded.