From 8fb2d9acc19b29946f03ca5ddd4448c1a3ecdb30 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Wed, 20 Nov 2024 15:18:14 -0500 Subject: [PATCH 1/7] kojira: adjust expire_check --- util/kojira | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/util/kojira b/util/kojira index 6b6741de..cb1b78ad 100755 --- a/util/kojira +++ b/util/kojira @@ -150,9 +150,30 @@ class ManagedRepo(object): if self.data['end_event'] is None and not self.data['custom_opts']: # repo is current and has default options. keep it return - # otherwise repo is either obsolete or custom - if self.get_age() > self.options.repo_lifetime: - self.expire() + + # keep repos for configured lifetime + if self.get_age() <= self.options.repo_lifetime: + return + + # keep latest default repo for build tags + if not self.data['custom_opts']: + targets = self.session.get_build_targets(buildTagID=self.data['tag_id']) + if targets and self.is_latest(): + return + + self.expire() + + def is_latest(self): + """Check if repo is latest for its tag (not necessarily current)""" + # similar query to symlink_if_latest on hub + clauses = [ + ['tag_id', '=', self.data['tag_id']], + ['state', '=', koji.REPO_READY], + ['custom_opts', '=', '{}'], + ['create_event', '>', self.data['create_event']], + ] + newer = self.session.repo.query(clauses, ['id']) + return not newer # True if no newer matching repo def dist_expire_check(self): """Check to see if a dist repo should be expired""" From 0f78b9adb025592d85f4750904fb9f9772313ff9 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Wed, 20 Nov 2024 15:26:32 -0500 Subject: [PATCH 2/7] tweak unit test --- tests/test_kojira/test_repo_manager.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_kojira/test_repo_manager.py b/tests/test_kojira/test_repo_manager.py index 047d343d..b4327464 100644 --- a/tests/test_kojira/test_repo_manager.py +++ b/tests/test_kojira/test_repo_manager.py @@ -145,8 +145,9 @@ class RepoManagerTest(unittest.TestCase): self.assertEqual(set(self.mgr.repos), set([r['id'] for r in repos])) # using autospec so we can grab self from mock_calls + @mock.patch.object(kojira.ManagedRepo, 'is_latest', autospec=True) @mock.patch.object(kojira.ManagedRepo, 'delete_check', autospec=True) - def test_update_repos(self, delete_check): + def test_update_repos(self, delete_check, is_latest): self.options.init_timeout = 3600 self.options.repo_lifetime = 3600 * 24 self.options.dist_repo_lifetime = 3600 * 24 @@ -168,6 +169,7 @@ class RepoManagerTest(unittest.TestCase): repos[2]['state'] = koji.REPO_EXPIRED # do the run + is_latest.return_value = False self.session.repo.query.return_value = repos with mock.patch('time.time') as _time: _time.return_value = base_ts + 100 # shorter than all timeouts From c4948587a3da7f26cfdf95f897b7ef17f5ec9e2b Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Wed, 20 Nov 2024 22:32:57 -0500 Subject: [PATCH 3/7] recheck_period for expire check --- util/kojira | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/util/kojira b/util/kojira index cb1b78ad..5406ab6f 100755 --- a/util/kojira +++ b/util/kojira @@ -155,9 +155,15 @@ class ManagedRepo(object): if self.get_age() <= self.options.repo_lifetime: return - # keep latest default repo for build tags + # remaining checks are more expensive, don't recheck every cycle + last_check = getattr(self, 'expire_check_ts', None) + if last_check and time.time() - last_check < self.options.recheck_period: + return + self.expire_check_ts = time.time() + + # keep latest default repo for build tags, even if not current if not self.data['custom_opts']: - targets = self.session.get_build_targets(buildTagID=self.data['tag_id']) + targets = self.session.getBuildTargets(buildTagID=self.data['tag_id']) if targets and self.is_latest(): return @@ -777,7 +783,8 @@ def get_options(): 'expired_repo_lifetime': None, # default handled below 'deleted_repo_lifetime': None, # compat alias for expired_repo_lifetime 'init_timeout': 7200, - 'reference_recheck_period': 3600, + 'recheck_period': 3600, + 'reference_recheck_period': None, # defaults to recheck_period 'no_repo_effective_age': 2 * 24 * 3600, 'check_external_repos': True, 'sleeptime': 15, @@ -791,7 +798,7 @@ def get_options(): 'retry_interval', 'max_retries', 'offline_retry_interval', 'max_delete_processes', 'dist_repo_lifetime', 'sleeptime', 'expired_repo_lifetime', - 'repo_lifetime', 'reference_recheck_period') + 'repo_lifetime', 'recheck_period', 'reference_recheck_period') str_opts = ('topdir', 'server', 'user', 'password', 'logfile', 'principal', 'keytab', 'cert', 'serverca', 'ccache') bool_opts = ('verbose', 'debug', 'ignore_stray_repos', 'offline_retry', @@ -831,6 +838,8 @@ def get_options(): options.expired_repo_lifetime = options.deleted_repo_lifetime elif options.expired_repo_lifetime is None: options.expired_repo_lifetime = 7 * 24 * 3600 + if options.reference_recheck_period is None: + options.reference_recheck_period = options.recheck_period if options.logfile in ('', 'None', 'none'): options.logfile = None # special handling for cert defaults From d159356713b66b578731a7eb7a6cf35b46198390 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Wed, 20 Nov 2024 22:53:09 -0500 Subject: [PATCH 4/7] unit tests --- tests/test_kojira/test_managed_repo.py | 83 ++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/test_kojira/test_managed_repo.py b/tests/test_kojira/test_managed_repo.py index 56bda2a7..9d19b6a3 100644 --- a/tests/test_kojira/test_managed_repo.py +++ b/tests/test_kojira/test_managed_repo.py @@ -40,6 +40,7 @@ class ManagedRepoTest(unittest.TestCase): 'end_event': None, 'id': 2385, 'opts': {'debuginfo': False, 'separate_src': False, 'src': False}, + 'custom_opts': {}, 'state': 1, 'state_ts': 1710705227.166751, 'tag_id': 50, @@ -126,4 +127,86 @@ class ManagedRepoTest(unittest.TestCase): self.session.repo.setState.assert_called_once_with(self.repo.id, koji.REPO_DELETED) self.mgr.rmtree.assert_called_once_with(path) + def test_expire_check_recent(self): + self.options.repo_lifetime = 3600 * 24 + self.options.recheck_period = 3600 + base_ts = 444888888 + now = base_ts + 100 + + self.repo.data['state'] = koji.REPO_READY + self.repo.data['state_ts'] = base_ts + self.repo.data['end_event'] = 999 + + with mock.patch('time.time') as _time: + _time.return_value = now + self.repo.expire_check() + + # we should have stopped at the age check + self.session.getBuildTargets.assert_not_called() + self.session.repoExpire.assert_not_called() + + def test_expire_check_recheck(self): + self.options.repo_lifetime = 3600 * 24 + self.options.recheck_period = 3600 + base_ts = 444888888 + now = base_ts + self.options.repo_lifetime + 100 + + # recheck period still in effect + self.repo.expire_check_ts = now - 3500 + # otherwise eligible to expire + self.repo.data['state'] = koji.REPO_READY + self.repo.data['state_ts'] = base_ts + self.repo.data['end_event'] = 999 + + with mock.patch('time.time') as _time: + _time.return_value = now + self.repo.expire_check() + + self.session.getBuildTargets.assert_not_called() + self.session.repo.query.assert_not_called() + self.session.repoExpire.assert_not_called() + + def test_expire_check_latest(self): + self.options.repo_lifetime = 3600 * 24 + self.options.recheck_period = 3600 + base_ts = 444888888 + now = base_ts + self.options.repo_lifetime + 100 + + self.repo.data['state'] = koji.REPO_READY + self.repo.data['state_ts'] = base_ts + self.repo.data['end_event'] = 999 + # latest for a target, should not get expired + self.session.getBuildTargets.return_value = ['TARGET'] + self.session.repo.query.return_value = [] + + with mock.patch('time.time') as _time: + _time.return_value = now + self.repo.expire_check() + + self.session.getBuildTargets.assert_called_once() + self.session.repo.query.assert_called_once() + self.session.repoExpire.assert_not_called() + + def test_expire_check_expire(self): + self.options.repo_lifetime = 3600 * 24 + self.options.recheck_period = 3600 + base_ts = 444888888 + now = base_ts + self.options.repo_lifetime + 100 + + self.repo.data['state'] = koji.REPO_READY + self.repo.data['state_ts'] = base_ts + self.repo.data['end_event'] = 999 + # not latest + self.session.getBuildTargets.return_value = ['TARGET'] + self.session.repo.query.return_value = ['NEWER_REPO'] + + with mock.patch('time.time') as _time: + _time.return_value = now + self.repo.expire_check() + + self.session.getBuildTargets.assert_called_once() + self.session.repo.query.assert_called_once() + self.session.repoExpire.assert_called_once() + + # the end From c0690c821f9dfa22d01a5311ef7d6910a154332d Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Fri, 17 Jan 2025 10:32:59 -0500 Subject: [PATCH 5/7] avoid expiring latest dist repos also Fixes: https://pagure.io/koji/issue/4295 --- util/kojira | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/util/kojira b/util/kojira index 5406ab6f..36a3c08e 100755 --- a/util/kojira +++ b/util/kojira @@ -144,15 +144,20 @@ class ManagedRepo(object): return time.time() - self.first_seen def expire_check(self): - if self.state != koji.REPO_READY or self.dist: + if self.state != koji.REPO_READY: return if self.data['end_event'] is None and not self.data['custom_opts']: # repo is current and has default options. keep it + # this covers dist repos, where custom_opts=None return # keep repos for configured lifetime - if self.get_age() <= self.options.repo_lifetime: + if self.dist: + lifetime = self.options.dist_repo_lifetime + else: + lifetime = self.options.repo_lifetime + if self.get_age() <= lifetime: return # remaining checks are more expensive, don't recheck every cycle @@ -163,6 +168,7 @@ class ManagedRepo(object): # keep latest default repo for build tags, even if not current if not self.data['custom_opts']: + # this covers dist repos, where custom_opts=None targets = self.session.getBuildTargets(buildTagID=self.data['tag_id']) if targets and self.is_latest(): return @@ -175,21 +181,17 @@ class ManagedRepo(object): clauses = [ ['tag_id', '=', self.data['tag_id']], ['state', '=', koji.REPO_READY], - ['custom_opts', '=', '{}'], ['create_event', '>', self.data['create_event']], ] + if self.dist: + clauses.append(['dist', '=', True]) + else: + clauses.append(['dist', '=', False]) + clauses.append(['custom_opts', '=', '{}']) + # ^this clause is only for normal repos, dist repos have custom_opts=None newer = self.session.repo.query(clauses, ['id']) return not newer # True if no newer matching repo - def dist_expire_check(self): - """Check to see if a dist repo should be expired""" - if not self.dist or self.state != koji.REPO_READY: - return - - if self.get_age() > self.options.dist_repo_lifetime: - self.logger.info("Expiring dist repo %(id)s for tag %(tag_name)s", self.data) - self.expire() - def delete_check(self): """Delete the repo if appropriate""" @@ -645,10 +647,7 @@ class RepoManager(object): if repo.state == koji.REPO_INIT: repo.check_init() elif repo.state == koji.REPO_READY: - if repo.dist: - repo.dist_expire_check() - else: - repo.expire_check() + repo.expire_check() elif repo.state == koji.REPO_EXPIRED: repo.delete_check() elif repo.state == koji.REPO_PROBLEM: From e691d28caed84f060f21020399230cbfde5f27c4 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Fri, 17 Jan 2025 11:57:18 -0500 Subject: [PATCH 6/7] target check makes no sense for dist repos --- util/kojira | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/util/kojira b/util/kojira index 36a3c08e..f20f11df 100755 --- a/util/kojira +++ b/util/kojira @@ -149,7 +149,7 @@ class ManagedRepo(object): if self.data['end_event'] is None and not self.data['custom_opts']: # repo is current and has default options. keep it - # this covers dist repos, where custom_opts=None + # this covers current dist repos, where custom_opts=None return # keep repos for configured lifetime @@ -166,9 +166,13 @@ class ManagedRepo(object): return self.expire_check_ts = time.time() - # keep latest default repo for build tags, even if not current - if not self.data['custom_opts']: - # this covers dist repos, where custom_opts=None + # keep latest default repo in some cases, even if not current + if self.dist: + # no target check -- they are irrelevant for dist repos + if self.is_latest(): + return + elif not self.data['custom_opts']: + # normal repo, default options targets = self.session.getBuildTargets(buildTagID=self.data['tag_id']) if targets and self.is_latest(): return From 3f0de62389ef93b84260f2fd89aad5f4dcb03279 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Fri, 17 Jan 2025 12:02:18 -0500 Subject: [PATCH 7/7] unit test --- tests/test_kojira/test_managed_repo.py | 48 ++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/test_kojira/test_managed_repo.py b/tests/test_kojira/test_managed_repo.py index 9d19b6a3..df8a9324 100644 --- a/tests/test_kojira/test_managed_repo.py +++ b/tests/test_kojira/test_managed_repo.py @@ -187,6 +187,30 @@ class ManagedRepoTest(unittest.TestCase): self.session.repo.query.assert_called_once() self.session.repoExpire.assert_not_called() + def test_expire_check_latest_dist(self): + self.options.dist_repo_lifetime = 3600 * 24 + self.options.recheck_period = 3600 + base_ts = 444888888 + now = base_ts + self.options.dist_repo_lifetime + 100 + + self.repo.data['dist'] = True + self.repo.dist = True + self.repo.data['state'] = koji.REPO_READY + self.repo.data['state_ts'] = base_ts + self.repo.data['end_event'] = 999 + # latest for a target, should not get expired + self.session.getBuildTargets.return_value = ['TARGET'] + self.session.repo.query.return_value = [] + + with mock.patch('time.time') as _time: + _time.return_value = now + self.repo.expire_check() + + self.session.getBuildTargets.assert_not_called() + # no target check for dist repos + self.session.repo.query.assert_called_once() + self.session.repoExpire.assert_not_called() + def test_expire_check_expire(self): self.options.repo_lifetime = 3600 * 24 self.options.recheck_period = 3600 @@ -208,5 +232,29 @@ class ManagedRepoTest(unittest.TestCase): self.session.repo.query.assert_called_once() self.session.repoExpire.assert_called_once() + def test_expire_check_expire_dist(self): + self.options.dist_repo_lifetime = 3600 * 24 + self.options.recheck_period = 3600 + base_ts = 444888888 + now = base_ts + self.options.dist_repo_lifetime + 100 + + self.repo.data['dist'] = True + self.repo.dist = True + self.repo.data['state'] = koji.REPO_READY + self.repo.data['state_ts'] = base_ts + self.repo.data['end_event'] = 999 + # not latest + self.session.getBuildTargets.return_value = ['TARGET'] + self.session.repo.query.return_value = ['NEWER_REPO'] + + with mock.patch('time.time') as _time: + _time.return_value = now + self.repo.expire_check() + + self.session.getBuildTargets.assert_not_called() + # no target check for dist repos + self.session.repo.query.assert_called_once() + self.session.repoExpire.assert_called_once() + # the end