PR#413: Improve error reporting for pulp composes - odcs

odcs

#413 Improve error reporting for pulp composes

Merged 4 years ago by lsedlar. Opened 4 years ago by onosek.

onosek/odcs pulp_reporting into master

Improve error reporting for pulp composes

Ondrej Nosek • 4 years ago

2675658

client/contrib/odcs

file modified

+1 -2

		`@@ -169,7 +169,6 @@`
		`"--target-dir", "--flag")`


		`-`
		`create_tag_parser = subparsers.add_parser(`
		`'create-tag', help='Create new compose from Koji tag.')`
		`create_tag_parser.set_defaults(command='create-tag')`
		`@@ -377,7 +376,7 @@`
		`result = client.get_compose(args.compose_id)`
		`else:`
		`print("Unknown command %s" % args.command)`
		`- except requests.exceptions.HTTPError as e:`
		`+ except requests.exceptions.HTTPError:`
		`# error message gets printed in ODCS class.`
		`sys.exit(-1)`

server/odcs/server/pulp.py

file modified

+13 -6

		`@@ -45,12 +45,19 @@`
		`@retry(wait_on=requests.exceptions.RequestException)`
		`def _rest_post(self, endpoint, post_data):`
		`query_data = json.dumps(post_data)`
		`- r = requests.post(`
		`- '{0}{1}'.format(self.rest_api_root, endpoint.lstrip('/')),`
		`- query_data,`
		`- auth=(self.username, self.password),`
		`- timeout=conf.net_timeout,`
		`- )`
		`+ try:`
		`+ r = requests.post(`
		`+ '{0}{1}'.format(self.rest_api_root, endpoint.lstrip('/')),`
		`+ query_data,`
		`+ auth=(self.username, self.password),`
		`+ timeout=conf.net_timeout,`
		`+ )`
		`+ except requests.exceptions.RequestException as e:`
		`+ # also catches ConnectTimeout, ConnectionError`
		`+ # change message of the catched exception and re-raise`
		`+ msg = "Pulp connection has failed: {}".format(e.args)`
		`+ raise requests.exceptions.RequestException(msg)`
		`+`
		`r.raise_for_status()`
		`return r.json()`

server/odcs/server/utils.py

file modified

+2 -2

		`@@ -57,8 +57,8 @@`
		`logger.warn("Exception %r raised from %r. Retry in %rs",`
		`e, function, interval)`
		`time.sleep(interval)`
		`- if (time.time() - start) >= timeout:`
		`- raise # This re-raises the last exception.`
		`+ if (time.time() - start) >= timeout:`
		`+ raise # This re-raises the last exception.`
lsedlar commented 4 years ago Does this actually change something? The `try` block ends with `return`, so this code is never invoked after successful run. And if we get to the `except` branch, does it matter if this block is in `except` block or right after it?
onosek commented 4 years ago If `raise` is called outside of the block without parameter, it doesn't re-raise intended exception as it was described in comment. It raises RuntimeException.
		`return inner`
		`return wrapper`

onosek commented 4 years ago

Catches exception during Pulp connection and reports it with additional
description.

Fixes: #385
JIRA: RHELCMP-696

Signed-off-by: Ondrej Nosek onosek@redhat.com

lsedlar commented on line 8 of server/odcs/server/utils.py 4 years ago

Does this actually change something? The try block ends with return, so this code is never invoked after successful run. And if we get to the except branch, does it matter if this block is in except block or right after it?

onosek commented on line 8 of server/odcs/server/utils.py 4 years ago

If raise is called outside of the block without parameter, it doesn't re-raise intended exception as it was described in comment. It raises RuntimeException.

onosek commented 4 years ago

testable by:

try:
    raise requests.exceptions.ConnectionError()
except requests.exceptions.RequestException:
    pass
raise

lsedlar commented 4 years ago

Wow, I didn't know that. Based on what I found on raise, it should work, but your example demonstrates it doesn't.

lsedlar commented 4 years ago

:thumbsup:

Pull-Request has been merged by lsedlar

4 years ago

onosek commented 4 years ago

Yes, it was surprising to me too.

onosek commented 4 years ago

I do have other question to @retry decorator: in some cases request's get/post is called with timeout parameter. It is the same timeout parameter as in @retry method. I suspect that if get/post is terminated by ConnectTimeout exception. It doesn't have enough time to be retried. Maybe interval should be used instead?