From b6bd952b4759173218c560ec2ef5ea0a62009f9e Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 19 2021 13:30:40 +0000 Subject: [PATCH 1/4] getNextRelease race condition retry Fixes: https://pagure.io/koji/issue/3079 --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 12a6295..0f8e9f0 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -4369,7 +4369,7 @@ def get_build_logs(build): return logs -def get_next_release(build_info): +def get_next_release(build_info, incr=1): """ Find the next release for a package's version. @@ -4388,6 +4388,8 @@ def get_next_release(build_info): :param dict build_info: a dict with two keys: a package "name" and "version" of the builds to search. For example, {"name": "bash", "version": "4.4.19"} + :param incr int: value which should be added to latest found release + (it is used for solving race-condition conflicts) :returns: a release string for this package, for example "15.el8". :raises: BuildError if the latest build uses a release value that Koji does not know how to increment. @@ -4413,18 +4415,18 @@ def get_next_release(build_info): release = result['release'] if not release: - release = '1' + release = str(incr) elif release.isdigit(): - release = str(int(release) + 1) + release = str(int(release) + incr) elif len(release.split('.')) == 2 and release.split('.')[0].isdigit(): # Handle the N.%{dist} case r_split = release.split('.') - r_split[0] = str(int(r_split[0]) + 1) + r_split[0] = str(int(r_split[0]) + incr) release = '.'.join(r_split) elif len(release.split('.')) == 3 and release.split('.')[2].isdigit(): # Handle the {date}.nightly.%{id} case r_split = release.split('.') - r_split[2] = str(int(r_split[2]) + 1) + r_split[2] = str(int(r_split[2]) + incr) release = '.'.join(r_split) else: raise koji.BuildError('Unable to increment release value: %s' % release) @@ -14348,7 +14350,13 @@ class HostExports(object): data['owner'] = task.getOwner() data['state'] = koji.BUILD_STATES['BUILDING'] data['completion_time'] = None - build_id = new_build(data) + for try_no in range(2, 10): + try: + build_id = new_build(data) + except IntegrityError: + data['release'] = get_next_release(data, try_no) + if not build_id: + raise koji.GenericError("Can't find available release") data['id'] = build_id new_maven_build(data, maven_info) @@ -14513,9 +14521,16 @@ class HostExports(object): data['owner'] = task.getOwner() data['state'] = koji.BUILD_STATES['BUILDING'] data['completion_time'] = None + build_id = None if data.get('release') is None: - data['release'] = get_next_release(build_info) - build_id = new_build(data) + data['release'] = get_next_release(data) + for try_no in range(2, 10): + try: + build_id = new_build(data) + except IntegrityError: + data['release'] = get_next_release(data, try_no) + if not build_id: + raise koji.GenericError("Can't find available release") data['id'] = build_id new_image_build(data) return data From f596f19bd3965d23b360dbe3a92e4398ea3618e4 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Oct 28 2021 14:59:23 +0000 Subject: [PATCH 2/4] move logic into get_next_build() --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 0f8e9f0..fe44b76 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -4394,6 +4394,8 @@ def get_next_release(build_info, incr=1): :raises: BuildError if the latest build uses a release value that Koji does not know how to increment. """ + if not isinstance(incr, int): + raise koji.GenericError("incr parameter must be an integer") values = { 'name': build_info['name'], 'version': build_info['version'], @@ -4433,6 +4435,32 @@ def get_next_release(build_info, incr=1): return release +def get_next_build(data): + """ + Returns a new build entry with automatic release incrementing + + :param dict data: data for the build to be created + :returns: build id for the created build + + If data includes a non-None release value, then this function is + equivalent to new_build. Otherwise, it will use get_next_release() + to choose the release value. + + To limit race conditions, this function will try a series of release + increments. + """ + if data.get('release') is not None: + return new_build(data) + data['release'] = get_next_release(data) + for try_no in range(2, 10): + try: + return new_build(data) + except IntegrityError: + data['release'] = get_next_release(data, try_no) + # otherwise + raise koji.GenericError("Can't find available release") + + def _fix_rpm_row(row): if 'extra' in row: row['extra'] = parse_json(row['extra'], desc='rpm extra') @@ -14344,19 +14372,14 @@ class HostExports(object): host.verify() task = Task(task_id) task.assertHost(host.id) - build_info['release'] = get_next_release(build_info) + # ensure release is None so get_next_build will handle incrementing + build_info['release'] = None data = build_info.copy() data['task_id'] = task_id data['owner'] = task.getOwner() data['state'] = koji.BUILD_STATES['BUILDING'] data['completion_time'] = None - for try_no in range(2, 10): - try: - build_id = new_build(data) - except IntegrityError: - data['release'] = get_next_release(data, try_no) - if not build_id: - raise koji.GenericError("Can't find available release") + build_id = get_next_build(data) data['id'] = build_id new_maven_build(data, maven_info) @@ -14521,16 +14544,7 @@ class HostExports(object): data['owner'] = task.getOwner() data['state'] = koji.BUILD_STATES['BUILDING'] data['completion_time'] = None - build_id = None - if data.get('release') is None: - data['release'] = get_next_release(data) - for try_no in range(2, 10): - try: - build_id = new_build(data) - except IntegrityError: - data['release'] = get_next_release(data, try_no) - if not build_id: - raise koji.GenericError("Can't find available release") + build_id = get_next_build(data) data['id'] = build_id new_image_build(data) return data From da8860cfe593924155438adaa80f899feef8952f Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 05 2021 14:30:00 +0000 Subject: [PATCH 3/4] naming/docstring adjustments --- diff --git a/hub/kojihub.py b/hub/kojihub.py index fe44b76..4d38266 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -4388,14 +4388,14 @@ def get_next_release(build_info, incr=1): :param dict build_info: a dict with two keys: a package "name" and "version" of the builds to search. For example, {"name": "bash", "version": "4.4.19"} - :param incr int: value which should be added to latest found release + :param int incr: value which should be added to latest found release (it is used for solving race-condition conflicts) :returns: a release string for this package, for example "15.el8". :raises: BuildError if the latest build uses a release value that Koji does not know how to increment. """ if not isinstance(incr, int): - raise koji.GenericError("incr parameter must be an integer") + raise koji.ParameterError("incr parameter must be an integer") values = { 'name': build_info['name'], 'version': build_info['version'], @@ -4435,11 +4435,11 @@ def get_next_release(build_info, incr=1): return release -def get_next_build(data): +def get_next_build(build_info): """ Returns a new build entry with automatic release incrementing - :param dict data: data for the build to be created + :param dict build_info: data for the build to be created :returns: build id for the created build If data includes a non-None release value, then this function is @@ -4449,14 +4449,14 @@ def get_next_build(data): To limit race conditions, this function will try a series of release increments. """ - if data.get('release') is not None: - return new_build(data) - data['release'] = get_next_release(data) + if build_info.get('release') is not None: + return new_build(build_info) + build_info['release'] = get_next_release(build_info) for try_no in range(2, 10): try: - return new_build(data) + return new_build(build_info) except IntegrityError: - data['release'] = get_next_release(data, try_no) + build_info['release'] = get_next_release(build_info, try_no) # otherwise raise koji.GenericError("Can't find available release") From 10d74781b3d369ed22518bc38ef58d442b928274 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 05 2021 15:44:41 +0000 Subject: [PATCH 4/4] update unit tests --- diff --git a/tests/test_hub/test_get_next_build.py b/tests/test_hub/test_get_next_build.py new file mode 100644 index 0000000..34b4ece --- /dev/null +++ b/tests/test_hub/test_get_next_build.py @@ -0,0 +1,73 @@ +import mock +import unittest +import koji +import kojihub + +from psycopg2._psycopg import IntegrityError + + +class TestGetNextBuild(unittest.TestCase): + + def setUp(self): + self.get_next_release = mock.patch('kojihub.get_next_release').start() + self.new_build = mock.patch('kojihub.new_build').start() + self._dml = mock.patch('kojihub._dml').start() + self.binfo = {'name': 'name', 'version': 'version'} + + def tearDown(self): + mock.patch.stopall() + + def test_get_next_build_simple(self): + # typical case + self.get_next_release.return_value = '2.mydist' + self.new_build.return_value = 'mybuild' + result = kojihub.get_next_build(self.binfo) + self.assertEqual(result, 'mybuild') + self.new_build.assert_called_once() + # release value should be passed to new_build + self.assertEqual(self.new_build.call_args[0][0]['release'], '2.mydist') + + def test_get_next_build_have_release(self): + # if a release is passed, get_next_release should not be called + self.binfo['release'] = '42' + result = kojihub.get_next_build(self.binfo) + self.new_build.assert_called_once() + self.get_next_release.assert_not_called() + # release value should be passed to new_build + self.assertEqual(self.new_build.call_args[0][0]['release'], '42') + + def test_get_next_build_retry(self): + # set up new_build to fail a few times + nb_callnum = 0 + def my_new_build(data, strict=False): + nonlocal nb_callnum + nb_callnum += 1 + if nb_callnum < 3: + raise IntegrityError('fake error') + return 'mybuild' + self.new_build.side_effect = my_new_build + + self.get_next_release.return_value = '2.mydist' + + result = kojihub.get_next_build(self.binfo) + self.assertEqual(result, 'mybuild') + self.assertEqual(len(self.new_build.mock_calls), 3) + self.assertEqual(len(self.get_next_release.mock_calls), 3) + # incr arg should have incremented on successive tries + self.assertEqual(self.get_next_release.mock_calls[1][1][1], 2) + self.assertEqual(self.get_next_release.mock_calls[2][1][1], 3) + + def test_get_next_build_fail(self): + # set up new_build to fail forever + self.new_build.side_effect = IntegrityError('fake error') + self.get_next_release.return_value = '2.mydist' + + with self.assertRaises(koji.GenericError): + result = kojihub.get_next_build(self.binfo) + + # there should have been ten tries + self.assertEqual(len(self.new_build.mock_calls), 8) + self.assertEqual(len(self.get_next_release.mock_calls), 9) + # incr arg should have incremented on successive tries + for i in range(1, 9): + self.assertEqual(self.get_next_release.mock_calls[i][1][1], i+1) diff --git a/tests/test_hub/test_get_next_release.py b/tests/test_hub/test_get_next_release.py index ea1c0d5..561a3f1 100644 --- a/tests/test_hub/test_get_next_release.py +++ b/tests/test_hub/test_get_next_release.py @@ -33,6 +33,7 @@ class TestGetNextRelease(unittest.TestCase): ['1.el6', '2.el6'], ['1.fc23', '2.fc23'], ['45.fc23', '46.fc23'], + ['20211105.nightly.7', '20211105.nightly.8'], ] for a, b in data: self.query.executeOne.return_value = {'release': a} @@ -53,3 +54,15 @@ class TestGetNextRelease(unittest.TestCase): with self.assertRaises(koji.BuildError): kojihub.get_next_release(self.binfo) + def test_get_next_release_bad_incr(self): + data = [ + # bad_incr_value + "foo", + None, + 1.1, + {1:1}, + [1], + ] + for val in data: + with self.assertRaises(koji.ParameterError): + kojihub.get_next_release(self.binfo, incr=val)