PR#4270: keep latest default repo for build tags

Merges #4270
https://pagure.io/koji/pull-request/4270

Fixes: #4276
https://pagure.io/koji/issue/4276
Kojira can leave build tags with no repo at all

Fixes: #4295
https://pagure.io/koji/issue/4295
kojiria seems to be gc'ing latest repos-dist

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,134 @@ 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_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
+        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()
+
+    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

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

util/kojira

@@ -144,24 +144,57 @@ 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
-            return
+            # this covers current dist repos, where custom_opts=None
-        # otherwise repo is either obsolete or custom
-        if self.get_age() > self.options.repo_lifetime:
-            self.expire()
-
-    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:
+        # keep repos for configured lifetime
-            self.logger.info("Expiring dist repo %(id)s for tag %(tag_name)s", self.data)
+        if self.dist:
-            self.expire()
+            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
+        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 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
+
+        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],
+            ['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 delete_check(self):
         """Delete the repo if appropriate"""
@@ -618,10 +651,7 @@ class RepoManager(object):
             if repo.state == koji.REPO_INIT:
                 repo.check_init()
             elif repo.state == koji.REPO_READY:
-                if repo.dist:
+                repo.expire_check()
-                    repo.dist_expire_check()
-                else:
-                    repo.expire_check()
             elif repo.state == koji.REPO_EXPIRED:
                 repo.delete_check()
             elif repo.state == koji.REPO_PROBLEM:
@@ -756,7 +786,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,
@@ -770,7 +801,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',
@@ -810,6 +841,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