From 3fc4b4a64eb07ba1a373827cf2355c578e6b528b Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Nov 07 2021 09:45:25 +0000 Subject: PR#3103: retry get_next_release to avoid race condition Merges #3103 https://pagure.io/koji/pull-request/3103 Fixes: #3079 https://pagure.io/koji/issue/3079 Race condition for getNextRelease still present --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 45fd06d..489de5d 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -4378,7 +4378,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. @@ -4397,10 +4397,14 @@ 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 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.ParameterError("incr parameter must be an integer") values = { 'name': build_info['name'], 'version': build_info['version'], @@ -4422,24 +4426,50 @@ 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) return release +def get_next_build(build_info): + """ + Returns a new build entry with automatic release incrementing + + :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 + 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 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(build_info) + except IntegrityError: + build_info['release'] = get_next_release(build_info, 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') @@ -14413,13 +14443,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 - build_id = new_build(data) + build_id = get_next_build(data) data['id'] = build_id new_maven_build(data, maven_info) @@ -14584,9 +14615,7 @@ class HostExports(object): data['owner'] = task.getOwner() data['state'] = koji.BUILD_STATES['BUILDING'] data['completion_time'] = None - if data.get('release') is None: - data['release'] = get_next_release(build_info) - build_id = new_build(data) + build_id = get_next_build(data) data['id'] = build_id new_image_build(data) return data 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)