Merge lp:~jibel/ubuntu-cdimage/support_for_multilayer into lp:ubuntu-cdimage

[Jean-Baptiste Lallement]

Adds support for multilayer images

* Made artefacts download generic: Don't hardcode artefact names and ensure they can be matched via regexp, on launchpad or local directory build.
* Fixed test suite
* Removed obsolete edubuntu server-squashfs test (LP: #1154601)
* Ensure we are still compatible with tests using a local server and with a local build.
* Add new ubuntu:canary subproject in automated builds

Extract from the discussion with cjwatson about these changes:
"""""
didrocks hey cjwatson! jibel and I have a question on ubuntu-cdimage: as with the new layered images we are building, we might not know in advance the squashfs, size, manifest files. We want to avoid to hardcode more filenames that already exists. In livefs.py, there are basically 2 code paths to download assets. One is using LP API (and so, can list all artefacts attached to a specific build) and the other
didrocks one is just for filesystem + http:// download. We thought to match against regexp, which works in the LP case and for filesystem (via os.listdir()). However, tests are failing as they are using http mock server. I wonder if in production, only the LP path is took. If so, can we just detect "http" for tests and either create a manifest that we download or rely on hardcoded filenames?
cjwatson As far as I know it's only ever the LP path in production, indeed
didrocks so, do you think special casing tests, like not using the regexp for them, is fine?
cjwatson Yeah, I think so
didrocks perfect! Thanks a lot cjwatson :)
cjwatson The http:// case may even be legacy - not sure
didrocks yeah, sounds like it, but we preferred asking you directly :)
cjwatson Obviously for good software engineering reasons keeping special cases for tests to a minimum is a good idea, but creating a sort of meta-manifest doesn't seem unreasonable, maybe with a fallback if it doesn't exist
didrocks yeah, the best would be ofc to rewrite the tests to mock LP
didrocks but I think that's out of our current scope :)
"""""

This is the support for multi layer images based on dead code removal branch from from Steve.

[Jean-Baptiste Lallement (jibel)]

Rebased on trunk, could you please review? Thanks

[Steve Langasek (vorlon)]

review: Needs Fixing

[Didier Roche-Tolomelli (didrocks)]

Thanks for the review!

We just rebased on your cleanup branch to avoid adapating tests and code that are going to be removed soon. All your comments should hopefully now be addressed :)

[Didier Roche-Tolomelli (didrocks)]

FYI, some answers to your comments were in https://code.launchpad.net/~jibel/ubuntu-cdimage/support_for_multilayer/+merge/359512 (+ selecting the correct diff version, can't link directly apparently).

[Steve Langasek (vorlon)]

Thanks, getting close now, mostly I just want you to delete some more code ;)

Also since you have (IMHO reasonably) based this on my other branch which has not yet landed, landing this will now necessarily block on review of that other branch, which I've asked sil2100 to look at.

review: Needs Fixing

[lp:~jibel/ubuntu-cdimage/support_for_multilayer]

1804. By Jean-Baptiste Lallement on 2019-03-08

* Removed dead code
* Fixed PEP8 warning
* Updated tests to match the code removal and use the LP code path
* Added negative test to download from remote URL

[Jean-Baptiste Lallement (jibel)]

Thanks for the review.

We tried to explain our choices to show that some decisions were not arbitrary. No issue on pending on the other MP, as stated before, there was no point anyway to modify tests and code that are going to be removed.

So, we mixed a little bit of dead code removal as you requested on this MP, and adapted the tests as the API isn’t called in the way the tests were calling them.

[Steve Langasek (vorlon)]

I still get the following linting failures from ./run-tests on cosmic, do you also see this?

./lib/cdimage/livefs.py:552:21: E201 whitespace after '['
./lib/cdimage/livefs.py:552:69: E202 whitespace before ']'
./lib/cdimage/livefs.py:586:42: E225 missing whitespace around operator
./lib/cdimage/tests/test_livefs.py:1294: local variable 'url' is assigned to but never used

review: Needs Fixing

[Jean-Baptiste Lallement (jibel)]

I didn't notice these linter errors on cosmic because pep8 and pyflakes were not installed in the build env.
test_pep8_clean and test_pyflakes_clean should probably fail (or as least explicitly skip) when the linter is not installed instead of returning.

[lp:~jibel/ubuntu-cdimage/support_for_multilayer]

1805. By Jean-Baptiste Lallement on 2019-03-11

Fixed linter errors

1806. By Jean-Baptiste Lallement on 2019-03-11

Factorized download_live_items_* tests and removed duplicated tests
Added test for filtering several artefacts by suffix
Added test for downloading several artefacts

[Jean-Baptiste Lallement (jibel)]

We removed duplicated test and added one to cover filtering of artefacts and another to ensure that several artefacts are downloaded.

The remaining point is the question about server-squashfs (cf inline comment)

[Steve Langasek (vorlon)]

I covered the point in my prior review that there are no actual consumers of the "server-squashfs" support apart from the tests; so both the tests and the implementation should be culled.

[lp:~jibel/ubuntu-cdimage/support_for_multilayer]

1807. By Jean-Baptiste Lallement on 2019-03-12

Removed all the references to server-squashfs

[Jean-Baptiste Lallement (jibel)]

All the references to server-squashfs have been removed from livefs.py and the related tests.

[Steve Langasek (vorlon)]

review: Approve

=== modified file 'etc/crontab'
--- etc/crontab	2018-10-30 13:23:58 +0000
+++ etc/crontab	2019-03-12 08:44:31 +0000
@@ -21,7 +21,7 @@
 20 15 * * *	DIST=bionic for-project ubuntu-budgie cron.daily-live --live
 26 2 * * *	DIST=bionic for-project ubuntu-mate cron.daily-live --live
 # regular disco daily builds
-31 7 * * *	for-project ubuntu cron.daily-live --live
+31 7 * * *	for-project ubuntu cron.daily-live --live; SUBPROJECT=canary for-project ubuntu cron.daily-live --live
 14 5 * * *	for-project kubuntu cron.daily-live --live
 50 1 * * *	for-project xubuntu cron.daily-live --live
 29 6 * * *	for-project ubuntu-server cron.daily --live; for-project ubuntu-server cron.daily-preinstalled --live; SUBPROJECT=live for-project ubuntu-server cron.daily-live --live

=== modified file 'etc/livefs-launchpad'
--- etc/livefs-launchpad	2019-03-12 08:44:31 +0000
+++ etc/livefs-launchpad	2019-03-12 08:44:31 +0000
@@ -3,6 +3,7 @@
 # Like etc/default-arches, "PROJECT" is in fact PROJECT[-SUBPROJECT][-LOCALE].
 
 ubuntu			daily-live		*		*		ubuntu-cdimage/ubuntu
+ubuntu-canary			daily-live		*		*		ubuntu-cdimage/ubuntu-canary
 kubuntu			daily-live		*		*		ubuntu-cdimage/kubuntu
 edubuntu		dvd			*		*		ubuntu-cdimage/edubuntu
 xubuntu			daily-live		*		*		ubuntu-cdimage/xubuntu

=== modified file 'etc/qa-products'
--- etc/qa-products	2019-03-12 08:44:31 +0000
+++ etc/qa-products	2019-03-12 08:44:31 +0000
@@ -45,6 +45,7 @@
 
 # Ubuntu Desktop 
 Ubuntu Desktop amd64						ubuntu			daily-live			desktop					amd64			iso
+Ubuntu Desktop (Canary) amd64				ubuntu			daily-live/canary	desktop					amd64			iso
 Ubuntu Desktop amd64+mac					ubuntu			daily-live			desktop					amd64+mac		iso
 Ubuntu Desktop armhf+omap4					ubuntu			daily-live			desktop					armhf+omap4		iso
 Ubuntu Desktop i386							ubuntu			daily-live			desktop					i386			iso

=== modified file 'lib/cdimage/livefs.py'
--- lib/cdimage/livefs.py	2019-03-12 08:44:31 +0000
+++ lib/cdimage/livefs.py	2019-03-12 08:44:31 +0000
@@ -538,22 +538,32 @@
         liveproject_subarch = liveproject
 
     lp, lp_livefs = get_lp_livefs(config, arch)
+    uris = []
+    root = ""
     if lp_livefs is not None:
         lp_kwargs = live_build_lp_kwargs(config, lp, lp_livefs, arch)
         lp_build = lp_livefs.getLatestBuild(
             lp_kwargs["distro_arch_series"],
             unique_key=lp_kwargs.get("unique_key"))
-        lp_urls = list(lp_build.getFileUrls())
-
-        def urls_for(base):
-            for url in lp_urls:
-                if unquote(os.path.basename(url)) == base:
-                    yield url
+        uris = list(lp_build.getFileUrls())
     else:
         root = livecd_base(config, arch)
+        try:
+            uris = [os.path.join(root, u) for u in os.listdir(root)]
+        except OSError:
+            # fallback to exact given uri (for http://) in url_for as we can't
+            # list content.
+            pass
 
-        def urls_for(base):
-            yield "%s/%s" % (root, base)
+    def urls_for(base, item):
+        if uris:
+            for uri in uris:
+                filename = unquote(os.path.basename(uri))
+                if (filename.startswith(base + '.') and
+                        filename.endswith('.' + item)):
+                    yield uri
+        else:
+            yield os.path.join(root, base + '.' + item)
 
     if item in (
         "cloop", "squashfs", "manifest", "manifest-desktop", "manifest-remove",
@@ -561,20 +571,22 @@
         "rootfs.tar.gz", "custom.tar.gz", "device.tar.gz",
         "azure.device.tar.gz", "raspi2.device.tar.gz", "plano.device.tar.gz",
         "tar.xz", "iso", "os.snap", "kernel.snap", "disk1.img.xz",
-        "dragonboard.kernel.snap", "raspi2.kernel.snap", "installer.squashfs",
-        "maas-rack.squashfs", "maas-region.squashfs",
+        "dragonboard.kernel.snap", "raspi2.kernel.snap",
         "img.xz", "model-assertion"
     ):
         if item == "ext4" and arch == "armhf+nexus7":
             for url in urls_for(
-                    "livecd.%s.%s-nexus7" % (liveproject_subarch, item)):
+                    "livecd." + liveproject_subarch, item + "-nexus7"):
                 yield url
         elif item == "disk1.img.xz":
             for url in urls_for(
-                    "livecd.%s.%s" % (liveproject, item)):
+                    "livecd." + liveproject, item):
                 yield url
         else:
-            for url in urls_for("livecd.%s.%s" % (liveproject_subarch, item)):
+            for url in urls_for("livecd." + liveproject_subarch, item):
+                # filter out redundant artefacts
+                if url.endswith("modules.squashfs"):
+                    continue
                 yield url
     elif item in (
         "kernel", "initrd", "bootimg", "modules.squashfs"
@@ -582,8 +594,8 @@
         our_flavours = flavours(config, arch)
         our_flavours.extend(["%s-hwe" % (f,) for f in our_flavours])
         for flavour in our_flavours:
-            base = "livecd.%s.%s-%s" % (liveproject_subarch, item, flavour)
-            for url in urls_for(base):
+            for url in urls_for("livecd." + liveproject_subarch,
+                                item + "-" + flavour):
                 yield url
     elif item in (
         "boot-%s+%s.img" % (target.ubuntu_arch, target.subarch)
@@ -596,15 +608,13 @@
             for target in Touch.list_targets_by_ubuntu_arch(arch)
     ):
         for flavour in flavours(config, arch):
-            base = "livecd.%s.%s" % (liveproject_subarch, item)
-            for url in urls_for(base):
+            for url in urls_for("livecd." + liveproject_subarch, item):
                 yield url
     elif item == "kernel-efi-signed":
         if series >= "precise" and arch == "amd64":
             for flavour in flavours(config, arch):
-                base = "livecd.%s.kernel-%s.efi.signed" % (
-                    liveproject_subarch, flavour)
-                for url in urls_for(base):
+                for url in urls_for("livecd." + liveproject_subarch,
+                                    "kernel-" + flavour + ".efi.signed"):
                     yield url
     elif item == "wubi":
         if (project != "xubuntu" and arch in ("amd64", "i386")):
@@ -616,7 +626,7 @@
                    "stable" % series)
     elif item == "ltsp-squashfs":
         if arch in ("amd64", "i386"):
-            for url in urls_for("livecd.%s-ltsp.squashfs" % liveproject):
+            for url in urls_for("livecd." + liveproject + "-ltsp", "squashfs"):
                 yield url
     else:
         raise UnknownLiveItem("Unknown live filesystem item '%s'" % item)
@@ -635,15 +645,7 @@
     output_dir = live_output_directory(config)
     found = False
 
-    if item == "server-squashfs":
-        original_project = config.project
-        try:
-            config["PROJECT"] = "ubuntu-server"
-            urls = list(live_item_paths(config, arch, "squashfs"))
-        finally:
-            config["PROJECT"] = original_project
-    else:
-        urls = list(live_item_paths(config, arch, item))
+    urls = list(live_item_paths(config, arch, item))
     if not urls:
         return False
 
@@ -726,14 +728,17 @@
         except osextras.FetchError:
             pass
     else:
-        target = os.path.join(output_dir, "%s.%s" % (arch, item))
-        try:
-            osextras.fetch(config, urls[0], target)
-            if item in ["squashfs", "server-squashfs"]:
-                sign.sign_cdimage(config, target)
-            found = True
-        except osextras.FetchError:
-            pass
+        for url in urls:
+            # strip livecd.<PROJECT> and replace by arch
+            filename = unquote(os.path.basename(url)).split('.', 2)[-1]
+            target = os.path.join(output_dir, "%s.%s" % (arch, filename))
+            try:
+                osextras.fetch(config, url, target)
+                if target.endswith("squashfs"):
+                    sign.sign_cdimage(config, target)
+                found = True
+            except osextras.FetchError:
+                pass
     return found
 
 
@@ -796,9 +801,6 @@
             elif download_live_items(config, arch, "cloop"):
                 got_image = True
             elif download_live_items(config, arch, "squashfs"):
-                download_live_items(config, arch, "installer.squashfs")
-                download_live_items(config, arch, "maas-rack.squashfs")
-                download_live_items(config, arch, "maas-region.squashfs")
                 download_live_items(config, arch, "modules.squashfs")
                 got_image = True
             elif download_live_items(config, arch, "rootfs.tar.gz"):
@@ -904,10 +906,5 @@
     if project == "edubuntu" and config["CDIMAGE_DVD"]:
         for arch in config.arches:
             if arch in ("amd64", "i386"):
-                # TODO: Disabled for raring (LP: #1154601)
-                # if series >= "raring":
-                #     # Fetch the Ubuntu Server squashfs for Edubuntu Server.
-                #     download_live_items(config, arch, "server-squashfs")
-
                 # Fetch the i386 LTSP chroot for Edubuntu Terminal Server.
                 download_live_items(config, arch, "ltsp-squashfs")

=== modified file 'lib/cdimage/tests/test_livefs.py'
--- lib/cdimage/tests/test_livefs.py	2019-03-12 08:44:31 +0000
+++ lib/cdimage/tests/test_livefs.py	2019-03-12 08:44:31 +0000
@@ -1201,50 +1201,56 @@
         mock_fetch.assert_called_once_with(
             self.config, url, os.path.join(target_dir, "i386.squashfs"))
 
-    @mock.patch("cdimage.osextras.fetch")
-    def test_download_live_items_server_squashfs(self, mock_fetch):
-        self.config["PROJECT"] = "edubuntu"
-        self.config["DIST"] = "trusty"
-        self.config["IMAGE_TYPE"] = "dvd"
-        self.assertTrue(
-            download_live_items(self.config, "i386", "server-squashfs"))
-        url = ("http://cardamom.buildd/~buildd/LiveCD/trusty/ubuntu-server/"
-               "current/livecd.ubuntu-server.squashfs")
-        target_dir = os.path.join(
-            self.temp_dir, "scratch", "edubuntu", "trusty", "dvd", "live")
-        mock_fetch.assert_called_once_with(
-            self.config, url, os.path.join(target_dir, "i386.server-squashfs"))
-
-    @mock.patch("cdimage.osextras.fetch")
-    def assert_server_live_download_items(self, series, item, filenames,
-                                          mock_fetch):
-        self.config["PROJECT"] = "ubuntu-server"
-        self.config["SUBPROJECT"] = "live"
+    def assert_desktop_live_download_items(self, series, item, filenames):
+        self.assert_live_download_items("ubuntu", "", series, item,
+                                        filenames, filenames)
+
+    def assert_desktop_live_download_items_with_expected(self, series, item,
+                                                         filenames,
+                                                         expected_files):
+        self.assert_live_download_items("ubuntu", "", series, item,
+                                        filenames, expected_files)
+
+    def assert_server_live_download_items(self, series, item, filenames):
+        self.assert_live_download_items("ubuntu-server", "live", series, item,
+                                        filenames, filenames)
+
+    @mock.patch("cdimage.osextras.fetch")
+    def assert_live_download_items(self, project, subproject, series, item,
+                                   filenames, expected_files, mock_fetch):
+        artefacts_dir = self.use_temp_dir()
+        self.config["PROJECT"] = project
+        self.config["SUBPROJECT"] = subproject
         self.config["DIST"] = series
         self.config["IMAGE_TYPE"] = "daily-live"
-        self.assertTrue(
-            download_live_items(self.config, "amd64", item))
+        self.config["LIVECD"] = artefacts_dir
         calls = []
+        project_path = project
+        if subproject:
+            project_path = "%s-%s" % (project_path, subproject)
         for filename in filenames:
-            url = ("http://kapok.buildd/~buildd/LiveCD/%s/ubuntu-server-live/"
-                   "current/livecd.ubuntu-server.%s") % (series, filename)
+            uri = os.path.join(
+                artefacts_dir, series,
+                project_path, "current",
+                "livecd.%s.%s" % (project, filename))
+            touch(uri)
+        for filename in expected_files:
             target = os.path.join(
-                self.temp_dir, "scratch", "ubuntu-server", series,
+                self.temp_dir, "scratch", project, series,
                 "daily-live", "live", "amd64." + filename)
-            calls.append(mock.call(self.config, url, target))
+            uri = os.path.join(
+                artefacts_dir, series,
+                project_path, "current",
+                "livecd.%s.%s" % (project, filename))
+            calls.append(mock.call(self.config, uri, target))
+
+        self.assertTrue(
+            download_live_items(self.config, "amd64", item))
         self.assertCountEqual(mock_fetch.call_args_list, calls)
 
     def test_download_live_items_installer_squashfs(self):
         self.assert_server_live_download_items(
-            "bionic", "installer.squashfs", ["installer.squashfs"])
-
-    def test_download_live_items_maas_rack_squashfs(self):
-        self.assert_server_live_download_items(
-            "bionic", "maas-rack.squashfs", ["maas-rack.squashfs"])
-
-    def test_download_live_items_maas_region_squashfs(self):
-        self.assert_server_live_download_items(
-            "bionic", "maas-region.squashfs", ["maas-region.squashfs"])
+            "bionic", "squashfs", ["installer.squashfs"])
 
     def test_download_live_server_boot_items(self):
         self.assert_server_live_download_items(
@@ -1257,6 +1263,50 @@
             "bionic", "modules.squashfs",
             ["modules.squashfs-generic", "modules.squashfs-generic-hwe"])
 
+    @mock.patch("cdimage.osextras.fetch")
+    def test_download_live_items_multi_layers_squashfs(self, mock_fetch):
+        self.assert_desktop_live_download_items(
+            "disco", "squashfs",
+            ["minimal.standard.live.squashfs"])
+
+    @mock.patch("cdimage.osextras.fetch")
+    def test_download_live_items_multiple_squashfses(self, mock_fetch):
+        self.assert_desktop_live_download_items(
+            "disco", "squashfs",
+            ["minimal.squashfs",
+             "minimal.standard.squashfs",
+             "minimal.standard.live.squashfs"])
+
+    @mock.patch("cdimage.osextras.fetch")
+    def test_download_live_items_filter_suffix(self, mock_fetch):
+        self.assert_desktop_live_download_items_with_expected(
+            "disco", "squashfs",
+            ["minimal.squashfs", "minimal.size"], ["minimal.squashfs"])
+
+    @mock.patch("cdimage.osextras.fetch")
+    def test_download_remote_doesnt_honor_suffix(self, mock_fetch):
+        '''Downloading from remote (not LP or local path) will only download
+           basic names'''
+        self.config["PROJECT"] = "ubuntu"
+        self.config["DIST"] = "disco"
+        self.config["IMAGE_TYPE"] = "daily-live"
+        target_dir = os.path.join(
+            self.temp_dir, "scratch", "ubuntu", "disco", "daily-live", "live")
+
+        self.assertTrue(
+            download_live_items(self.config, "amd64", "squashfs"))
+        mock_fetch.assert_called_once_with(
+            self.config,
+            "http://kapok.buildd/~buildd/LiveCD/disco/ubuntu/" +\
+            # This can't be livecd.ubuntu.minimal.standard.live.squashfs for
+            # instance
+            "current/livecd.ubuntu.squashfs",
+            os.path.join(
+                target_dir,
+                # This can't be amd64.minimal.standard.live.squashfs for
+                # instance
+                "amd64.squashfs"))
+
     def test_write_autorun(self):
         self.config["PROJECT"] = "ubuntu"
         self.config["DIST"] = "trusty"