From c03d23c0f275cf4169b8b20064a07e514da9a47a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 20 2024 20:18:14 +0000 Subject: [PATCH 1/7] kojira: adjust expire_check --- diff --git a/util/kojira b/util/kojira index 5e98f39..ac60546 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 81433e588369e5adcfdaeea1b91626039d29a967 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 20 2024 20:26:32 +0000 Subject: [PATCH 2/7] tweak unit test --- diff --git a/tests/test_kojira/test_repo_manager.py b/tests/test_kojira/test_repo_manager.py index 85ec47e..21e4116 100644 --- a/tests/test_kojira/test_repo_manager.py +++ b/tests/test_kojira/test_repo_manager.py @@ -183,8 +183,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 @@ -206,6 +207,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 99f001e1807e404f9642d0237cc2e8385a65e6f9 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 21 2024 18:13:33 +0000 Subject: [PATCH 3/7] recheck_period for expire check --- diff --git a/util/kojira b/util/kojira index ac60546..3a571df 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 @@ -835,7 +841,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, @@ -849,7 +856,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') bool_opts = ('verbose', 'debug', 'ignore_stray_repos', 'offline_retry', @@ -889,6 +896,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 b2a7f90143fd872439e834a2731a01cbf1f1b561 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Nov 21 2024 18:13:33 +0000 Subject: [PATCH 4/7] unit tests --- diff --git a/tests/test_kojira/test_managed_repo.py b/tests/test_kojira/test_managed_repo.py index 56bda2a..9d19b6a 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 55d0dbef06bf38041ead2911f8e23a9f0be37c6a Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jan 17 2025 15:33:26 +0000 Subject: [PATCH 5/7] avoid expiring latest dist repos also Fixes: https://pagure.io/koji/issue/4295 --- diff --git a/util/kojira b/util/kojira index 3a571df..416fa57 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""" @@ -666,10 +668,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 c56a9c84ef552e4c2cac2ab962f82e861484b482 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jan 17 2025 16:57:18 +0000 Subject: [PATCH 6/7] target check makes no sense for dist repos --- diff --git a/util/kojira b/util/kojira index 416fa57..4cb55d1 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 547342e41e38485877d7c862424898e446acdfd5 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Jan 17 2025 17:08:48 +0000 Subject: [PATCH 7/7] unit test --- diff --git a/tests/test_kojira/test_managed_repo.py b/tests/test_kojira/test_managed_repo.py index 9d19b6a..df8a932 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