Merge lp:~mwhudson/ubuntu-cdimage/netboot-tarball into lp:ubuntu-cdimage

[Michael Hudson-Doyle]

Publish netboot tarballs from livecd-rootfs

This is the companion part to https://code.launchpad.net/~mwhudson/livecd-rootfs/+git/livecd-rootfs/+merge/423209.

It downloads the netboot tarball that livecd-rootfs change makes and publishes it alongside the ISO, rewriting the config files to refer to where the ISO is being published. It turns out that if you name various files *just so* then a lot of things are taken care of by general machinery, but that general machinery is pretty opaque. Oh well!

I've tried to keep the code fairly generic so if we want, say, the desktop build to start spitting out netboot tarballs no changes should be required here.

The glaring thing this is missing that has to be addressed before merge is release publication, which will have to rewrite the config files again (which probably means we should put the unmodified netboot tarball file in the published directory too but hide it from the index... hmm...).

The other thing that I want to do is generate some nice headers and footers for the netboot/ directory and generate some snippets somewhere that have example dhcpd and dnsmasq config.

I've tested code like this, but I refactored a bit before review and haven't tested exactly this code. So it might be a bit broken.

[lp:~mwhudson/ubuntu-cdimage/netboot-tarball]

2040. By Michael Hudson-Doyle on 2022-05-25

download netboot tarball from livefs

2041. By Michael Hudson-Doyle on 2022-05-25

copy netboot tarball to location suitable for publication to find it

2042. By Michael Hudson-Doyle on 2022-05-25

copy any netboot tarball to target directory when publishing

2043. By Michael Hudson-Doyle on 2022-05-25

rewrite #ISOURL# to the url to the iso when publishing netboot tarball

2044. By Michael Hudson-Doyle on 2022-05-25

unpack netboot tarball during publication as well

[Michael Hudson-Doyle (mwhudson)]

I've tested this now and it seems to work! I uploaded the result of a run to https://people.canonical.com/~mwh/20220525/ and it all looks fairly plausible to me (don't try to use the ISOs in that page, I shrank them to 100M before upload)

[Łukasz Zemczak (sil2100)]

In general this feels good. Of course the re-tarring of the tarball is a bit controversial, but makes sense in this case - especially that per the specification we also want the contents to be untarred in the netboot/ directory anyway.

Left a few inline comments with some things that need changing though. Another thing that needs fixing is: we need unit tests for this new case. The cdimage codebase has usually quite a good test suit, so whenever we add something new, we should make sure it's tested as well. This way we can know if it works-ish.

That being said, I think currently the unit tests are failing! Mostly because of some issues with the simplestreams tests. I have a ticket open for that, so I'll try to fix those in the nearest week. For now, I'd recommend to just running a clean cdimage test run, noting which tests fail and then running with the new codebase + new tests ;p Sorry about that.

review: Needs Fixing

[lp:~mwhudson/ubuntu-cdimage/netboot-tarball]

2045. By Michael Hudson-Doyle on 2022-05-30

add simple test to test_livefs.py

2046. By Michael Hudson-Doyle on 2022-05-30

add tests for and comments to publish_netboot

[Michael Hudson-Doyle (mwhudson)]

> In general this feels good.

Thanks!

> Of course the re-tarring of the tarball is a bit
> controversial, but makes sense in this case - especially that per the
> specification we also want the contents to be untarred in the netboot/
> directory anyway.

Yeah, that's definitely feels a bit unusual. One consequence is that we'll have to do it again when doing the release publication -- not sure the code is set up for that though, does it recompute checksums as part of release publication? (I guess given that in general, not every arch gets published, it must?)

> Left a few inline comments with some things that need changing though. Another
> thing that needs fixing is: we need unit tests for this new case. The cdimage
> codebase has usually quite a good test suit, so whenever we add something new,
> we should make sure it's tested as well. This way we can know if it works-ish.

Good point. I've added test cases to test_livefs.py and test_tree.py. Couldn't figure out how to test the build.py change!

[lp:~mwhudson/ubuntu-cdimage/netboot-tarball]

2047. By Michael Hudson-Doyle on 2022-05-31

release publication for netboot tarballs, maybe?

[Michael Hudson-Doyle (mwhudson)]

I added some stuff around release publication but it doesn't get the release URLs right. I don't think I understand how the directories release publication puts things in map to URLs on releases.ubuntu.com yet...

[lp:~mwhudson/ubuntu-cdimage/netboot-tarball]

2048. By Michael Hudson-Doyle on 2022-06-03

a little refactoring. add a comment where there is something wrong

[Łukasz Zemczak (sil2100)]

This looks okay. There's two inline comments that can be addressed but don't have to be.

As for the publish bits - without any test coverage for those, it's hard to see if the release URLs are right or not. Can you maybe paste what end URLs are you getting in the tarball with the current changes? It would make it much easier for me to figure out how to make the mapping better.

Let's merge this tomorrow after we discuss those ^

review: Approve

[lp:~mwhudson/ubuntu-cdimage/netboot-tarball]

2049. By Michael Hudson-Doyle on 2022-06-21

fix tarball save path

2050. By Michael Hudson-Doyle on 2022-06-21

more fixes

2051. By Michael Hudson-Doyle on 2022-06-21

rewrite SimpleReleaseTree.url_for_path a bit

2052. By Michael Hudson-Doyle on 2022-06-21

logging, dry run

[Michael Hudson-Doyle (mwhudson)]

> This looks okay. There's two inline comments that can be addressed but don't
> have to be.

Thanks. I actually hadn't pushed my most recent commit and it was interesting coming back to this code after a week or two, I think it helped me see the wood for the trees a bit.

> As for the publish bits - without any test coverage for those, it's hard to
> see if the release URLs are right or not. Can you maybe paste what end URLs
> are you getting in the tarball with the current changes? It would make it much
> easier for me to figure out how to make the mapping better.

I think I have this right now, lets see, for daily publication:

SUBPROJECT=live DIST=jammy PROJECT=ubuntu-server-live for-project ubuntu-server cron.daily-live

gives in the log:

Rewriting /home/ubuntu/cdimage/www/full/ubuntu-server/jammy/daily-live/20220621.6/.jammy-netboot-amd64.tar.gz to /home/ubuntu/cdimage/www/full/ubuntu-server/jammy/daily-live/20220621.6/jammy-netboot-amd64.tar.gz with iso_url=https://cdimage.ubuntu.com/ubuntu-server/jammy/daily-live/20220621.6/jammy-live-server-amd64.iso

And publish-release seems to do the right thing now:

DIST=jammy for-project ubuntu publish-release --dry-run ubuntu-server/daily-live 20220621.5 live-server yes
...
Rewriting /home/ubuntu/cdimage/www/full/ubuntu-server/jammy/daily-live/20220621.5/.jammy-netboot-amd64.tar.gz to /home/ubuntu/cdimage/www/simple/jammy/ubuntu-22.04-netboot-amd64.tar.gz with iso_url=https://releases.ubuntu.com/22.04/ubuntu-22.04-live-server-amd64.iso

DIST=jammy for-project ubuntu publish-release --dry-run ubuntu-server/daily-live 20220621.5 live-server named
...
Rewriting /home/ubuntu/cdimage/www/full/ubuntu-server/jammy/daily-live/20220621.5/.jammy-netboot-amd64.tar.gz to /home/ubuntu/cdimage/www/full/releases/jammy/release/ubuntu-22.04-netboot-amd64.tar.gz with iso_url=https://cdimage.ubuntu.com/releases/jammy/release/ubuntu-22.04-live-server-amd64.iso

> Let's merge this tomorrow after we discuss those ^

Looking forward to it :-)

[lp:~mwhudson/ubuntu-cdimage/netboot-tarball]

2053. By Michael Hudson-Doyle on 2022-06-21

sigh

[Łukasz Zemczak (sil2100)]

Okay, I think this looks good now. So merging this branch causes some tests to fail, but I can fix those after merging this. They're trivial to fix it seems. So don't worry about those! But a note for the future: even if tests are not 100% green, be sure to check if your branch doesn't cause new test failures ;p (the test case should be much happier now though)

[Łukasz Zemczak (sil2100)]

review: Approve

[Michael Hudson-Doyle (mwhudson)]

Thanks for the review and merge and sorry about the tests! I should see if I can write some tests for the release publication I guess.

=== modified file 'lib/cdimage/build.py'
--- lib/cdimage/build.py	2022-06-02 05:07:57 +0000
+++ lib/cdimage/build.py	2022-06-21 04:29:40 +0000
@@ -523,6 +523,23 @@
                                 snap, "%s.%s.snap" % (output_prefix, snaptype))
 
 
+def copy_netboot_tarballs(config):
+    # cp $scratch/$arch.netboot.tar.gz $output/$series-netboot-$arch.tar.gz
+    scratch_dir = os.path.join(
+        config.root, "scratch", config.project, config.full_series,
+        config.image_type)
+    for arch in config.arches:
+        netboot_path = os.path.join(
+            live_output_directory(config),
+            '%s.netboot.tar.gz' % (arch,))
+        if os.path.exists(netboot_path):
+            output_dir = os.path.join(scratch_dir, "debian-cd", arch)
+            shutil.copy2(
+                netboot_path, os.path.join(
+                    output_dir,
+                    '%s-netboot-%s.tar.gz' % (config.series, arch)))
+
+
 def _debootstrap_script(config):
     return "usr/share/debootstrap/scripts/%s" % config.series
 
@@ -724,6 +741,7 @@
             configure_splash(config)
 
             run_debian_cd(config)
+            copy_netboot_tarballs(config)
             fix_permissions(config)
 
         # Temporarily turned off for live builds.
 
=== modified file 'lib/cdimage/livefs.py'
--- lib/cdimage/livefs.py	2022-06-10 12:55:35 +0000
+++ lib/cdimage/livefs.py	2022-06-21 04:29:40 +0000
@@ -560,7 +560,7 @@
         "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",
-        "img.xz", "model-assertion", "qcow2", "yaml",
+        "img.xz", "model-assertion", "qcow2", "yaml", "netboot.tar.gz"
     ):
         if item == "ext4" and arch == "armhf+nexus7":
             for url in urls_for(
@@ -793,6 +793,7 @@
             elif download_live_items(config, arch, "squashfs"):
                 download_live_items(config, arch, "modules.squashfs")
                 download_live_items(config, arch, "yaml")
+                download_live_items(config, arch, "netboot.tar.gz")
                 got_image = True
             elif download_live_items(config, arch, "rootfs.tar.gz"):
                 got_image = True
 
=== modified file 'lib/cdimage/tests/test_livefs.py'
--- lib/cdimage/tests/test_livefs.py	2022-05-29 19:28:43 +0000
+++ lib/cdimage/tests/test_livefs.py	2022-06-21 04:29:40 +0000
@@ -1272,6 +1272,8 @@
             "bionic", "squashfs", ["installer.squashfs"])
         self.assert_server_live_download_items(
             "bionic", "yaml", ["installer.yaml"])
+        self.assert_server_live_download_items(
+            "bionic", "netboot.tar.gz", ["netboot.tar.gz"])
 
     def test_download_live_server_boot_items(self):
         self.assert_server_live_download_items(
 
=== modified file 'lib/cdimage/tests/test_tree.py'
--- lib/cdimage/tests/test_tree.py	2022-05-29 19:39:25 +0000
+++ lib/cdimage/tests/test_tree.py	2022-06-21 04:29:40 +0000
@@ -24,10 +24,12 @@
     from html.parser import HTMLParser
 except ImportError:
     from HTMLParser import HTMLParser
+import io
 import os
 import shutil
 import sys
 from textwrap import dedent
+import tarfile
 import traceback
 
 try:
@@ -891,6 +893,46 @@
                 target_dir, "%s-desktop-i386.iso.zsync" % self.config.series),
             "%s-desktop-i386.iso" % self.config.series)
 
+    def test_publish_netboot(self):
+        publisher = self.make_publisher("ubuntu-server", "daily-live")
+        source_dir = publisher.image_output("amd64")
+        tarname = "%s-netboot-amd64.tar.gz" % self.config.series
+        isoname = "%s-live-server-amd64.iso" % self.config.series
+        date = "20201215"
+        os.makedirs(source_dir)
+        with tarfile.open(os.path.join(source_dir, tarname), 'w:gz') as tf:
+            ti = tarfile.TarInfo('config.in')
+            content = b'v=#ISOURL#'
+            ti.size = len(content)
+            tf.addfile(ti, io.BytesIO(content))
+        target_dir = os.path.join(publisher.publish_base, date)
+        target_image = os.path.join(target_dir, isoname)
+        touch(target_image)
+
+        publisher.publish_netboot("amd64", target_image)
+
+        self.assertCountEqual([
+            tarname,
+            isoname,
+            "netboot",
+        ], os.listdir(target_dir))
+        self.assertCountEqual([
+            "config",
+        ], os.listdir(os.path.join(target_dir, "netboot")))
+        with open(os.path.join(target_dir, "netboot/config")) as fp:
+            self.assertEqual(
+                'v=https://%s/ubuntu-server/daily-live/%s/%s' % (
+                    publisher.tree.site_name, date, isoname),
+                fp.read())
+        with tarfile.open(os.path.join(target_dir, tarname), 'r:gz') as tf:
+            for ti in tf:
+                self.assertEqual(ti.name, 'config')
+                self.assertEqual(
+                    ('v=https://%s/ubuntu-server/daily-live/%s/%s' % (
+                        publisher.tree.site_name, date, isoname)
+                    ).encode('utf-8'),
+                    tf.extractfile(ti).read())
+
     @mock.patch("cdimage.osextras.find_on_path", return_value=True)
     @mock.patch("cdimage.tree.DailyTreePublisher.detect_image_extension",
                 return_value="img.xz")
@@ -2084,6 +2126,9 @@
     def test_publish_core_binary(self):
         pass
 
+    def test_publish_netboot(self):
+        pass
+
     def test_publish_appliance_binary(self):
         pass
 
 
=== modified file 'lib/cdimage/tree.py'
--- lib/cdimage/tree.py	2022-06-08 08:29:32 +0000
+++ lib/cdimage/tree.py	2022-06-21 04:29:40 +0000
@@ -20,6 +20,7 @@
 from __future__ import print_function
 
 import errno
+import io
 from itertools import count
 from optparse import OptionParser
 import os
@@ -33,6 +34,7 @@
 import stat
 import subprocess
 import sys
+import tarfile
 from textwrap import dedent
 import time
 import traceback
@@ -93,6 +95,35 @@
         subprocess.check_call(command)
 
 
+def rewrite_and_unpack_tarball(dry_run, source_path, target_path, iso_url):
+    logger.info(
+        "Rewriting %s to %s with iso_url=%s",
+        source_path, target_path, iso_url)
+    if dry_run:
+        return
+    iso_url_b = iso_url.encode('utf-8')
+    netboot_dir = os.path.join(os.path.dirname(target_path), 'netboot')
+    osextras.ensuredir(netboot_dir)
+
+    with tarfile.open(source_path) as inf:
+        with tarfile.open(target_path, 'w:gz') as outf:
+            for ti in inf:
+                if ti.name.endswith('.in'):
+                    new_ti = inf.getmember(ti.name)
+                    new_ti.name = ti.name[:-3]
+                    content = inf.extractfile(ti).read()
+                    content = content.replace(b"#ISOURL#", iso_url_b)
+                    new_ti.size = len(content)
+                    with open(
+                            os.path.join(netboot_dir, new_ti.name),
+                            'wb') as fp:
+                        fp.write(content)
+                    outf.addfile(new_ti, io.BytesIO(content))
+                else:
+                    inf.extract(ti, netboot_dir)
+                    outf.addfile(ti, inf.extractfile(ti))
+
+
 class Tree:
     """A publication tree."""
 
@@ -168,6 +199,13 @@
         """Return the public host name corresponding to this tree."""
         raise NotImplementedError
 
+    def url_for_path(self, path):
+        """Return the public URL for the file at `path`.
+
+        `path` must be under self.directory.
+        """
+        raise NotImplementedError
+
     def path_to_manifest(self, path):
         """Return a manifest file entry for a tree-relative path.
 
@@ -487,6 +525,8 @@
                 return "classroom server %s" % cd
             else:
                 return "server install %s" % cd
+        elif publish_type == "netboot":
+            return "netboot tarball"
         elif publish_type == "legacy-server":
             return "legacy server install %s" % cd
         elif publish_type == "serveraddon":
@@ -579,6 +619,10 @@
             sentences.append(
                 "The install %s allows you to install %s permanently on a "
                 "computer." % (cd, capproject))
+        elif publish_type == "netboot":
+            sentences.append(
+                "The netboot tarball contains files needed to boot the %s "
+                "installer over the network." % (capproject,))
         elif publish_type == "alternate":
             sentences.append(
                 "The alternate install %s allows you to perform certain "
@@ -1121,6 +1165,7 @@
         all_publish_types = (
             "live", "desktop",
             "live-server",
+            "netboot",
             "legacy-server",
             "server", "install", "alternate",
             "serveraddon", "addon",
@@ -1653,7 +1698,8 @@
             print("ReadmeName FOOTER.html", file=htaccess)
             print(
                 "IndexIgnore .htaccess HEADER.html FOOTER.html "
-                "published-ec2-daily.txt published-ec2-release.txt",
+                "published-ec2-daily.txt published-ec2-release.txt "
+                ".*.tar.gz",
                 file=htaccess)
             print(
                 "IndexOptions NameWidth=* DescriptionWidth=* "
@@ -1760,6 +1806,16 @@
     def site_name(self):
         return "cdimage.ubuntu.com"
 
+    def url_for_path(self, path):
+        logger.info(
+            "url_for_path(%s), self.directory = %s", path, self.directory)
+        if not path.startswith(self.directory):
+            raise Exception(
+                "url_for_path(%r) did not start with self.directory (%r)"
+                % (path, self.directory))
+        url_path = path[len(self.directory):].lstrip('/')
+        return "https://%s/%s" % (self.site_name, url_path)
+
     def manifest_files(self):
         """Yield all the files to include in a manifest of this tree."""
         seen_inodes = []
@@ -2002,6 +2058,30 @@
                 for line in jigdo_in:
                     jigdo_out.write(line.replace(from_line, to_line))
 
+    def publish_netboot(self, arch, image_path):
+        # Publishing a netboot tarball is a bit more complicated than
+        # just copying it into place, as we also unpack it into a
+        # netboot/ directory and replace rewrite any foo.cfg.in files
+        # referencing #ISOURL# to foo.cfg referencing the actual URL
+        # of the image.
+        #
+        # We also save a copy of the netboot tarball so we can rewrite
+        # it again during release publication.
+        tarname = "%s-netboot-%s.tar.gz" % (self.config.series, arch)
+        source_path = os.path.join(self.image_output(arch), tarname)
+        if not os.path.exists(source_path):
+            return
+
+        save_target_path = os.path.join(
+            os.path.dirname(image_path), '.' + tarname)
+        target_path = os.path.join(os.path.dirname(image_path), tarname)
+
+        shutil.move(source_path, save_target_path)
+
+        rewrite_and_unpack_tarball(
+            False, save_target_path, target_path,
+            self.tree.url_for_path(image_path))
+
     def publish_binary(self, publish_type, arch, date):
         in_prefix = "%s-%s-%s" % (self.config.series, publish_type, arch)
         if publish_type == "live-core":
@@ -2024,9 +2104,11 @@
         logger.info("Publishing %s ..." % arch)
         osextras.ensuredir(target_dir)
         extension = self.detect_image_extension(source_prefix)
+        target_path = "%s.%s" % (target_prefix, extension)
         shutil.move(
             "%s.%s" % (source_prefix, self.source_extension),
-            "%s.%s" % (target_prefix, extension))
+            target_path)
+        self.publish_netboot(arch, target_path)
         if os.path.exists("%s.list" % source_prefix):
             shutil.move("%s.list" % source_prefix, "%s.list" % target_prefix)
         self.checksum_dirs.append(source_dir)
@@ -2938,6 +3020,12 @@
             directory = os.path.join(config.root, "www", "simple")
         super(SimpleReleaseTree, self).__init__(config, directory)
 
+    def url_for_path(self, path):
+        series = self.config["DIST"]
+        version = getattr(series, "pointversion", series.version)
+        basename = os.path.basename(path)
+        return "https://%s/%s/%s" % (self.site_name, version, basename)
+
     def get_publisher(self, image_type, official, status=None, dry_run=False):
         return SimpleReleasePublisher(
             self, image_type, official, status=status, dry_run=dry_run)
@@ -3200,6 +3288,24 @@
     def want_torrent(self, publish_type):
         raise NotImplementedError
 
+    def publish_release_netboot(self, daily_dir, prefix, arch, image_path):
+        """Publish release images for a single architecture."""
+        logger.info("Copying netboot-%s image ..." % (arch, ))
+
+        source_tarname = ".%s-netboot-%s.tar.gz" % (self.config.series, arch)
+        source_tarpath = os.path.join(daily_dir, source_tarname)
+
+        if not os.path.exists(source_tarpath):
+            return
+
+        target_tarname = "%s-netboot-%s.tar.gz" % (prefix, arch)
+        target_tarpath = os.path.join(
+            os.path.dirname(image_path), target_tarname)
+
+        rewrite_and_unpack_tarball(
+            self.dry_run, source_tarpath, target_tarpath,
+            self.tree.url_for_path(image_path))
+
     def publish_release_arch(self, source, date, publish_type, arch):
         """Publish release images for a single architecture."""
         logger.info("Copying %s-%s image ..." % (publish_type, arch))
@@ -3236,9 +3342,12 @@
                 return os.path.join(
                     torrent_dir, "%s%s%s" % (base_plain, sep, ext))
 
+        main_img = None
+
         for ext in ("iso", "img", "img.gz", "img.xz", "tar.gz", "img.tar.gz",
                     "tar.xz"):
             if os.path.exists(daily(ext)):
+                main_img = daily(ext)
                 break
         else:
             return
@@ -3254,8 +3363,20 @@
                 self.copy(daily(ext), pool(ext))
             if self.want_dist:
                 self.symlink(pool(ext), dist(ext))
+                if daily(ext) == main_img:
+                    self.publish_release_netboot(
+                        os.path.dirname(daily(ext)),
+                        prefix_status,
+                        arch,
+                        dist(ext))
             if self.want_full:
                 self.copy(daily(ext), full(ext))
+                if daily(ext) == main_img:
+                    self.publish_release_netboot(
+                        os.path.dirname(daily(ext)),
+                        prefix,
+                        arch,
+                        full(ext))
 
         for ext in (
             "initrd-ec2", "initrd-virtual", "vmlinuz-ec2", "vmlinuz-virtual",