PR#4060: auto arch refusal for noarch tasks - koji

		`@@ -747,6 +747,7 @@`
		`opts['id'] = task_id`
		`koji.plugin.run_callbacks(`
		`'postTaskStateChange', attribute='state', old=None, new='FREE', info=opts)`
		`+ scheduler.auto_arch_refuse(task_id) # temporary workaround`
		`return task_id`

kojihub/scheduler.py

file modified

+64

		`@@ -104,6 +104,70 @@`
		`log_both(f'Host refused task: {msg}', task_id=taskID, host_id=hostID)`


		`+ def auto_arch_refuse(task_id):`
		`+ """Set refusals for hosts based on task parameters"""`
		`+ # This is a temporary workaround`
		`+ try:`
		`+ _auto_arch_refuse(task_id)`
		`+ except Exception:`
		`+ # better to not fail make_task()`
		`+ logger.exception('Error generating auto refusals for task %i', task_id)`
		`+ return`
		`+`
		`+`
		`+ def _auto_arch_refuse(task_id):`
		`+ task = kojihub.Task(task_id)`
		`+ info = task.getInfo(request=True)`
		`+ if info['arch'] != 'noarch':`
		`+ return`
		`+ if info['method'] not in {'buildArch', 'buildMaven', 'wrapperRPM', 'rebuildSRPM',`
		`+ 'buildSRPMFromSCM'}:`
		`+ return`
		`+ if task.isFinished():`
		`+ # shouldn't happen`
		`+ logger.warning('Skipping auto refusal for closed task %i', task_id)`
		`+ return`
		`+`
		`+ try:`
		`+ task_params = koji.tasks.parse_task_params(info['method'], info['request'])`
		`+ except Exception:`
		`+ logger.warning('Invalid params for task %i', task_id)`
		`+ return`
		`+`
		`+ # figure out build tag`
		`+ if info['method'] in {'buildMaven', 'buildSRPMFromSCM', 'rebuildSRPM'}:`
		`+ tag = task_params['build_tag']`
		`+ elif info['method'] == 'buildArch':`
		`+ tag = task_params['root']`
		`+ elif info['method'] == 'wrapperRPM':`
		`+ target = kojihub.get_build_target(task_params['build_target'])`
		`+ if not target:`
		`+ logger.warning('Invalid target for task %i', task_id)`
		`+ return`
		`+ tag = target['build_tag']`
		`+ taginfo = kojihub.get_tag(tag)`
		`+ if not taginfo:`
		`+ logger.warning('Invalid build tag for task %i', task_id)`
		`+ return`
		`+`
		`+ # from here, we're basically doing checkHostArch() for all hosts in the channel`
		`+ buildconfig = context.handlers.call('getBuildConfig', taginfo['id'])`
		`+ # getBuildConfig traverses inheritance to find arches if tag does not have them`
		`+ tag_arches = set([koji.canonArch(a) for a in buildconfig['arches'].split()])`
		`+ if not tag_arches:`
		`+ logger.warning("No arches for tag %(name)s [%(id)s]", taginfo)`
		`+ # we don't error here, allowing the task itself to fail`
		`+ return`
		`+`
		`+ hosts = context.handlers.call('listHosts', channelID=info['channel_id'], enabled=True,`
		`+ queryOpts={'order': 'id'})`
		`+ for host in hosts:`
		`+ host_arches = host['arches'].split()`
		`+ logger.debug('%r vs %r', tag_arches, host_arches)`
		`+ if not tag_arches.intersection(host_arches):`
		`+ set_refusal(host['id'], task_id, soft=False, msg='automatic arch refusal')`
		`+`
		`+`
		`class TaskRefusalsQuery(QueryView):`

		`tables = ['scheduler_task_refusals']`

tests/test_hub/test_auto_arch_refuse.py

file added

+217

		`@@ -0,0 +1,217 @@`
		`+ import datetime`
		`+ import mock`
		`+ import unittest`
		`+`
		`+ import koji`
		`+ import kojihub`
		`+ import kojihub.db`
		`+ from kojihub import scheduler`
		`+`
		`+`
		`+ QP = scheduler.QueryProcessor`
		`+ IP = scheduler.InsertProcessor`
		`+ UP = scheduler.UpdateProcessor`
		`+ TASK = kojihub.Task`
		`+`
		`+`
		`+ class MyError(Exception):`
		`+ pass`
		`+`
		`+`
		`+ class AutoRefuseTest(unittest.TestCase):`
		`+`
		`+ def setUp(self):`
		`+ self._dml = mock.patch('kojihub.db._dml').start()`
		`+ # self.exports = kojihub.RootExports()`
		`+ self.task = mock.MagicMock()`
		`+ self.Task = mock.patch('kojihub.kojihub.Task', return_value=self.task).start()`
		`+ self.get_build_target = mock.patch('kojihub.kojihub.get_build_target').start()`
		`+ self.get_tag = mock.patch('kojihub.kojihub.get_tag').start()`
		`+ self.context = mock.patch('kojihub.scheduler.context').start()`
		`+ self.set_refusal = mock.patch('kojihub.scheduler.set_refusal').start()`
		`+ self.handlers = {`
		`+ 'getBuildConfig': mock.MagicMock(),`
		`+ 'listHosts': mock.MagicMock(),`
		`+ }`
		`+ self.context.handlers.call.side_effect = self._my_handler_call`
		`+ self.set_base_data()`
		`+`
		`+ def tearDown(self):`
		`+ mock.patch.stopall()`
		`+`
		`+ def set_base_data(self):`
		`+ request = [`
		`+ 'tasks/8755/59888755/release-e2e-test-1.0.4474-1.el9.src.rpm',`
		`+ 'TAG_ID',`
		`+ 'x86_64',`
		`+ True,`
		`+ {'repo_id': 8075973}]`
		`+ self.taskinfo = {`
		`+ 'arch': 'noarch',`
		`+ 'channel_id': 35,`
		`+ 'id': 59888794,`
		`+ 'method': 'buildArch',`
		`+ 'request': request,`
		`+ 'state': 1,`
		`+ }`
		`+ self.task.getInfo.return_value = self.taskinfo`
		`+ self.task.isFinished.return_value = False`
		`+ self.get_tag.return_value = {'id': 'TAGID', 'name': 'MYTAG'}`
		`+ self.handlers['listHosts'].return_value = [{'id': 'HOST', 'arches': 'x86_64 i686'}]`
		`+ self.handlers['getBuildConfig'].return_value = {'arches': 'x86_64 s390x ppc64le aarch64'}`
		`+`
		`+ def _my_handler_call(self, method, a, *kw):`
		`+ handler = self.handlers[method]`
		`+ return handler(a, *kw)`
		`+`
		`+ def test_arch_overlap(self):`
		`+ # we mostly test the underlying function to avoid masking errors`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+ self.set_refusal.assert_not_called()`
		`+`
		`+ def test_arch_disjoint(self):`
		`+ self.handlers['listHosts'].return_value = [{'id': 'HOST', 'arches': 'riscv128'}]`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+ self.set_refusal.assert_called_once()`
		`+`
		`+ def test_no_tag_arches(self):`
		`+ self.handlers['getBuildConfig'].return_value = {'arches': ''}`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+ self.handlers['listHosts'].assert_not_called()`
		`+ self.set_refusal.assert_not_called()`
		`+`
		`+ def test_mixed_hosts(self):`
		`+ good1 = [{'id': n, 'arches': 'x86_64 i686'} for n in range(0,5)]`
		`+ bad1 = [{'id': n, 'arches': 'ia64'} for n in range(5,10)]`
		`+ good2 = [{'id': n, 'arches': 'aarch64'} for n in range(10,15)]`
		`+ bad2 = [{'id': n, 'arches': 'sparc64'} for n in range(15,20)]`
		`+ hosts = good1 + bad1 + good2 + bad2`
		`+ self.handlers['listHosts'].return_value = hosts`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+`
		`+ # should only refuse the bad ones`
		`+ expect = [mock.call(h['id'], 100, soft=False, msg='automatic arch refusal') for h in bad1 + bad2]`
		`+ self.assertListEqual(self.set_refusal.mock_calls, expect)`
		`+`
		`+ def test_not_noarch(self):`
		`+ self.taskinfo['arch'] = 'x86_64'`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.task.isFinished.assert_not_called()`
		`+`
		`+ def test_other_method(self):`
		`+ self.taskinfo['method'] = 'build'`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.task.isFinished.assert_not_called()`
		`+`
		`+ def test_task_finished(self):`
		`+ self.task.isFinished.return_value = True`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.get_tag.assert_not_called()`
		`+`
		`+ def test_bad_tag(self):`
		`+ self.get_tag.return_value = None`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.context.handlers.call.assert_not_called()`
		`+`
		`+ def test_bad_params(self):`
		`+ self.taskinfo['request'] = []`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.get_tag.assert_not_called()`
		`+`
		`+ def test_unexpected_error(self):`
		`+ self.get_tag.side_effect = MyError('should be caught')`
		`+`
		`+ # the wrapper should catch this`
		`+ scheduler.auto_arch_refuse(100)`
		`+`
		`+ self.context.handlers.call.assert_not_called()`
		`+`
		`+ def test_unexpected_error2(self):`
		`+ self.get_tag.side_effect = MyError('should not be caught')`
		`+`
		`+ # the underlying call should not`
		`+ with self.assertRaises(MyError):`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.context.handlers.call.assert_not_called()`
		`+`
		`+ def test_from_scm(self):`
		`+ self.taskinfo['method'] = 'buildSRPMFromSCM'`
		`+ self.taskinfo['request'] = [`
		`+ 'git+https://HOST/PATH',`
		`+ 'TAG_ID',`
		`+ {'repo_id': 8075973, 'scratch': None}]`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+ self.set_refusal.assert_not_called()`
		`+`
		`+ def test_from_srpm(self):`
		`+ self.taskinfo['method'] = 'rebuildSRPM'`
		`+ self.taskinfo['request'] = [`
		`+ 'cli-build/1709137799.6498768.BFGhzghk/fake-1.1-35.src.rpm',`
		`+ 'TAG_ID',`
		`+ {'repo_id': 2330, 'scratch': True}]`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+ self.set_refusal.assert_not_called()`
		`+`
		`+ def test_wrapper(self):`
		`+ self.taskinfo['method'] = 'wrapperRPM'`
		`+ self.taskinfo['request'] = [`
		`+ 'git://HOST/PATH',`
		`+ 'TARGET',`
		`+ {'build_id': 421},`
		`+ None,`
		`+ {'repo_id': 958, 'scratch': True}]`
		`+ self.get_build_target.return_value = {'build_tag': 'TAG_ID'}`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_build_target.assert_called_once_with('TARGET')`
		`+ self.get_tag.assert_called_once_with('TAG_ID')`
		`+ self.set_refusal.assert_not_called()`
		`+`
		`+ def test_bad_target(self):`
		`+ self.taskinfo['method'] = 'wrapperRPM'`
		`+ self.taskinfo['request'] = [`
		`+ 'git://HOST/PATH',`
		`+ 'TARGET',`
		`+ {'build_id': 421},`
		`+ None,`
		`+ {'repo_id': 958, 'scratch': True}]`
		`+ self.get_build_target.return_value = None`
		`+`
		`+ scheduler._auto_arch_refuse(100)`
		`+`
		`+ self.Task.assert_called_once_with(100)`
		`+ self.get_build_target.assert_called_once_with('TARGET')`
		`+ self.get_tag.assert_not_called()`

mikem commented 10 months ago

This is not a proper fix, but could serve as a temporary workaround.

I've run some basic tests, but nothing comprehensive. Posting for discussion.

tkopecek commented on line 13 of kojihub/scheduler.py 10 months ago

rebuildSRPM is here twice.

1 new commit added

typo

10 months ago

1 new commit added

add unit test and adjust error handling

10 months ago

mikem commented 10 months ago

Simple local test succeeds:

[mikem@localhost koji]$ lkoji scheduler-logs --task 13852
Task    Host    Time    Message
----------------------
13852   builder-02  Wed Mar 20 17:16:01 2024    Host refused task: automatic arch refusal
13852   builder-01  Wed Mar 20 17:16:01 2024    Assigning task

1 new commit added

one more test

10 months ago

1 new commit added

adjust comments

9 months ago

rebased onto b93ae26badd1c54e56a4c88419ce356dfd3fcbd6

9 months ago

mikem commented 9 months ago

rebased with no conflicts. unit tests pass

mikem commented 9 months ago

This has been working for us, and in retrospect it's not that bad of a hack.

This is definitely not the way that I think this should be fixed, but I think this can serve as a temporary workaround until we can make the scheduler a bit smarter.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

9 months ago

1 new commit added

adjust handling for tags with no arches

6 months ago

mikem commented 6 months ago

last change fixes a couple issues. FIrst, we need to use getBuildConfig to match checkHostArch. Second, we need to avoid refusing all hosts when the tag has no arches.

rebased onto f84fff1

6 months ago

mikem commented 6 months ago

rebased with no conflicts