launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Tom Wardill has proposed merging ~twom/launchpad:garboci into launchpad:master.

Commit message:
Add garbage collection for OCIFile

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/384121

Adding a column for date_last_used, delete any OCIFiles and the matching librarian files that haven't been used in an image for the last 7 days.

I think this should be safe, as the buildbehaviour updates the date_last_used on retrieval from the buildd. As long as there isn't more than 7 days between retrieval and upload, the files should still be available to put on disk before upload.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:garboci into launchpad:master.

diff --git a/database/schema/security.cfg b/database/schema/security.cfg
index f89f620..93dc9f8 100644
--- a/database/schema/security.cfg
+++ b/database/schema/security.cfg
@@ -997,7 +997,7 @@ public.libraryfilecontent                     = SELECT, INSERT
 public.livefs                                 = SELECT
 public.livefsbuild                            = SELECT, UPDATE
 public.livefsfile                             = SELECT
-public.ocifile                                = SELECT
+public.ocifile                                = SELECT, UPDATE
 public.ociproject                             = SELECT
 public.ociprojectname                         = SELECT
 public.ocipushrule                            = SELECT
@@ -2440,6 +2440,7 @@ public.livefsfile                       = SELECT, DELETE
 public.logintoken                       = SELECT, DELETE
 public.mailinglistsubscription          = SELECT, DELETE
 public.milestonetag                     = SELECT
+public.ocifile                          = SELECT, DELETE
 public.ociproject                       = SELECT
 public.ociprojectseries                 = SELECT
 public.ocirecipe                        = SELECT
diff --git a/lib/lp/oci/interfaces/ocirecipebuild.py b/lib/lp/oci/interfaces/ocirecipebuild.py
index 95539c0..5d2b3d5 100644
--- a/lib/lp/oci/interfaces/ocirecipebuild.py
+++ b/lib/lp/oci/interfaces/ocirecipebuild.py
@@ -83,13 +83,6 @@ class OCIRecipeBuildRegistryUploadStatus(EnumeratedType):
         """)
 
 
-class IOCIFileSet(Interface):
-    """A file artifact of an OCIRecipeBuild."""
-
-    def getByLayerDigest(layer_file_digest):
-        """Return an `IOCIFile` with the matching layer_file_digest."""
-
-
 class IOCIRecipeBuildView(IPackageBuild):
     """`IOCIRecipeBuild` attributes that require launchpad.View permission."""
 
@@ -256,3 +249,15 @@ class IOCIFile(Interface):
                 "a build to a registry. This hash is in an opaque format "
                 "generated by the OCI build tool."),
         required=False, readonly=True)
+
+    date_last_used = Datetime(
+        title=_("The datetime this file was last used in a build."),
+        required=True,
+        readonly=False)
+
+
+class IOCIFileSet(Interface):
+    """A file artifact of an OCIRecipeBuild."""
+
+    def getByLayerDigest(layer_file_digest):
+        """Return an `IOCIFile` with the matching layer_file_digest."""
diff --git a/lib/lp/oci/model/ocirecipebuild.py b/lib/lp/oci/model/ocirecipebuild.py
index 94b7326..de6ed3c 100644
--- a/lib/lp/oci/model/ocirecipebuild.py
+++ b/lib/lp/oci/model/ocirecipebuild.py
@@ -91,6 +91,9 @@ class OCIFile(Storm):
 
     layer_file_digest = Unicode(name='layer_file_digest', allow_none=True)
 
+    date_last_used = DateTime(
+        name='date_last_used', tzinfo=pytz.UTC, allow_none=False)
+
     def __init__(self, build, library_file, layer_file_digest=None):
         """Construct a `OCIFile`."""
         super(OCIFile, self).__init__()
diff --git a/lib/lp/oci/model/ocirecipebuildbehaviour.py b/lib/lp/oci/model/ocirecipebuildbehaviour.py
index 6205cfd..8b5f5e0 100644
--- a/lib/lp/oci/model/ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/model/ocirecipebuildbehaviour.py
@@ -14,14 +14,16 @@ __all__ = [
     ]
 
 
+from datetime import datetime
 import json
 import os
 
+import pytz
 from twisted.internet import defer
 from zope.component import getUtility
 from zope.interface import implementer
+from zope.security.proxy import removeSecurityProxy
 
-from lp.app.errors import NotFoundError
 from lp.buildmaster.enums import BuildBaseImageType
 from lp.buildmaster.interfaces.builder import (
     BuildDaemonError,
@@ -141,6 +143,8 @@ class OCIRecipeBuildBehaviour(SnapProxyMixin, BuildFarmJobBehaviourBase):
                     digest)
                 if oci_file:
                     librarian_file = oci_file.library_file
+                    unsecure_file = removeSecurityProxy(oci_file)
+                    unsecure_file.date_last_used = datetime.now(pytz.UTC)
                 # If it doesn't, we need to download it
                 else:
                     files.add(layer_filename)
diff --git a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
index cf0056b..87917f4 100644
--- a/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
+++ b/lib/lp/oci/tests/test_ocirecipebuildbehaviour.py
@@ -18,6 +18,7 @@ import uuid
 
 import fixtures
 from fixtures import MockPatch
+import pytz
 from six.moves.urllib_parse import urlsplit
 from testtools import ExpectedException
 from testtools.matchers import (
@@ -523,14 +524,17 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
     @defer.inlineCallbacks
     def test_handleStatus_OK_reuse_from_other_build(self):
         """We should be able to reuse a layer file from a separate build."""
-        self.factory.makeOCIFile(
+        oci_file = self.factory.makeOCIFile(
             layer_file_digest=u'digest_2',
             content="layer 2 retrieved from librarian")
 
-        now = datetime.now()
+        now = datetime.now(pytz.UTC)
         mock_datetime = self.useFixture(MockPatch(
             'lp.buildmaster.model.buildfarmjobbehaviour.datetime')).mock
+        mock_oci_datetime = self.useFixture(MockPatch(
+            'lp.oci.model.ocirecipebuildbehaviour.datetime')).mock
         mock_datetime.now = lambda: now
+        mock_oci_datetime.now = lambda tzinfo=None: now
         with dbuser(config.builddmaster.dbuser):
             yield self.behaviour.handleStatus(
                 self.build.buildqueue_record, 'OK',
@@ -554,6 +558,7 @@ class TestHandleStatusForOCIRecipeBuild(MakeOCIBuildMixin,
         with open(layer_2_path, 'rb') as layer_2_fp:
             contents = layer_2_fp.read()
             self.assertEqual(contents, b'layer 2 retrieved from librarian')
+        self.assertEqual(now, oci_file.date_last_used)
 
     @defer.inlineCallbacks
     def test_handleStatus_OK_absolute_filepath(self):
diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
index 98c4b04..cf0bc4f 100644
--- a/lib/lp/scripts/garbo.py
+++ b/lib/lp/scripts/garbo.py
@@ -70,6 +70,7 @@ from lp.code.model.revision import (
     RevisionCache,
     )
 from lp.hardwaredb.model.hwdb import HWSubmission
+from lp.oci.model.ocirecipebuild import OCIFile
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
 from lp.registry.model.sourcepackagename import SourcePackageName
@@ -1569,6 +1570,19 @@ class SnapFilePruner(BulkPruner):
         """ % (SnapBuildJobType.STORE_UPLOAD.value, JobStatus.COMPLETED.value)
 
 
+class OCIFilePruner(BulkPruner):
+    """Prune old `OCIFile`s that have expired."""
+    target_table_class = OCIFile
+    ids_to_prune_query = """
+        SELECT DISTINCT OCIFile.id
+        FROM OCIFile
+        WHERE
+            OCIFile.date_last_used <
+                CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
+                - CAST('7 days' AS INTERVAL)
+        """
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
     script_name = None  # Script name for locking and database user. Override.
@@ -1851,6 +1865,7 @@ class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
         HWSubmissionEmailLinker,
         LiveFSFilePruner,
         LoginTokenPruner,
+        OCIFilePruner,
         ObsoleteBugAttachmentPruner,
         OldTimeLimitedTokenDeleter,
         POTranslationPruner,
diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index dfeeba8..97978cd 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -69,6 +69,8 @@ from lp.code.model.gitjob import (
     GitJob,
     GitRefScanJob,
     )
+from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
+from lp.oci.model.ocirecipebuild import OCIFile
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
@@ -1057,6 +1059,48 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         switch_dbuser('testadmin')
         self.assertEqual(1, store.find(SnapBuildJob).count())
 
+    def test_OCIFileJobPruner(self):
+        self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+        # Garbo removes files that haven't been used in 7 days
+        switch_dbuser('testadmin')
+        store = IMasterStore(OCIFile)
+        ocifile = self.factory.makeOCIFile()
+        ocifile.date_last_used = THIRTY_DAYS_AGO
+        self.assertEqual(1, store.find(OCIFile).count())
+
+        self.runDaily()
+
+        switch_dbuser('testadmin')
+        self.assertEqual(0, store.find(OCIFile).count())
+
+    def test_OCIFileJobPruner_doesnt_prune_recent(self):
+        self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+        # Garbo removes files that haven't been used in 7 days
+        switch_dbuser('testadmin')
+        store = IMasterStore(OCIFile)
+        self.factory.makeOCIFile()
+        self.assertEqual(1, store.find(OCIFile).count())
+
+        self.runDaily()
+
+        switch_dbuser('testadmin')
+        self.assertEqual(1, store.find(OCIFile).count())
+
+    def test_OCIFileJobPruner_mixed_dates(self):
+        self.useFixture(FeatureFixture({OCI_RECIPE_ALLOW_CREATE: 'on'}))
+        # Garbo removes files that haven't been used in 7 days
+        switch_dbuser('testadmin')
+        store = IMasterStore(OCIFile)
+        ocifile = self.factory.makeOCIFile()
+        ocifile.date_last_used = THIRTY_DAYS_AGO
+        self.factory.makeOCIFile()
+        self.assertEqual(2, store.find(OCIFile).count())
+
+        self.runDaily()
+
+        switch_dbuser('testadmin')
+        self.assertEqual(1, store.find(OCIFile).count())
+
     def test_WebhookJobPruner(self):
         # Garbo should remove jobs completed over 30 days ago.
         switch_dbuser('testadmin')