Merge ~ilasc/launchpad:git-repack-cron-script into launchpad:master

[Ioana Lasc]

Add cron script for git repack

8e67df7... by Ioana Lasc on 2021-03-02

Organize imports

[Ioana Lasc (ilasc)]

The unit test (test_request_git_repack) doesn't seem to trigger the script, this is really a copy/edit/debug and attempt to understand how we trigger daily builds, but not getting far on this.

7ce4087... by Ioana Lasc on 2021-03-04

Move repackgitrepository to lp/code/scripts

51ca1f4... by Ioana Lasc on 2021-03-04

Cleanup unnecessary git-repack user

[Ioana Lasc (ilasc)]

Thanks Colin, MP is now ready for review.

[Colin Watson (cjwatson)]

Looks like a good first attempt, thanks!

This is potentially quite operationally expensive, so in this review I've paid special attention to things that I think could make it generally kinder and gentler to our infrastructure; the "Needs Fixing" is mainly for those.

review: Needs Fixing

3020f70... by Ioana Lasc on 2021-03-10

Add TunableLoop to git repack automation

[Ioana Lasc (ilasc)]

Thanks Colin, excellent suggestions as usual!

This is now working with a TunableLoop but needs more work: the TunableLoop doesn't hook into the fake turnip thread for the calls to gitHostingClient and it is oops-ing with: https://pastebin.canonical.com/p/6RKM3kfkTT/
I'm working on getting the LaunchpadCronScript and TunableLoop to interact with Fake Turnip Thread in unit test instead of trying to contact the real application.

In terms of the permissions added to the branchscanner user, this is how it crashed if SELECT rights are not granted to the user:https://pastebin.canonical.com/p/qVwRKjztzM/
And it appears I've gone overboard with permissions as UPDATE is not necessary, so will be taking that out.

I didn't miss your last comment about adding at least another test for the case where we have more than single repository qualifying for repack. As soon as I get the Unit Tests to play nicely with the FakeTurnipApplication will be adding that in too.

82b632e... by Ioana Lasc on 2021-03-10

Set default timeout function for RepackTunableLoop

5f84608... by Ioana Lasc on 2021-03-12

Add unit tests for RepackTunableLoop

[Ioana Lasc (ilasc)]

MP now ready for review.

[Colin Watson (cjwatson)]

Thanks for the updates. Various details follow; at least the index needs to be sorted out before landing this.

review: Approve

a078850... by Ioana Lasc on 2021-03-15

Address review comments for repack cron job

diff --git a/cronscripts/repack_git_repositories.py b/cronscripts/repack_git_repositories.py
 new file mode 100755
index 0000000..e553616
--- /dev/null
+++ b/cronscripts/repack_git_repositories.py
@@ -0,0 +1,33 @@
+#!/usr/bin/python2 -S
+#
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+import _pythonpath
+
+from lp.code.scripts.repackgitrepository import RepackTunableLoop
+from lp.services.config import config
+from lp.services.scripts.base import LaunchpadCronScript
+from lp.services.timeout import set_default_timeout_function
+
+
+class RepackGitRepositories(LaunchpadCronScript):
+
+    def add_my_options(self):
+        self.parser.add_option(
+            "--dry-run", action="store_true",
+            dest="dry_run", default=False,
+            help="Reports which repositories would be repacked without "
+                 "actually repacking the repositories.")
+
+    def main(self):
+        set_default_timeout_function(
+            lambda: config.repack_git_repositories.timeout)
+        updater = RepackTunableLoop(self.logger, self.options.dry_run)
+        updater.run()
+
+
+if __name__ == '__main__':
+    script = RepackGitRepositories(
+        'repack_git_repositories',  dbuser='branchscanner')
+    script.lock_and_run()
diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index 2f4ffdf..2911da2 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -699,6 +699,7 @@ public.bugsubscriptionfilterimportance    = SELECT
 public.bugsubscriptionfilterstatus        = SELECT
 public.bugsubscriptionfiltertag           = SELECT
 public.bugtag                             = SELECT
+public.codeimport                         = SELECT
 public.codereviewmessage                  = SELECT
 public.codereviewvote                     = SELECT
 public.diff                               = SELECT, INSERT, DELETE
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 589dcbf..429b502 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -489,6 +489,7 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
 
     def repackRepository(self):
         getUtility(IGitHostingClient).repackRepository(self.getInternalPath())
+        self.date_last_repacked = UTC_NOW
 
     @property
     def namespace(self):
diff --git a/lib/lp/code/scripts/repackgitrepository.py b/lib/lp/code/scripts/repackgitrepository.py
 new file mode 100644
index 0000000..0c25ff6
--- /dev/null
+++ b/lib/lp/code/scripts/repackgitrepository.py
@@ -0,0 +1,97 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Functions used with the repack Git repositories script."""
+
+__metaclass__ = type
+
+from psycopg2.extensions import TransactionRollbackError
+import six
+from storm.expr import (
+    And,
+    Or,
+    )
+import transaction
+
+from lp.code.enums import GitRepositoryStatus
+from lp.code.errors import CannotRepackRepository
+from lp.code.model.gitrepository import GitRepository
+from lp.services.config import config
+from lp.services.database.interfaces import IStore
+from lp.services.looptuner import (
+    LoopTuner,
+    TunableLoop,
+    )
+
+
+class RepackTunableLoop(TunableLoop):
+    tuner_class = LoopTuner
+    maximum_chunk_size = 5
+
+    def __init__(self, log, dry_run, abort_time=None):
+        super(RepackTunableLoop, self).__init__(log, abort_time)
+        self.dry_run = dry_run
+        self.start_at = 0
+        self.logger = log
+        self.store = IStore(GitRepository)
+
+    def findRepackCandidates(self):
+        repos = self.store.find(
+            GitRepository,
+            (Or(
+                GitRepository.loose_object_count >=
+                config.codehosting.loose_objects_threshold,
+                GitRepository.status == GitRepositoryStatus.AVAILABLE,
+                GitRepository.pack_count >=
+                config.codehosting.packs_threshold
+                ),
+             And(GitRepository.id > self.start_at))).order_by(GitRepository.id)
+        return repos
+
+    def isDone(self):
+        return self.findRepackCandidates().is_empty()
+
+    def __call__(self, chunk_size):
+        repackable_repos = list(self.findRepackCandidates()[:chunk_size])
+        counter = 0
+        for repo in repackable_repos:
+            try:
+                if self.dry_run:
+                    print ('Would repack %s' % repo.identity)
+                else:
+                    self.logger.info(
+                        'Requesting automatic git repository repack for %s.'
+                        % repo.identity)
+                    repo.repackRepository()
+                    counter += 1
+            except CannotRepackRepository as e:
+                self.logger.error(
+                    'An error occurred while requesting repository repack %s'
+                    % e.message)
+                continue
+            except TransactionRollbackError as error:
+                self.logger.error(
+                    'An error occurred while requesting repository repack %s'
+                    % six.text_type(error))
+                if transaction is not None:
+                    transaction.abort()
+                continue
+
+        if self.dry_run:
+            print(
+                'Reporting %d automatic git repository repacks '
+                'would have been requested as part of this run '
+                'out of the %d qualifying for repack.'
+                % (counter, len(repackable_repos)))
+        else:
+            self.logger.info(
+                'Requested %d automatic git repository repacks '
+                'out of the %d qualifying for repack.'
+                % (counter, len(repackable_repos)))
+
+        self.start_at = repackable_repos[-1].id
+
+        if not self.dry_run:
+            transaction.commit()
+        else:
+            transaction.abort()
diff --git a/lib/lp/code/scripts/tests/test_repack_git_repositories.py b/lib/lp/code/scripts/tests/test_repack_git_repositories.py
 new file mode 100644
index 0000000..59585cd
--- /dev/null
+++ b/lib/lp/code/scripts/tests/test_repack_git_repositories.py
@@ -0,0 +1,154 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the repack_git_repositories script."""
+import threading
+from wsgiref.simple_server import (
+    make_server,
+    WSGIRequestHandler,
+    )
+
+import transaction
+from zope.security.proxy import removeSecurityProxy
+
+from lp.services.config import config
+from lp.services.config.fixture import (
+    ConfigFixture,
+    ConfigUseFixture,
+    )
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import ZopelessAppServerLayer
+from lp.testing.script import run_script
+
+
+class SilentWSGIRequestHandler(WSGIRequestHandler):
+    """A request handler that doesn't log requests."""
+
+    def log_message(self, fmt, *args):
+        pass
+
+
+class FakeTurnipApplication:
+    """A WSGI application that provides a fake turnip endpoint."""
+
+    def __init__(self):
+        self.contents = []
+
+    def __call__(self, environ, start_response):
+        self.contents.append(environ['PATH_INFO'])
+        start_response('200 OK', [('Content-Type', 'text/plain')])
+        return [b'']
+
+
+class FakeTurnipServer(threading.Thread):
+    """Thread that runs a fake turnip server."""
+
+    def __init__(self):
+        super(FakeTurnipServer, self).__init__()
+        self.name = 'FakeTurnipServer'
+        self.app = FakeTurnipApplication()
+        self.server = make_server(
+            'localhost', 0, self.app, handler_class=SilentWSGIRequestHandler)
+
+    def run(self):
+        self.server.serve_forever()
+
+    def getURL(self):
+        host, port = self.server.server_address
+        return 'http://%s:%d/' % (host, port)
+
+    def stop(self):
+        self.server.shutdown()
+
+
+class TestRequestGitRepack(TestCaseWithFactory):
+
+    layer = ZopelessAppServerLayer
+
+    def setUp(self):
+        super(TestRequestGitRepack, self).setUp()
+
+    def runScript_no_Turnip(self):
+        transaction.commit()
+
+        (ret, out, err) = run_script('cronscripts/repack_git_repositories.py')
+        self.assertIn(
+            'An error occurred while requesting repository repack',
+            err)
+        self.assertIn(
+            'Failed to repack Git repository 1', err)
+        self.assertIn(
+            'Requested 0 automatic git repository '
+            'repacks out of the 1 qualifying for repack.', err)
+        transaction.commit()
+
+    def runScript_with_Turnip(self):
+        transaction.commit()
+        (ret, out, err) = run_script('cronscripts/repack_git_repositories.py')
+        self.assertIn(
+            'Requested 1 automatic git repository repacks '
+            'out of the 1 qualifying for repack.', err)
+        transaction.commit()
+
+    def makeTurnipServer(self):
+        self.turnip_server = FakeTurnipServer()
+        config_name = self.factory.getUniqueString()
+        config_fixture = self.useFixture(ConfigFixture(
+            config_name, config.instance_name))
+        setting_lines = [
+            '[codehosting]',
+            'internal_git_api_endpoint: %s' % self.turnip_server.getURL(),
+            ]
+        config_fixture.add_section('\n' + '\n'.join(setting_lines))
+        self.useFixture(ConfigUseFixture(config_name))
+        self.turnip_server.start()
+        self.addCleanup(self.turnip_server.stop)
+        return self.turnip_server
+
+    def test_auto_repack_without_Turnip(self):
+        repo = self.factory.makeGitRepository()
+        repo = removeSecurityProxy(repo)
+        repo.loose_object_count = 7000
+        repo.pack_count = 43
+
+        # Do not start the fake turnip server here
+        # to test if the RequestGitRepack will catch and
+        # log correctly the failure to establish
+        # a connection to Turnip
+        self.runScript_no_Turnip()
+        self.assertIsNone(repo.date_last_repacked)
+
+    def test_auto_repack_with_Turnip_one_repo(self):
+        # Test repack works when only one repository
+        # qualifies for a repack
+        repo = self.factory.makeGitRepository()
+        repo = removeSecurityProxy(repo)
+        repo.loose_object_count = 7000
+        repo.pack_count = 43
+        transaction.commit()
+
+        self.makeTurnipServer()
+
+        self.runScript_with_Turnip()
+
+        self.assertIsNotNone(repo.date_last_repacked)
+
+    def test_auto_repack_with_Turnip_multiple_repos(self):
+        # Test repack works when 10 repositories
+        # qualifies for a repack
+        repo = []
+        for i in range(10):
+            repo.append(self.factory.makeGitRepository())
+            repo[i] = removeSecurityProxy(repo[i])
+            repo[i].loose_object_count = 7000
+            repo[i].pack_count = 43
+        transaction.commit()
+
+        self.makeTurnipServer()
+
+        self.runScript_with_Turnip()
+
+        for i in range(10):
+            self.assertIsNotNone(repo[i].date_last_repacked)
+            self.assertEqual("/repo/%s/repack" % repo[i].getInternalPath(),
+                             self.turnip_server.app.contents[i])
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index fe28a8c..c676504 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -1495,6 +1495,9 @@ dbuser: request-daily-builds
 # datatype: integer
 timeout: 60
 
+[repack_git_repositories]
+# datatype: integer
+timeout: 15
 
 [revisionkarma]
 # The database user which will be used by this process.