libtaskotron/bodhi_utils.py
59–75	please use a more descriptive var name than `i`. Something like `num_retries` or `retries` would be better
70	why not use `e.code != 500` here instead of `<` and `>`?

libtaskotron/bodhi_utils.py
70	That's because Kamil checked it and server sometimes returns 500, sometimes 503. I was not sure whether it can return other errors, so it retries it on all 500 Server errors.

libtaskotron/bodhi_utils.py
70	oh, I see now. sorry - read too quickly

libtaskotron/bodhi_utils.py
46	If you want to make it pretty, you can use ``bodhi_request_max_retries``-times
68	The defensive approach here would be to use `>=` instead of `==`.

libtaskotron/bodhi_utils.py
75	The message is somewhat confusing though. If the server is always unavailable we get: Got HTTP 0 response from server, retrying (1 from 3)... Got HTTP 0 response from server, retrying (2 from 3)... re-raise the exception and that's it, where's (3 from 3)? :) Sure, we try querying the server three times, but the message is confusing. What about changing the message to something like `2 retries remaining`?

libtaskotron/bodhi_utils.py
68	I think that there is off-by-one error. `xrange(1,X)` generates numbers from 1 to X-1, so `retry_num` will never be equal to `max_retries`. I think that running tests confirms it.

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	665	69ccade		Sep 30 2014, 9:17 AM	★	★
Diff 2	668	f297227	Getting rid of the `i` in bodhi_utils, changing name to explicitly state max retries	Oct 6 2014, 2:53 PM	★	★
Diff 3	669	f297227	- fixing doc and retry comparison from comments in review	Oct 6 2014, 3:14 PM	★	★
Diff 4	671	f297227	- changing retry message and comparisons around slightly	Oct 7 2014, 5:54 AM	★	★
Diff 5	672	f297227	- correction of off-by-one error	Oct 7 2014, 7:54 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
90901338f8a1	5615ef9b72e8	3d89534fa96f	Garret Raziel	correction of off-by-one error	Oct 7 2014, 7:53 AM
3d89534fa96f	3c90a9dbd9f3	f29722796aa2	Tim Flink	retry Bodhi request if server returns HTTP 5xx (Show More…)	Oct 7 2014, 7:39 AM

Diff 672

conf/taskotron.yaml.example

Show First 20 Lines • Show All 46 Lines • ▼ Show 20 Line(s)
47	#koji_url: http://koji.fedoraproject.org/kojihub	47	#koji_url: http://koji.fedoraproject.org/kojihub
48		48
49	## URL of repository of all the RPM packages built in Koji	49	## URL of repository of all the RPM packages built in Koji
50	#pkg_url: http://kojipkgs.fedoraproject.org/packages	50	#pkg_url: http://kojipkgs.fedoraproject.org/packages
51		51
52	## URL of Bodhi instance used for communication about package updates	52	## URL of Bodhi instance used for communication about package updates
53	#bodhi_server: https://admin.fedoraproject.org/updates/	53	#bodhi_server: https://admin.fedoraproject.org/updates/
54		54
		55	## Number of maximum retries to Bodhi server if the query request
		56	## fails with 5xx HTTP error
		57	#bodhi_request_max_retries: 3
		58
55	## URL of ResultsDB server API interface, which can store all test results	59	## URL of ResultsDB server API interface, which can store all test results
56	#resultsdb_server: http://resultsdb.qa.fedoraproject.org/resultsdb/api/v1.0/	60	#resultsdb_server: http://resultsdb.qa.fedoraproject.org/resultsdb/api/v1.0/
57		61
58	## URL of taskotron buildmaster	62	## URL of taskotron buildmaster
59	#taskotron_master: http://taskotron.qa.fedoraproject.org/taskmaster/	63	#taskotron_master: http://taskotron.qa.fedoraproject.org/taskmaster/
60		64
61	## name of step in buildbot that executes the task	65	## name of step in buildbot that executes the task
62	#buildbot_task_step = 'runtask'	66	#buildbot_task_step = 'runtask'
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines

View Options

libtaskotron/bodhi_utils.py

Show All 21 Lines		17	class BodhiUtils(object):
22		22
23	def __init__(self, client=None):	23		def __init__(self, client=None):
24	'''Create a new BodhiUtils instance.	24		'''Create a new BodhiUtils instance.
25		25
26	:param client: custom :class:`BodhiClient` instance. If ``None``, a	26		:param client: custom :class:`BodhiClient` instance. If ``None``, a
27	default BodhiClient instance is used.	27		default BodhiClient instance is used.
28	'''	28		'''
29		29
30	taskotron_config = config.get_config()	30		self.taskotron_config = config.get_config()
31	username = taskotron_config.fas_username	31		username = self.taskotron_config.fas_username
32	password = taskotron_config.fas_password	32		password = self.taskotron_config.fas_password
33	bodhi_url = taskotron_config.bodhi_server	33		bodhi_url = self.taskotron_config.bodhi_server
34		34
35	if not client:	35		if not client:
36	log.debug('creating bodhi client to %s' % bodhi_url)	36		log.debug('creating bodhi client to %s' % bodhi_url)
37	self.client = fedora.client.bodhi.BodhiClient(username=username,	37		self.client = fedora.client.bodhi.BodhiClient(username=username,
38	password=password,	38		password=password,
39	base_url=bodhi_url)	39		base_url=bodhi_url)
40	else:	40		else:
41	self.client = client	41		self.client = client
42		42
43		43
44	def query_update(self, package):	44		def query_update(self, package):
45	'''Get the last Bodhi update matching criteria.	45		'''Get the last Bodhi update matching criteria. Retry
		46		``bodhi_request_max_retries``-times when server returns HTTP 5xx response.
			kparalUnsubmitted Not Done If you want to make it pretty, you can use ``bodhi_request_max_retries``-times
46		47
47	:param str package: package NVR or package name or update title or	48		:param str package: package NVR or package name or update title or
48	update ID	49		update ID
49		50
50	.. note::	51		.. note::
51	Only NVR allowed, not NEVR. See	52		Only NVR allowed, not NEVR. See
52	https://fedorahosted.org/bodhi/ticket/592.	53		https://fedorahosted.org/bodhi/ticket/592.
53	:return: most recent Bodhi update object matching criteria, or	54		:return: most recent Bodhi update object matching criteria, or
54	``None`` when no update is found.	55		``None`` when no update is found.
55	:rtype: :class:`bunch.Bunch`	56		:rtype: :class:`bunch.Bunch`
56	'''	57		'''
57	log.debug('Searching Bodhi updates for: %s', package)	58		log.debug('Searching Bodhi updates for: %s', package)
58	res = self.client.query(package=package, limit=1)	59		max_retries = self.taskotron_config.bodhi_request_max_retries
59	if res['updates']:	60		for retry_num in xrange(1, max_retries + 1):
60	return res['updates'][0]	61		try:
61	else:	62		res = self.client.query(package=package, limit=1)
62	return None	63		if res['updates']:
		64		return res['updates'][0]
		65		else:
		66		return None
		67		except fedora.client.ServerError as e:
		68		if retry_num >= max_retries:
			kparalUnsubmitted Not Done The defensive approach here would be to use `>=` instead of `==`.
			garretrazielAuthorUnsubmitted Not Done I think that there is off-by-one error. `xrange(1,X)` generates numbers from 1 to X-1, so `retry_num` will never be equal to `max_retries`. I think that running tests confirms it.
		69		log.critical('Maximum Bodhi server retries exceeded')
		70		raise
			tflinkUnsubmitted Not Done why not use `e.code != 500` here instead of `<` and `>`?
			garretrazielAuthorUnsubmitted Not Done That's because Kamil checked it and server sometimes returns 500, sometimes 503. I was not sure whether it can return other errors, so it retries it on all 500 Server errors.
			tflinkUnsubmitted Not Done oh, I see now. sorry - read too quickly
		71		elif e.code < 500 or e.code > 599:
		72		raise
		73		else:
		74		log.warning('Got HTTP %d response from bodhi, retrying \
		75		(%d retries remaining)...', e.code, max_retries - retry_num)
			tflinkUnsubmitted Not Done please use a more descriptive var name than `i`. Something like `num_retries` or `retries` would be better
			mkrizekUnsubmitted Not Done The message is somewhat confusing though. If the server is always unavailable we get: Got HTTP 0 response from server, retrying (1 from 3)... Got HTTP 0 response from server, retrying (2 from 3)... re-raise the exception and that's it, where's (3 from 3)? :) Sure, we try querying the server three times, but the message is confusing. What about changing the message to something like `2 retries remaining`?
63		76
64		77
65	def build2update(self, builds, strict=False):	78		def build2update(self, builds, strict=False):
66	'''Find matching Bodhi updates for provided builds.	79		'''Find matching Bodhi updates for provided builds.
67		80
68	:param builds: builds to search for in N(E)VR format (``foo-1.2-3.fc20``	81		:param builds: builds to search for in N(E)VR format (``foo-1.2-3.fc20``
69	or ``foo-4:1.2-3.fc20``)	82		or ``foo-4:1.2-3.fc20``)
70	:type builds: iterable of str	83		:type builds: iterable of str
▲ Show 20 Lines • Show All 81 Lines • Show Last 20 Lines

View Options

libtaskotron/config_defaults.py

Show First 20 Lines • Show All 47 Lines • ▼ Show 20 Line(s)		23	class Config(object):
48	bodhi_server = 'https://admin.fedoraproject.org/updates/' #:	48		bodhi_server = 'https://admin.fedoraproject.org/updates/' #:
49	resultsdb_server = \	49		resultsdb_server = \
50	'http://resultsdb.qa.fedoraproject.org/resultsdb/api/v1.0/' #:	50		'http://resultsdb.qa.fedoraproject.org/resultsdb/api/v1.0/' #:
51	taskotron_master = 'http://taskotron.qa.fedoraproject.org/taskmaster/' #:	51		taskotron_master = 'http://taskotron.qa.fedoraproject.org/taskmaster/' #:
52	buildbot_task_step = 'runtask' #:	52		buildbot_task_step = 'runtask' #:
53		53
54	bodhi_posting_comments_span = 4320 #:	54		bodhi_posting_comments_span = 4320 #:
55	# 3 days (32460 = 4320)	55		# 3 days (32460 = 4320)
		56		bodhi_request_max_retries = 3 #:
56		57
57	tmpdir = '/var/tmp/taskotron' #:	58		tmpdir = '/var/tmp/taskotron' #:
58	logdir = '/var/log/taskotron' #:	59		logdir = '/var/log/taskotron' #:
59	log_name = 'taskotron.log'	60		log_name = 'taskotron.log'
60	'''name of the main log file in :attr:`logdir`'''	61		'''name of the main log file in :attr:`logdir`'''
61		62
62	log_level_stream = 'INFO' #:	63		log_level_stream = 'INFO' #:
63	log_level_file = 'DEBUG' #:	64		log_level_file = 'DEBUG' #:
Show All 37 Lines

View Options

testing/test_bodhi_utils.py

1	# -- coding: utf-8 --	1		# -- coding: utf-8 --
2	# Copyright 2009-2014, Red Hat, Inc.	2		# Copyright 2009-2014, Red Hat, Inc.
3	# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>	3		# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4	# See the LICENSE file for more details on Licensing	4		# See the LICENSE file for more details on Licensing
5		5
6	'''Unit tests for libtaskotron/bodhi_utils.py'''	6		'''Unit tests for libtaskotron/bodhi_utils.py'''
7		7
8	import pytest	8		import pytest
9	from dingus import Dingus	9		from dingus import Dingus
10	from bunch import Bunch	10		from bunch import Bunch
		11		from fedora.client import ServerError
11		12
12	from libtaskotron import bodhi_utils	13		from libtaskotron import bodhi_utils
13	from libtaskotron import exceptions as exc	14		from libtaskotron import exceptions as exc
14		15
15		16
		17		# http://stackoverflow.com/questions/3190706/nonlocal-keyword-in-python-2-x
		18		def create_failing_server(response):
		19		first_call = {"value": True}
		20
		21		def multicall(args, *kwargs):
		22		if first_call["value"]:
		23		first_call["value"] = False
		24		raise ServerError("nonexist", 500, "Internal Server Error")
		25		else:
		26		return response
		27
		28		return multicall
		29
		30
		31		def raise_500_server_error(args, *kwargs):
		32		raise ServerError("nonexist", 500, "Internal Server Error")
		33
		34
		35		def raise_404_not_found(args, *kwargs):
		36		raise ServerError("nonexist", 404, "Not found")
		37
		38
16	class TestQueryUpdate():	39		class TestQueryUpdate():
17	'''Test query_update()'''	40		'''Test query_update()'''
18		41
19	def setup_method(self, method):	42		def setup_method(self, method):
20	'''Run this before every test invocation'''	43		'''Run this before every test invocation'''
21	self.ref_bodhi_id = 'FEDORA-1234-56789'	44		self.ref_bodhi_id = 'FEDORA-1234-56789'
22	self.ref_update = {'title': 'Random update',	45		self.ref_update = {'title': 'Random update',
23	'builds': [{'nvr': 'foo-1.2-3.fc99',	46		'builds': [{'nvr': 'foo-1.2-3.fc99',
Show All 23 Lines		67	ref_query_answer = {'tg_flash': None,
47	'updates': [],	70		'updates': [],
48	}	71		}
49	stub_bodhi = Dingus(query__returns = ref_query_answer)	72		stub_bodhi = Dingus(query__returns = ref_query_answer)
50	bodhi = bodhi_utils.BodhiUtils(client=stub_bodhi)	73		bodhi = bodhi_utils.BodhiUtils(client=stub_bodhi)
51	update = bodhi.query_update(self.ref_bodhi_id)	74		update = bodhi.query_update(self.ref_bodhi_id)
52		75
53	assert update is None	76		assert update is None
54		77
		78		def test_query_retry(self):
		79		'''Test whether query can retry if server returns HTTP 500'''
		80		ref_query_answer = create_failing_server({'tg_flash': None,
		81		'num_items': 1,
		82		'title': '1 update found',
		83		'updates': [self.ref_update]})
		84		stub_bodhi = Dingus(query = ref_query_answer)
		85		bodhi = bodhi_utils.BodhiUtils(client=stub_bodhi)
		86		update = bodhi.query_update(self.ref_bodhi_id)
		87
		88		assert update == self.ref_update
		89
		90		def test_query_retry_fail(self):
		91		'''Test query on failing server'''
		92		stub_bodhi = Dingus(query = raise_500_server_error)
		93		bodhi = bodhi_utils.BodhiUtils(client=stub_bodhi)
		94
		95		with pytest.raises(ServerError):
		96		bodhi.query_update(self.ref_bodhi_id)
		97
		98		stub_bodhi = Dingus(query = raise_404_not_found)
		99		bodhi = bodhi_utils.BodhiUtils(client=stub_bodhi)
		100
		101		with pytest.raises(ServerError):
		102		bodhi.query_update(self.ref_bodhi_id)
		103
55		104
56	@pytest.mark.usefixtures('setup')	105		@pytest.mark.usefixtures('setup')
57	class TestBuild2Update():	106		class TestBuild2Update():
58	'''Test build2update()'''	107		'''Test build2update()'''
59		108
60	# fake requests and responses for query_update()	109		# fake requests and responses for query_update()
61	requests = {	110		requests = {
62	'foo-ok-1.2-3.fc20': Bunch(builds=[Bunch(nvr=u'foo-ok-1.2-3.fc20')]),	111		'foo-ok-1.2-3.fc20': Bunch(builds=[Bunch(nvr=u'foo-ok-1.2-3.fc20')]),
▲ Show 20 Lines • Show All 106 Lines • Show Last 20 Lines

			Path	Packages
M			conf/taskotron.yaml.example (4 lines)
M			libtaskotron/bodhi_utils.py (33 lines)
M			libtaskotron/config_defaults.py (1 line)
M			testing/test_bodhi_utils.py (49 lines)

retry Bodhi request if server returns HTTP 5xx
ClosedPublic
Actions

Details

Diff Detail

Revision Contents

Diff 672

conf/taskotron.yaml.example

libtaskotron/bodhi_utils.py

libtaskotron/config_defaults.py

testing/test_bodhi_utils.py

retry Bodhi request if server returns HTTP 5xxClosedPublicActions

Details

Diff Detail

Revision Contents

Diff 672

conf/taskotron.yaml.example

libtaskotron/bodhi_utils.py

libtaskotron/config_defaults.py

testing/test_bodhi_utils.py

retry Bodhi request if server returns HTTP 5xx
ClosedPublic
Actions