launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/livefs-keep-binary-files-interval into lp:launchpad.

Commit message:
Allow configuring the binary file retention period of a LiveFS.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1832477 in Launchpad itself: "Allow configuring binary file retention period for live filesystems"
  https://bugs.launchpad.net/launchpad/+bug/1832477

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/livefs-keep-binary-files-interval/+merge/368703

This makes it more practical to deal with live filesystems that are expected to produce base images for use by Launchpad itself.  Setting the base image of course ensures that the corresponding LFA stays around, but we may very well want to set a base image from a live filesystem build that's more than a day old.

https://code.launchpad.net/~cjwatson/launchpad/db-livefs-keep-binary-files-interval/+merge/368702 is the corresponding DB change.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/livefs-keep-binary-files-interval into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2019-01-30 11:23:33 +0000
+++ lib/lp/scripts/garbo.py	2019-06-12 10:00:53 +0000
@@ -1574,13 +1574,14 @@
     target_table_class = LiveFSFile
     ids_to_prune_query = """
         SELECT DISTINCT LiveFSFile.id
-        FROM LiveFSFile, LiveFSBuild, LibraryFileAlias
+        FROM LiveFSFile, LiveFSBuild, LiveFS, LibraryFileAlias
         WHERE
             LiveFSFile.livefsbuild = LiveFSBuild.id
+            AND LiveFSBuild.livefs = LiveFS.id
             AND LiveFSFile.libraryfile = LibraryFileAlias.id
             AND LiveFSBuild.date_finished <
                 CURRENT_TIMESTAMP AT TIME ZONE 'UTC'
-                - CAST('1 day' AS interval)
+                - LiveFS.keep_binary_files_interval
             AND LibraryFileAlias.mimetype != 'text/plain'
         """
 

=== modified file 'lib/lp/scripts/tests/test_garbo.py'
--- lib/lp/scripts/tests/test_garbo.py	2019-01-30 11:23:33 +0000
+++ lib/lp/scripts/tests/test_garbo.py	2019-06-12 10:00:53 +0000
@@ -1513,10 +1513,12 @@
             'PopulateLatestPersonSourcePackageReleaseCache')
         self.assertEqual(spph_2.id, job_data['last_spph_id'])
 
-    def _test_LiveFSFilePruner(self, content_type, interval, expected_count=0):
+    def _test_LiveFSFilePruner(self, content_type, interval,
+                               keep_binary_files_days=None, expected_count=0):
         # Garbo should (or should not, if `expected_count=1`) remove LiveFS
         # files of MIME type `content_type` that finished more than
-        # `interval` days ago.
+        # `interval` days ago.  If `keep_binary_files_days` is not None, set
+        # that on the test LiveFS.
         now = datetime.now(UTC)
         switch_dbuser('testadmin')
         self.useFixture(FeatureFixture({LIVEFS_FEATURE_FLAG: u'on'}))
@@ -1524,7 +1526,8 @@
 
         db_build = self.factory.makeLiveFSBuild(
             date_created=now - timedelta(days=interval, minutes=15),
-            status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=10))
+            status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=10),
+            keep_binary_files_days=keep_binary_files_days)
         db_lfa = self.factory.makeLibraryFileAlias(content_type=content_type)
         db_file = self.factory.makeLiveFSFile(
             livefsbuild=db_build, libraryfile=db_lfa)
@@ -1537,7 +1540,8 @@
         self.assertEqual(expected_count, store.find(LiveFSFile).count())
 
     def test_LiveFSFilePruner_old_binary_files(self):
-        # LiveFS binary files attached to builds over a day old are pruned.
+        # By default, LiveFS binary files attached to builds over a day old
+        # are pruned.
         self._test_LiveFSFilePruner('application/octet-stream', 1)
 
     def test_LiveFSFilePruner_old_text_files(self):
@@ -1550,6 +1554,19 @@
         self._test_LiveFSFilePruner(
             'application/octet-stream', 0, expected_count=1)
 
+    def test_LiveFSFilePruner_custom_interval_old_binary_files(self):
+        # If a custom retention interval is set, LiveFS binary files
+        # attached to builds over that interval old are pruned.
+        self._test_LiveFSFilePruner(
+            'application/octet-stream', 7, keep_binary_files_days=7)
+
+    def test_LiveFSFilePruner_custom_interval_recent_binary_files(self):
+        # If a custom retention interval is set, LiveFS binary files
+        # attached to builds less than that interval old are pruned.
+        self._test_LiveFSFilePruner(
+            'application/octet-stream', 6, keep_binary_files_days=7,
+            expected_count=1)
+
     def _test_SnapFilePruner(self, filename, job_status, interval,
                              expected_count=0):
         # Garbo should (or should not, if `expected_count=1`) remove snap

=== modified file 'lib/lp/soyuz/browser/livefs.py'
--- lib/lp/soyuz/browser/livefs.py	2018-07-16 00:51:23 +0000
+++ lib/lp/soyuz/browser/livefs.py	2019-06-12 10:00:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """LiveFS views."""
@@ -181,6 +181,7 @@
         'name',
         'require_virtualized',
         'relative_build_score',
+        'keep_binary_files_days',
         ])
     distro_series = Choice(
         vocabulary='BuildableDistroSeries', title=u'Distribution series')
@@ -282,13 +283,18 @@
 
     label = title
 
-    field_names = ['require_virtualized', 'relative_build_score']
+    field_names = [
+        'require_virtualized',
+        'relative_build_score',
+        'keep_binary_files_days',
+        ]
 
     @property
     def initial_values(self):
         return {
             'require_virtualized': self.context.require_virtualized,
             'relative_build_score': self.context.relative_build_score,
+            'keep_binary_files_days': self.context.keep_binary_files_days,
             }
 
 

=== modified file 'lib/lp/soyuz/interfaces/livefs.py'
--- lib/lp/soyuz/interfaces/livefs.py	2017-07-18 16:22:03 +0000
+++ lib/lp/soyuz/interfaces/livefs.py	2019-06-12 10:00:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Live filesystem interfaces."""
@@ -262,6 +262,13 @@
             "A delta to apply to all build scores for the live filesystem.  "
             "Builds with a higher score will build sooner.")))
 
+    keep_binary_files_days = exported(Int(
+        title=_("Binary file retention period"),
+        required=True, readonly=False,
+        description=_(
+            "Keep binary files attached to builds of this live filesystem "
+            "for at least this many days.")))
+
 
 class ILiveFSAdminAttributes(Interface):
     """`ILiveFS` attributes that can be edited by admins.

=== modified file 'lib/lp/soyuz/model/livefs.py'
--- lib/lp/soyuz/model/livefs.py	2017-07-18 16:22:03 +0000
+++ lib/lp/soyuz/model/livefs.py	2019-06-12 10:00:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -6,6 +6,9 @@
     'LiveFS',
     ]
 
+from datetime import timedelta
+import math
+
 import pytz
 from storm.locals import (
     Bool,
@@ -17,6 +20,7 @@
     Reference,
     Store,
     Storm,
+    TimeDelta,
     Unicode,
     )
 from zope.component import getUtility
@@ -113,8 +117,12 @@
 
     relative_build_score = Int(name='relative_build_score', allow_none=False)
 
+    keep_binary_files_interval = TimeDelta(
+        name='keep_binary_files_interval', allow_none=False)
+
     def __init__(self, registrant, owner, distro_series, name,
-                 metadata, require_virtualized, date_created):
+                 metadata, require_virtualized, keep_binary_files_days,
+                 date_created):
         """Construct a `LiveFS`."""
         if not getFeatureFlag(LIVEFS_FEATURE_FLAG):
             raise LiveFSFeatureDisabled
@@ -128,6 +136,23 @@
         self.relative_build_score = 0
         self.date_created = date_created
         self.date_last_modified = date_created
+        if keep_binary_files_days is None:
+            keep_binary_files_days = 1
+        self.keep_binary_files_interval = timedelta(
+            days=keep_binary_files_days)
+
+    @property
+    def keep_binary_files_days(self):
+        """See `ILiveFS`."""
+        # Rounding up preserves the "at least this many days" part of the
+        # contract, and makes the interface simpler.
+        return int(math.ceil(
+            self.keep_binary_files_interval.total_seconds() / 86400))
+
+    @keep_binary_files_days.setter
+    def keep_binary_files_days(self, days):
+        """See `ILiveFS`."""
+        self.keep_binary_files_interval = timedelta(days=days)
 
     def requestBuild(self, requester, archive, distro_arch_series, pocket,
                      unique_key=None, metadata_override=None, version=None):
@@ -230,7 +255,8 @@
     """See `ILiveFSSet`."""
 
     def new(self, registrant, owner, distro_series, name, metadata,
-            require_virtualized=True, date_created=DEFAULT):
+            require_virtualized=True, keep_binary_files_days=None,
+            date_created=DEFAULT):
         """See `ILiveFSSet`."""
         if not registrant.inTeam(owner):
             if owner.is_team:
@@ -248,7 +274,7 @@
         store = IMasterStore(LiveFS)
         livefs = LiveFS(
             registrant, owner, distro_series, name, metadata,
-            require_virtualized, date_created)
+            require_virtualized, keep_binary_files_days, date_created)
         store.add(livefs)
 
         return livefs

=== modified file 'lib/lp/soyuz/tests/test_livefs.py'
--- lib/lp/soyuz/tests/test_livefs.py	2019-01-25 11:47:20 +0000
+++ lib/lp/soyuz/tests/test_livefs.py	2019-06-12 10:00:53 +0000
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test live filesystems."""
@@ -122,6 +122,20 @@
         with celebrity_logged_in("buildd_admin"):
             livefs.relative_build_score = 100
 
+    def test_keep_binary_files_days(self):
+        # Buildd admins can change the binary file retention period of a
+        # LiveFS, but ordinary users cannot.
+        livefs = self.factory.makeLiveFS()
+        self.assertEqual(1, livefs.keep_binary_files_days)
+        with person_logged_in(livefs.owner):
+            self.assertRaises(
+                Unauthorized, setattr, livefs, "keep_binary_files_days", 2)
+        with celebrity_logged_in("buildd_admin"):
+            livefs.keep_binary_files_days = 2
+        self.assertEqual(
+            timedelta(days=2),
+            removeSecurityProxy(livefs).keep_binary_files_interval)
+
     def test_requestBuild(self):
         # requestBuild creates a new LiveFSBuild.
         livefs = self.factory.makeLiveFS()

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2019-05-06 14:22:34 +0000
+++ lib/lp/testing/factory.py	2019-06-12 10:00:53 +0000
@@ -4628,7 +4628,7 @@
 
     def makeLiveFS(self, registrant=None, owner=None, distroseries=None,
                    name=None, metadata=None, require_virtualized=True,
-                   date_created=DEFAULT):
+                   keep_binary_files_days=None, date_created=DEFAULT):
         """Make a new LiveFS."""
         if registrant is None:
             registrant = self.makePerson()
@@ -4642,7 +4642,9 @@
             metadata = {}
         livefs = getUtility(ILiveFSSet).new(
             registrant, owner, distroseries, name, metadata,
-            require_virtualized=require_virtualized, date_created=date_created)
+            require_virtualized=require_virtualized,
+            keep_binary_files_days=keep_binary_files_days,
+            date_created=date_created)
         IStore(livefs).flush()
         return livefs