From 338da4d25aebfd9f5b75529964e5637a85fdcc06 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 1/7] drop strict --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 53ae020..f09392e 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -4877,8 +4877,7 @@ def get_next_build(build_info): for try_no in range(2, 10): savepoint = Savepoint('get_next_build_pre_insert') try: - # using strict so we don't try to recycle - return new_build(build_info, strict=True) + return new_build(build_info) except (IntegrityError, koji.GenericError): savepoint.rollback() build_info['release'] = get_next_release(build_info, try_no) From 6bd5d69c2118433279eb7b88498a79e4077a91bf Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 2/7] add a row lock in recycle_build --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index f09392e..0fdc4d1 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -6523,10 +6523,15 @@ def new_build(data, strict=False): def recycle_build(old, data): """Check to see if a build can by recycled and if so, update it""" - st_desc = koji.BUILD_STATES[old['state']] + # re-query with a rowlock + query = QueryProcessor(tables=['build'], columns=['state', 'task_id'], + clauses=['id = %(id)s'], values=old, + opts={'rowlock': True}) + check = query.executeOne() + st_desc = koji.BUILD_STATES[check['state']] if st_desc == 'BUILDING': # check to see if this is the controlling task - if data['state'] == old['state'] and data.get('task_id', '') == old['task_id']: + if data['state'] == check['state'] and data.get('task_id', '') == check['task_id']: # the controlling task must have restarted (and called initBuild again) return raise koji.GenericError("Build already in progress (task %(task_id)d)" From d49a989b5626bba5ef142559b0f3fdbfa8b52276 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 3/7] fix unit tests --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 0fdc4d1..d95c691 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -6524,10 +6524,7 @@ def recycle_build(old, data): """Check to see if a build can by recycled and if so, update it""" # re-query with a rowlock - query = QueryProcessor(tables=['build'], columns=['state', 'task_id'], - clauses=['id = %(id)s'], values=old, - opts={'rowlock': True}) - check = query.executeOne() + check = _recycle_lock(old) st_desc = koji.BUILD_STATES[check['state']] if st_desc == 'BUILDING': # check to see if this is the controlling task @@ -6604,6 +6601,13 @@ def recycle_build(old, data): old=old['state'], new=data['state'], info=buildinfo) +def _recycle_lock(old): + query = QueryProcessor(tables=['build'], columns=['state', 'task_id'], + clauses=['id = %(id)s'], values=old, + opts={'rowlock': True}) + return query.executeOne() + + def check_noarch_rpms(basepath, rpms, logs=None): """ If rpms contains any noarch rpms with identical names, diff --git a/tests/test_hub/test_recycle_build.py b/tests/test_hub/test_recycle_build.py index f63942d..4717ddc 100644 --- a/tests/test_hub/test_recycle_build.py +++ b/tests/test_hub/test_recycle_build.py @@ -2,7 +2,7 @@ from unittest import mock import unittest import koji -import kojihub +from kojihub import kojihub QP = kojihub.QueryProcessor UP = kojihub.UpdateProcessor @@ -30,6 +30,12 @@ class TestRecycleBuild(unittest.TestCase): self.get_build = mock.patch('kojihub.kojihub.get_build').start() self.list_volumes = mock.patch('kojihub.kojihub.list_volumes').start() self.list_volumes.return_value = [{'id': 0, 'name': 'DEFAULT'}] + self._recycle_lock = mock.patch('kojihub.kojihub._recycle_lock').start() + + # base data + self.new = self.new_base.copy() + self.old = self.old_base.copy() + self._recycle_lock.return_value = self.old def tearDown(self): mock.patch.stopall() @@ -53,7 +59,8 @@ class TestRecycleBuild(unittest.TestCase): return delete # Basic old and new build infos - old = {'id': 2, + old_base = { + 'id': 2, 'state': 3, 'task_id': None, 'epoch': None, @@ -68,7 +75,8 @@ class TestRecycleBuild(unittest.TestCase): 'cg_id': None, 'volume_id': 0, 'volume_name': 'DEFAULT'} - new = {'state': 3, + new_base = { + 'state': 3, 'name': 'GConf2', 'version': '3.2.6', 'release': '15.fc23', @@ -83,13 +91,11 @@ class TestRecycleBuild(unittest.TestCase): 'volume_id': 0} def test_build_already_in_progress(self): - new = self.new.copy() - old = self.old.copy() - old['state'] = new['state'] = koji.BUILD_STATES['BUILDING'] - old['task_id'] = 137 + self.old['state'] = self.new['state'] = koji.BUILD_STATES['BUILDING'] + self.old['task_id'] = 137 with self.assertRaises(koji.GenericError) as ex: - kojihub.recycle_build(old, new) - self.assertEqual(f"Build already in progress (task {old['task_id']})", str(ex.exception)) + kojihub.recycle_build(self.old, self.new) + self.assertEqual(f"Build already in progress (task {self.old['task_id']})", str(ex.exception)) self.assertEqual(len(self.queries), 0) self.assertEqual(len(self.updates), 0) self.assertEqual(len(self.deletes), 0) @@ -98,11 +104,9 @@ class TestRecycleBuild(unittest.TestCase): self.run_callbacks.assert_not_called() def test_build_already_in_progress_same_task_id(self): - new = self.new.copy() - old = self.old.copy() - old['state'] = new['state'] = koji.BUILD_STATES['BUILDING'] - old['task_id'] = new['task_id'] = 137 - result = kojihub.recycle_build(old, new) + self.old['state'] = self.new['state'] = koji.BUILD_STATES['BUILDING'] + self.old['task_id'] = self.new['task_id'] = 137 + result = kojihub.recycle_build(self.old, self.new) self.assertEqual(result, None) self.assertEqual(len(self.queries), 0) self.assertEqual(len(self.updates), 0) @@ -112,11 +116,10 @@ class TestRecycleBuild(unittest.TestCase): self.run_callbacks.assert_not_called() def test_not_in_failed_or_canceled_state(self): - old = self.old.copy() - old['state'] = koji.BUILD_STATES['COMPLETE'] + self.old['state'] = koji.BUILD_STATES['COMPLETE'] with self.assertRaises(koji.GenericError) as ex: - kojihub.recycle_build(old, self.new) - self.assertEqual(f"Build already exists (id={old['id']}, state=COMPLETE): {self.new}", + kojihub.recycle_build(self.old, self.new) + self.assertEqual(f"Build already exists (id={self.old['id']}, state=COMPLETE): {self.new}", str(ex.exception)) self.assertEqual(len(self.queries), 0) self.assertEqual(len(self.updates), 0) @@ -126,11 +129,10 @@ class TestRecycleBuild(unittest.TestCase): self.run_callbacks.assert_not_called() def test_tag_activity_already_exists(self): - old = self.old.copy() - old['task_id'] = 137 + self.old['task_id'] = 137 self.query_execute.return_value = [{'tag_id': 123}] with self.assertRaises(koji.GenericError) as ex: - kojihub.recycle_build(old, self.new) + kojihub.recycle_build(self.old, self.new) self.assertEqual("Build already exists. Unable to recycle, has tag history", str(ex.exception)) self.assertEqual(len(self.queries), 1) @@ -141,18 +143,17 @@ class TestRecycleBuild(unittest.TestCase): self.assertEqual(query.tables, ['tag_listing']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['tag_id']) self.get_build.assert_not_called() self.run_callbacks.assert_not_called() def test_rpm_activity_already_exists(self): - old = self.old.copy() - old['task_id'] = 137 + self.old['task_id'] = 137 self.query_execute.side_effect = [[], [{'id': 1}]] with self.assertRaises(koji.GenericError) as ex: - kojihub.recycle_build(old, self.new) + kojihub.recycle_build(self.old, self.new) self.assertEqual("Build already exists. Unable to recycle, has rpm data", str(ex.exception)) @@ -164,25 +165,24 @@ class TestRecycleBuild(unittest.TestCase): self.assertEqual(query.tables, ['tag_listing']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['tag_id']) query = self.queries[1] self.assertEqual(query.tables, ['rpminfo']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['id']) self.get_build.assert_not_called() self.run_callbacks.assert_not_called() def test_archive_activity_already_exists(self): - old = self.old.copy() - old['task_id'] = 137 + self.old['task_id'] = 137 self.query_execute.side_effect = [[], [], [{'id': 11}]] with self.assertRaises(koji.GenericError) as ex: - kojihub.recycle_build(old, self.new) + kojihub.recycle_build(self.old, self.new) self.assertEqual("Build already exists. Unable to recycle, has archive data", str(ex.exception)) @@ -194,35 +194,33 @@ class TestRecycleBuild(unittest.TestCase): self.assertEqual(query.tables, ['tag_listing']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['tag_id']) query = self.queries[1] self.assertEqual(query.tables, ['rpminfo']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['id']) query = self.queries[2] self.assertEqual(query.tables, ['archiveinfo']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['id']) self.get_build.assert_not_called() self.run_callbacks.assert_not_called() def test_valid(self): - old = self.old.copy() - new = self.new.copy() - old['task_id'] = new['task_id'] = 137 + self.old['task_id'] = self.new['task_id'] = 137 self.query_execute.side_effect = [[], [], []] self.get_build.return_value = {'build_id': 2, 'name': 'GConf2', 'version': '3.2.6', 'release': '15.fc23'} - kojihub.recycle_build(old, new) + kojihub.recycle_build(self.old, self.new) self.assertEqual(len(self.queries), 3) self.assertEqual(len(self.updates), 1) @@ -232,53 +230,53 @@ class TestRecycleBuild(unittest.TestCase): self.assertEqual(query.tables, ['tag_listing']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['tag_id']) query = self.queries[1] self.assertEqual(query.tables, ['rpminfo']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['id']) query = self.queries[2] self.assertEqual(query.tables, ['archiveinfo']) self.assertEqual(query.joins, None) self.assertEqual(query.clauses, ['build_id = %(id)s']) - self.assertEqual(query.values, old) + self.assertEqual(query.values, self.old) self.assertEqual(query.columns, ['id']) delete = self.deletes[0] self.assertEqual(delete.table, 'maven_builds') self.assertEqual(delete.clauses, ['build_id = %(id)i']) - self.assertEqual(delete.values, old) + self.assertEqual(delete.values, self.old) delete = self.deletes[1] self.assertEqual(delete.table, 'win_builds') self.assertEqual(delete.clauses, ['build_id = %(id)i']) - self.assertEqual(delete.values, old) + self.assertEqual(delete.values, self.old) delete = self.deletes[2] self.assertEqual(delete.table, 'image_builds') self.assertEqual(delete.clauses, ['build_id = %(id)i']) - self.assertEqual(delete.values, old) + self.assertEqual(delete.values, self.old) delete = self.deletes[3] self.assertEqual(delete.table, 'build_types') self.assertEqual(delete.clauses, ['build_id = %(id)i']) - self.assertEqual(delete.values, old) + self.assertEqual(delete.values, self.old) update = self.updates[0] self.assertEqual(update.table, 'build') - self.assertEqual(update.values, new) + self.assertEqual(update.values, self.new) for key in ['state', 'task_id', 'owner', 'start_time', 'completion_time', 'epoch']: - assert update.data[key] == new[key] + assert update.data[key] == self.new[key] self.assertEqual(update.rawdata, {'create_event': 'get_event()'}) self.assertEqual(update.clauses, ['id=%(id)s']) - self.get_build.assert_called_once_with(new['id'], strict=True) + self.get_build.assert_called_once_with(self.new['id'], strict=True) self.assertEqual(self.run_callbacks.call_count, 2) # our default data does not include stray files @@ -286,30 +284,42 @@ class TestRecycleBuild(unittest.TestCase): self.unlink.assert_not_called() def test_stray_link(self): - old = self.old.copy() - new = self.new.copy() - old['task_id'] = new['task_id'] = 137 + self.old['task_id'] = self.new['task_id'] = 137 self.query_execute.side_effect = [[], [], []] self.get_build.return_value = {'build_id': 2, 'name': 'GConf2', 'version': '3.2.6', 'release': '15.fc23'} self.islink.return_value = True - kojihub.recycle_build(old, new) + kojihub.recycle_build(self.old, self.new) self.rmtree.assert_not_called() self.unlink.assert_called_once_with('/mnt/koji/packages/GConf2/3.2.6/15.fc23') def test_stray_dir(self): - old = self.old.copy() - new = self.new.copy() - old['task_id'] = new['task_id'] = 137 + self.old['task_id'] = self.new['task_id'] = 137 self.query_execute.side_effect = [[], [], []] self.get_build.return_value = {'build_id': 2, 'name': 'GConf2', 'version': '3.2.6', 'release': '15.fc23'} self.exists.return_value = True - kojihub.recycle_build(old, new) + kojihub.recycle_build(self.old, self.new) self.unlink.assert_not_called() self.rmtree.assert_called_once_with('/mnt/koji/packages/GConf2/3.2.6/15.fc23') +class TestRecycleLock(unittest.TestCase): + + def setUp(self): + self.QP = mock.patch('kojihub.kojihub.QueryProcessor').start() + + def tearDown(self): + mock.patch.stopall() + + def test_recycle_lock(self): + old = {'id': 12345} + self.QP().executeOne.return_value = mock.sentinel.lock + result = kojihub._recycle_lock(old) + + self.assertEqual(result, mock.sentinel.lock) + + # the end From 97261f2582f06f65fdd33d94962f25c0dac3b8c2 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 4/7] a bit more logging --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index d95c691..6cf32ad 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -4878,7 +4878,8 @@ def get_next_build(build_info): savepoint = Savepoint('get_next_build_pre_insert') try: return new_build(build_info) - except (IntegrityError, koji.GenericError): + except (IntegrityError, koji.GenericError) as e: + logger.warning(f'Incrementing next build release due to: {e}') savepoint.rollback() build_info['release'] = get_next_release(build_info, try_no) # otherwise From 9327a3f3a7e609099ae7c93681b58117b8d02765 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 5/7] fix ref for consistency --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 6cf32ad..aa36946 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -6533,7 +6533,7 @@ def recycle_build(old, data): # the controlling task must have restarted (and called initBuild again) return raise koji.GenericError("Build already in progress (task %(task_id)d)" - % old) + % check) # TODO? - reclaim 'stale' builds (state=BUILDING and task_id inactive) if st_desc not in ('FAILED', 'CANCELED'): From 4be7657d9d537482cc8e0f957036cd8f746ae854 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 6/7] add more tries in get_next_build --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index aa36946..802aec3 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -4874,14 +4874,14 @@ def get_next_build(build_info): 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): + for incr in range(2, 30): savepoint = Savepoint('get_next_build_pre_insert') try: return new_build(build_info) except (IntegrityError, koji.GenericError) as e: - logger.warning(f'Incrementing next build release due to: {e}') savepoint.rollback() - build_info['release'] = get_next_release(build_info, try_no) + build_info['release'] = get_next_release(build_info, incr) + logger.info(f'Incrementing next build release to {build_info['release']}: {e}') # otherwise raise koji.GenericError("Can't find available release") diff --git a/tests/test_hub/test_get_next_build.py b/tests/test_hub/test_get_next_build.py index 7661386..4fc05b5 100644 --- a/tests/test_hub/test_get_next_build.py +++ b/tests/test_hub/test_get_next_build.py @@ -66,9 +66,12 @@ class TestGetNextBuild(unittest.TestCase): 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) + # loop is over range(2, 30) + self.assertEqual(len(self.new_build.mock_calls), 28) + self.assertEqual(len(self.get_next_release.mock_calls), 29) # 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) + + +# the end From 362cdf930c623e3a7fbb38e4e617bedf0c48baa2 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 10 2026 19:04:06 +0000 Subject: [PATCH 7/7] backwards compatible fstring quoting --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 802aec3..265e4ee 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -4881,7 +4881,7 @@ def get_next_build(build_info): except (IntegrityError, koji.GenericError) as e: savepoint.rollback() build_info['release'] = get_next_release(build_info, incr) - logger.info(f'Incrementing next build release to {build_info['release']}: {e}') + logger.info(f'Incrementing next build release to {build_info["release"]}: {e}') # otherwise raise koji.GenericError("Can't find available release")