Merge lp:~sil2100/ubuntu-cdimage/purge-core-images into lp:ubuntu-cdimage

[Łukasz Zemczak]

Add support for another retention policy: purge-count. Refactor some of the purge code to support this better. Add tests. By default keep up to 5 core images.

Add support for another retention policy: purge-count. Refactor some of the purge code to support this better. Add tests. By default keep up to 5 core images.

This is my proposition of adding an additional purge policy to cdimage. The way it works is basically if no image-count based set is defined, the purge mechanics should work as they did before. One can define only one of them or both at once. In case a combination of age-based and count-based purging is defined, the first one to cause a purge wins. Example:

 * If one defines keeping 3 days worth of images and up to 5 images, and 4 days-worth of images appear, the oldest one will be purged (leaving 3 images per age-count).
 * If one defines keeping 5 days worth of images and up to 3 images, and 4 days-worth of images appear, the oldest one will be purged (leaving 3 images per image-count).

This can be useful in some future cases where we want to have this mixture of 'keep 3 days worth of images but not more than 5 at once'.

[Steve Langasek (vorlon)]

Mostly looks good, one comment inline, the other issue is that I don't actually see anywhere that you've implemented the "keep up to 5 core images" part of this.

review: Needs Fixing

[Łukasz Zemczak (sil2100)]

eeek, I think I forgot a bzr add for the up-to-5-days. Let me find the bzr branch and re-push. Thanks for the review as well! Let me also look into your inline proposition.

[Łukasz Zemczak (sil2100)]

Commented inline. Should I rewrite the part to handle both cases as an error, or should we just go with the approach presented here? Thanks for the review!

[lp:~sil2100/ubuntu-cdimage/purge-core-images]

1750. By Łukasz Zemczak on 2019-02-25

bzr add the missint etc/purge-count file.

[Steve Langasek (vorlon)]

It is still my preference that we error out instead of pruning images according to rules in two separate files. Better to have an error than data loss; and we can always revisit later if someone has a use case for pruning based on the intersection of the two rules. (Today, we have no such use case.)

[lp:~sil2100/ubuntu-cdimage/purge-core-images]

1751. By Łukasz Zemczak on 2019-03-06

As per Steve's request, do not support stating both purge-count and purge-days for a project - treat it as an error. Add test.

[Łukasz Zemczak (sil2100)]

Ok, this should be good for a final re-review. Thanks!

[Steve Langasek (vorlon)]

LGTM.

review: Approve

=== added file 'etc/purge-count'
--- etc/purge-count	1970-01-01 00:00:00 +0000
+++ etc/purge-count	2019-03-06 17:57:04 +0000
@@ -0,0 +1,2 @@
+# Keep up to 5 images for Ubuntu Core projects
+ubuntu-core 5
\ No newline at end of file

=== modified file 'etc/purge-days'
--- etc/purge-days	2018-04-20 01:15:18 +0000
+++ etc/purge-days	2019-03-06 17:57:04 +0000
@@ -5,7 +5,7 @@
 dvd 2
 # Only purge TheOpenCDv3 images manually.
 tocd3/daily-live 0
-# Only purge ubuntu-core images manually.
+# Ubuntu Core images are not purged per age
 ubuntu-core 0
 ubuntu-mid 3
 ubuntu-netbook 3

=== modified file 'lib/cdimage/tests/test_tree.py'
--- lib/cdimage/tests/test_tree.py	2019-02-27 21:21:04 +0000
+++ lib/cdimage/tests/test_tree.py	2019-03-06 17:57:04 +0000
@@ -1540,11 +1540,11 @@
             "20120807",
             ["ubuntu/daily-live/%s-desktop-i386" % self.config.series])
 
-    def test_get_purge_days_no_config(self):
+    def test_get_purge_data_no_config(self):
         publisher = self.make_publisher("ubuntu", "daily")
-        self.assertIsNone(publisher.get_purge_days("daily"))
+        self.assertIsNone(publisher.get_purge_data("daily", "purge-days"))
 
-    def test_get_purge_days(self):
+    def test_get_purge_data(self):
         publisher = self.make_publisher("ubuntu", "daily")
         with mkfile(os.path.join(
                 self.temp_dir, "etc", "purge-days")) as purge_days:
@@ -1553,9 +1553,28 @@
 
                 daily 1
                 daily-live 2"""), file=purge_days)
-        self.assertEqual(1, publisher.get_purge_days("daily"))
-        self.assertEqual(2, publisher.get_purge_days("daily-live"))
-        self.assertIsNone(publisher.get_purge_days("dvd"))
+        self.assertEqual(1, publisher.get_purge_data("daily", "purge-days"))
+        self.assertEqual(2, publisher.get_purge_data(
+            "daily-live", "purge-days"))
+        self.assertIsNone(publisher.get_purge_data("dvd", "purge-days"))
+
+    def test_get_purge_count_no_config(self):
+        publisher = self.make_publisher("ubuntu", "daily")
+        self.assertIsNone(publisher.get_purge_data("daily", "purge-count"))
+
+    def test_get_purge_count(self):
+        publisher = self.make_publisher("ubuntu", "daily")
+        with mkfile(os.path.join(
+                self.temp_dir, "etc", "purge-count")) as purge_count:
+            print(dedent("""\
+                # comment
+
+                daily 1
+                daily-live 2"""), file=purge_count)
+        self.assertEqual(1, publisher.get_purge_data("daily", "purge-count"))
+        self.assertEqual(2, publisher.get_purge_data(
+            "daily-live", "purge-count"))
+        self.assertIsNone(publisher.get_purge_data("dvd", "purge-count"))
 
     @mock.patch("time.time", return_value=date_to_time("20130321"))
     def test_purge_removes_old(self, *args):
@@ -1575,8 +1594,8 @@
             purge_desc = project
         self.assertLogEqual([
             "Purging %s/daily images older than 1 day ..." % project,
+            "Purging %s/daily/20130319" % purge_desc,
             "Purging %s/daily/20130318" % purge_desc,
-            "Purging %s/daily/20130319" % purge_desc,
         ])
         self.assertCountEqual(
             ["20130320", "20130321"], os.listdir(publisher.publish_base))
@@ -1673,11 +1692,81 @@
             purge_desc = project
         self.assertLogEqual([
             "Purging %s/daily images older than 1 day ..." % project,
-            "Purging %s/daily/20130319" % purge_desc,
             "Purging %s/daily/20130319.1" % purge_desc,
+            "Purging %s/daily/20130319" % purge_desc,
         ])
         self.assertEqual([], os.listdir(publisher.publish_base))
 
+    def test_purge_leaves_count(self, *args):
+        publisher = self.make_publisher("ubuntu", "daily")
+        for name in "20130318", "20130319", "20130320", "20130321":
+            touch(os.path.join(publisher.publish_base, name, "file"))
+        with mkfile(os.path.join(
+                self.temp_dir, "etc", "purge-count")) as purge_count:
+            print("daily 2", file=purge_count)
+        self.capture_logging()
+        publisher.purge()
+        if self.config["UBUNTU_DEFAULTS_LOCALE"]:
+            project = "ubuntu-zh_CN"
+            purge_desc = "%s/%s" % (project, self.config.series)
+        else:
+            project = "ubuntu"
+            purge_desc = project
+        self.assertLogEqual([
+            "Purging %s/daily images to leave only the latest 2 images "
+            "..." % project,
+            "Purging %s/daily/20130319" % purge_desc,
+            "Purging %s/daily/20130318" % purge_desc,
+        ])
+        self.assertCountEqual(
+            ["20130320", "20130321"], os.listdir(publisher.publish_base))
+
+    @mock.patch("time.time", return_value=date_to_time("20130321"))
+    def test_purge_both_days_and_count_raises(self, *args):
+        publisher = self.make_publisher("ubuntu", "daily")
+        for name in "20130321", "20130321.1", "20130321.2", "20130321.3":
+            touch(os.path.join(publisher.publish_base, name, "file"))
+        with mkfile(os.path.join(
+                self.temp_dir, "etc", "purge-days")) as purge_days:
+            print("daily 1", file=purge_days)
+        with mkfile(os.path.join(
+                self.temp_dir, "etc", "purge-count")) as purge_count:
+            print("daily 3", file=purge_count)
+        if self.config["UBUNTU_DEFAULTS_LOCALE"]:
+            project = "ubuntu-zh_CN"
+        else:
+            project = "ubuntu"
+        self.capture_logging()
+        self.assertRaisesRegex(
+            Exception, r"Both purge-days and purge-count are defined for "
+                       "%s/daily. Such scenario is currently "
+                       "unsupported." % project,
+            publisher.purge)
+        self.assertCountEqual(
+            ["20130321", "20130321.1", "20130321.2", "20130321.3"],
+            os.listdir(publisher.publish_base))
+
+    @mock.patch("time.time", return_value=date_to_time("20130321"))
+    def test_purge_no_purge(self, *args):
+        publisher = self.make_publisher("ubuntu", "daily")
+        for name in "20130318", "20130319", "20130320", "20130321":
+            touch(os.path.join(publisher.publish_base, name, "file"))
+        with mkfile(os.path.join(
+                self.temp_dir, "etc", "purge-days")) as purge_days:
+            print("daily 0", file=purge_days)
+        self.capture_logging()
+        publisher.purge()
+        if self.config["UBUNTU_DEFAULTS_LOCALE"]:
+            project = "ubuntu-zh_CN"
+        else:
+            project = "ubuntu"
+        self.assertLogEqual([
+            "Not purging images for %s/daily" % project,
+        ])
+        self.assertCountEqual(
+            ["20130318", "20130319", "20130320", "20130321"],
+            os.listdir(publisher.publish_base))
+
 
 class TestChinaDailyTree(TestDailyTree):
     def setUp(self):

=== modified file 'lib/cdimage/tree.py'
--- lib/cdimage/tree.py	2019-02-27 21:10:40 +0000
+++ lib/cdimage/tree.py	2019-03-06 17:57:04 +0000
@@ -2679,8 +2679,8 @@
 
         self.post_qa(date, published)
 
-    def get_purge_days(self, key):
-        path = os.path.join(self.config.root, "etc", "purge-days")
+    def get_purge_data(self, key, purge_type):
+        path = os.path.join(self.config.root, "etc", purge_type)
         try:
             with open(path) as purge_days:
                 for line in purge_days:
@@ -2697,7 +2697,7 @@
                 raise
         return None
 
-    def purge(self, days=None):
+    def purge(self, days=None, count=None):
         project = self.project
         if self.config["UBUNTU_DEFAULTS_LOCALE"]:
             project = "-".join(
@@ -2705,25 +2705,48 @@
         project_image_type = "%s/%s" % (project, self.image_type)
 
         if days is None:
-            days = self.get_purge_days(project)
-        if days is None:
-            days = self.get_purge_days(project_image_type)
-        if days is None:
-            days = self.get_purge_days(self.image_type)
-        if days is None:
-            logger.info("No purge time configured for %s" % project_image_type)
-            return
-        if days == 0:
+            days = self.get_purge_data(project, "purge-days")
+        if days is None:
+            days = self.get_purge_data(project_image_type, "purge-days")
+        if days is None:
+            days = self.get_purge_data(self.image_type, "purge-days")
+
+        if count is None:
+            count = self.get_purge_data(project, "purge-count")
+        if count is None:
+            count = self.get_purge_data(project_image_type, "purge-count")
+        if count is None:
+            count = self.get_purge_data(self.image_type, "purge-count")
+
+        if not days and not count:
             logger.info("Not purging images for %s" % project_image_type)
             return
-        logger.info(
-            "Purging %s images older than %d %s ..." %
-            (project_image_type, days, "day" if days == 1 else "days"))
-        oldest = time.strftime(
-            "%Y%m%d", time.gmtime(time.time() - 60 * 60 * 24 * days))
+        elif days and count:
+            raise Exception("Both purge-days and purge-count are defined for "
+                            "%s. Such scenario is currently unsupported." %
+                            project_image_type)
+
+        image_count = 0
+        oldest = 0
+
+        if days:
+            logger.info(
+                "Purging %s images older than %d %s ..." %
+                (project_image_type, days, "day" if days == 1 else "days"))
+            oldest = int(time.strftime(
+                "%Y%m%d", time.gmtime(time.time() - 60 * 60 * 24 * days)))
+        elif count:
+            logger.info(
+                "Purging %s images to leave only the latest %d %s ..." %
+                (project_image_type, count,
+                 "image" if count == 1 else "images"))
+
         to_purge = []
+        publish_pending = os.path.join(self.publish_base, "pending")
+        publish_current = os.path.join(self.publish_base, "current")
 
-        for entry in sorted(osextras.listdir_force(self.publish_base)):
+        for entry in sorted(
+                osextras.listdir_force(self.publish_base), reverse=True):
             entry_path = os.path.join(self.publish_base, entry)
 
             # Directory?
@@ -2734,16 +2757,21 @@
             if not entry[0].isdigit():
                 continue
 
+            image_count += 1
+
             # Older than cut-off date?
-            if int(oldest) <= int(entry.split(".", 1)[0]):
+            # Did we leave enough images already?
+            # In the case where both cut-off date and image count have been
+            # defined, we purge anything that doesn't satisfy both of the above
+            # conditions at once
+            if ((not days or oldest <= int(entry.split(".", 1)[0])) and
+                    (not count or image_count <= count)):
                 continue
 
             # Pointed to by "pending" or "current" symlink?
-            publish_pending = os.path.join(self.publish_base, "pending")
             if (os.path.islink(publish_pending) and
                     os.readlink(publish_pending) == entry):
                 continue
-            publish_current = os.path.join(self.publish_base, "current")
             if os.path.islink(publish_current):
                 if os.readlink(publish_current) == entry:
                     continue
@@ -2763,6 +2791,7 @@
                             break
                 if found_current:
                     continue
+
             to_purge.append((entry, entry_path))
 
         for entry, entry_path in to_purge: