many: when building initramfs, pull files from the run system #225

alfonsosanchezbeato · 2023-11-30T17:52:01Z

Previously we were building a rootfs from files at debian package build time, and the ubuntu-core-initramfs was generating the initramfs from this file. This PR changes this script so it pulls files from other packages when invoked, instead. This removes most of the build dependencies and makes maintenance of the package easier, as it does not need to be updated when systemd or other run-time dependencies are updated anymore.

xnox

Did you create initrd before and after the change and do they look the same?

or would you like me to build multiple kernel snaps side by side, for us to compare?

xnox · 2023-12-01T00:42:00Z

bin/ubuntu-core-initramfs

+    install_files(rules, dest_dir)
+    # Other needed stuff
+    out = subprocess.check_output(["dpkg-architecture", "-q",
+                                   "DEB_BUILD_MULTIARCH"]).decode("utf-8")


you definitely want DEB_HOST_MULTIARCH here (as in the native, default, current architecture one is currently running under)

xnox · 2023-12-01T00:47:07Z

bin/ubuntu-core-initramfs

@@ -272,7 +272,173 @@ def add_modules_from_file(dest_d, kernel_root, modules_d, fw_d, conf_file, db,
                )


+def install_files(files, dest_dir):
+    proc_env = os.environ.copy()
+    proc_env["LP_PRELOAD"] = ""


do you mean LD_PRELOAD here instead?

Also this is called so many times, it seems like it will be worthwhile to add a small wrapper def check_call(*args, **kwargs) that wraps check_call without LD_PRELOAD.

Oops, changed, good catch.

Not sure though about the wrapper to subprocess.check_call as it would not save as much, instead I have now done from subprocess import run, check_call, check_output so the package prefix is not needed, which is afaiu what is slightly annoying.

xnox · 2023-12-01T00:48:17Z

bin/ubuntu-core-initramfs

+
+# This is useful if the destination path inside dest_dir is different
+# from the current file path.
+def install_file_to_path(file, dest_dir, dest_path):


potentially can be merged with above function and make dest_path an optional kwarg argument.

I think that would complicate too much the function to make it worth it tbh, because of the different arguments, the need to specify dest_path for all files if we have a list, etc.

xnox · 2023-12-01T00:49:41Z

bin/ubuntu-core-initramfs

+        [
+            r"dpkg -L systemd | "
+            r"grep -E '(^/lib/systemd/system|/modprobe\.d/|/sysctl\.d/|"
+            r"/rules\.d/|/tmpfiles\.d/|/bin/)'",


whilst grep works, using more pythonic functions for filtering might have been better. I would rather just slurp all the lines from dpkg, and then use python for filtering by using glob or re. Ideally one pattern at a time, explaining why we need it.

also multiple packages can be specified to dpkg call, which might work better for the set of related systemd .deb packages (i.e. systemd udev libudev1 systemd-sysv).

Changed now

xnox · 2023-12-01T00:51:42Z

bin/ubuntu-core-initramfs

+def install_busybox(dest_dir):
+    # Use busybox shell instead of dash
+    os.remove(os.path.join(dest_dir, "usr/bin/sh"))
+    os.remove(os.path.join(dest_dir, "usr/bin/dash"))


i would want to install these first, such that dractu-install later just uses existing ones, rather than try to override them. Because getting this at the start matters.

Good point, I've pulled now busybox first so there is no need to remove sh/dash.

xnox · 2023-12-01T00:53:53Z

bin/ubuntu-core-initramfs

+        "/sbin/fsck.vfat",
+        "/sbin/mkfs.ext4",
+        "/sbin/mkfs.vfat",
+        "/sbin/sfdisk",


we require merged-usr for many releases now, it would help static lists like these to use "/usr" prefixed paths already.

contents of .deb packages is moving to have everything built with "/usr" paths too in noble, but it is a slow process as packages upgrade.

xnox · 2023-12-01T00:57:27Z

bin/ubuntu-core-initramfs

+        # Copy systemd bits
+        install_systemd_files(main)
+        # copy busybox
+        install_busybox(main)


I think installing busybox before systemd is safer.

bin/ubuntu-core-initramfs

alfonsosanchezbeato

@xnox thanks for the quick review, I've addressed your comments now.

FTR, yes, to check correctness I have compared the final list of files with what the old approach produced. The are just some small differences that should be fine:

Some links/binaries that used to be in /usr/bin are now in /usr/sbin: reboot, rmmod, e2fsck and others, which seems more correct to me as that is their path in the rootfs. Not sure how they were originally placed in /usr/bin.
Due to an error in an invocation of dracut-install, we were pulling more libraries than needed in the initramfs (concretely, when calling it on /usr/lib/$(DEB_HOST_MULTIARCH)/plymouth/*.so, where we should have run it only for the plugins we are pulling - two-step, script, label-ft). I've fixed this so these unneeded libraries are not included anymore.

Fwiw, I've tested this locally by replacing initramfs in a 24 snap kernel and things were good, also CI seems to be happy.

alfonsosanchezbeato · 2023-12-01T09:03:25Z

bin/ubuntu-core-initramfs

@@ -272,7 +272,173 @@ def add_modules_from_file(dest_d, kernel_root, modules_d, fw_d, conf_file, db,
                )


+def install_files(files, dest_dir):
+    proc_env = os.environ.copy()
+    proc_env["LP_PRELOAD"] = ""


Oops, changed, good catch.

Not sure though about the wrapper to subprocess.check_call as it would not save as much, instead I have now done from subprocess import run, check_call, check_output so the package prefix is not needed, which is afaiu what is slightly annoying.

alfonsosanchezbeato · 2023-12-01T09:06:19Z

bin/ubuntu-core-initramfs

+
+# This is useful if the destination path inside dest_dir is different
+# from the current file path.
+def install_file_to_path(file, dest_dir, dest_path):


I think that would complicate too much the function to make it worth it tbh, because of the different arguments, the need to specify dest_path for all files if we have a list, etc.

alfonsosanchezbeato · 2023-12-01T11:47:18Z

bin/ubuntu-core-initramfs

+        [
+            r"dpkg -L systemd | "
+            r"grep -E '(^/lib/systemd/system|/modprobe\.d/|/sysctl\.d/|"
+            r"/rules\.d/|/tmpfiles\.d/|/bin/)'",


Changed now

alfonsosanchezbeato · 2023-12-01T13:18:32Z

bin/ubuntu-core-initramfs

+def install_busybox(dest_dir):
+    # Use busybox shell instead of dash
+    os.remove(os.path.join(dest_dir, "usr/bin/sh"))
+    os.remove(os.path.join(dest_dir, "usr/bin/dash"))


Good point, I've pulled now busybox first so there is no need to remove sh/dash.

alfonsosanchezbeato · 2023-12-01T13:51:03Z

bin/ubuntu-core-initramfs

+    install_files(rules, dest_dir)
+    # Other needed stuff
+    out = subprocess.check_output(["dpkg-architecture", "-q",
+                                   "DEB_BUILD_MULTIARCH"]).decode("utf-8")


alfonsosanchezbeato · 2023-12-01T13:59:10Z

bin/ubuntu-core-initramfs

+        "/sbin/fsck.vfat",
+        "/sbin/mkfs.ext4",
+        "/sbin/mkfs.vfat",
+        "/sbin/sfdisk",


alfonsosanchezbeato · 2023-12-01T13:59:25Z

bin/ubuntu-core-initramfs

+        # Copy systemd bits
+        install_systemd_files(main)
+        # copy busybox
+        install_busybox(main)


bin/ubuntu-core-initramfs

alfonsosanchezbeato · 2023-12-01T15:28:11Z

Hm, ubuntu-image in edge is failing now...

alfonsosanchezbeato · 2023-12-01T18:50:45Z

There is an issue with the just merged assertions, see snapcore/models#17 (comment)

valentindavid

Looks good. Some minor comments. The $$ is the only one that requires a fix before merge.

I would have kept /usr/sbin as symlink change it in a follow up PR so we do not have to revert the whole thing if we find an issue. But maybe it is safe enough.

valentindavid · 2023-12-08T08:50:14Z

bin/ubuntu-core-initramfs

+    # remove redundant definitions after that.
+    check_call(["systemd-hwdb", "--root", dest_dir,
+                "update", "--usr", "--strict"])
+    check_call(["rm -rf " + os.path.join(dest_dir,


shutil.rmtree?

valentindavid · 2023-12-08T09:00:13Z

bin/ubuntu-core-initramfs

+def install_systemd_files(dest_dir):
+    # Build list of files and directories
+
+    out = check_output(["dpkg", "-L", "systemd", "systemd-sysv"])


It seems we have 3 places where we do dpkg -L then filtering. I wonder if we could have a function for that.

Function added

valentindavid · 2023-12-08T09:20:34Z

factory/lib

@@ -1 +0,0 @@
-usr/lib


What is it replaced with? I did not find it.

This symlink is created now by dracut-install when the u-c-i script runs, so it is not necessary to include it inside the deb (same for the other symlinks I removed).

valentindavid · 2023-12-08T09:39:21Z

bin/ubuntu-core-initramfs

+    # This hack should be removed with PR#113
+    check_call([r"sed -i '/^After=/"
+                r"{;s, *plymouth-start[.]service *, ,;/"
+                r"^After= *$$/d;}' "


$$? This was probably copied from the makefile.

Good catch, extra $ removed now

valentindavid · 2023-12-08T09:47:09Z

bin/ubuntu-core-initramfs

+    out = check_output([bb_cmd, "--list-long"])
+    cmds = out.decode("utf-8").splitlines()
+    # Remove command we do not want
+    to_remove = ["busybox", "reboot", "mount", "umount"]


It seems you fixed "modinfo" probably because of ordering. Now it uses kmod, But maybe we should make explicit and remove it from here.

Excellent point, I've removed it explicitly (it was still in usr/bin, although the one in usr/sbin already pointed to kmod)

debian/rules

alfonsosanchezbeato

@valentindavid thanks for the review, comments addressed now

alfonsosanchezbeato · 2023-12-08T15:49:36Z

factory/lib

@@ -1 +0,0 @@
-usr/lib


This symlink is created now by dracut-install when the u-c-i script runs, so it is not necessary to include it inside the deb (same for the other symlinks I removed).

alfonsosanchezbeato · 2023-12-08T15:58:34Z

bin/ubuntu-core-initramfs

+def install_systemd_files(dest_dir):
+    # Build list of files and directories
+
+    out = check_output(["dpkg", "-L", "systemd", "systemd-sysv"])


Function added

alfonsosanchezbeato · 2023-12-08T16:05:36Z

bin/ubuntu-core-initramfs

+    # This hack should be removed with PR#113
+    check_call([r"sed -i '/^After=/"
+                r"{;s, *plymouth-start[.]service *, ,;/"
+                r"^After= *$$/d;}' "


Good catch, extra $ removed now

alfonsosanchezbeato · 2023-12-08T16:07:11Z

bin/ubuntu-core-initramfs

+    # remove redundant definitions after that.
+    check_call(["systemd-hwdb", "--root", dest_dir,
+                "update", "--usr", "--strict"])
+    check_call(["rm -rf " + os.path.join(dest_dir,


alfonsosanchezbeato · 2023-12-08T16:12:50Z

bin/ubuntu-core-initramfs

+    out = check_output([bb_cmd, "--list-long"])
+    cmds = out.decode("utf-8").splitlines()
+    # Remove command we do not want
+    to_remove = ["busybox", "reboot", "mount", "umount"]


Excellent point, I've removed it explicitly (it was still in usr/bin, although the one in usr/sbin already pointed to kmod)

debian/rules

alfonsosanchezbeato · 2024-01-02T09:01:00Z

@valentindavid @xnox this is ready for another review round.

valentindavid

Looks good. Thank you!

and use it to install some of files in the initramfs.

Install systemd files from the system where the script is run instead of using the ones included in the ubuntu-core-initramfs package.

These folders get created anyway by create-initrd.

As now we pull most of the files at run time.

For clearer calls.

filtering in the script instead of by using grep.

dracut-install installs the interpreters detected in shebangs (see resolve_deps() in dracut-install.c) - but we want busybox, not the system default.

as we are interested in the run system now.

we want to avoid symbolic links.

xnox

Here be dragons!

alfonsosanchezbeato requested review from xnox and valentindavid November 30, 2023 17:52

xnox suggested changes Dec 1, 2023

View reviewed changes

alfonsosanchezbeato commented Dec 1, 2023

View reviewed changes

alfonsosanchezbeato requested a review from xnox December 1, 2023 14:53

valentindavid reviewed Dec 8, 2023

View reviewed changes

alfonsosanchezbeato force-pushed the systemd-run-time branch from a962469 to 660aaf9 Compare December 8, 2023 16:29

alfonsosanchezbeato commented Dec 8, 2023

View reviewed changes

alfonsosanchezbeato requested a review from valentindavid December 8, 2023 17:26

alfonsosanchezbeato mentioned this pull request Dec 10, 2023

bin/ubuntu-core-initramfs: make sure the initramfs can be cross-built #226

Merged

valentindavid approved these changes Jan 3, 2024

View reviewed changes

alfonsosanchezbeato added 13 commits January 10, 2024 15:06

bin/ubuntu-core-initramfs: add ancillary install files function

33fb44c

and use it to install some of files in the initramfs.

bin/ubuntu-core-initramfs: install systemd files

ecb0b6e

Install systemd files from the system where the script is run instead of using the ones included in the ubuntu-core-initramfs package.

bin/ubuntu-core-initramfs: install all from run system

cb0a3ac

factory: remove folders that we do not really need to ship

e862f66

These folders get created anyway by create-initrd.

debian: move build deps to normal dependencies

483a0fa

As now we pull most of the files at run time.

bin/ubuntu-core-initramfs: fix name of env var

614308b

bin/ubuntu-core-initramfs: import used functions from subprocess

3bab853

For clearer calls.

bin/ubuntu-core-initramfs: obtain files to install by

91f59aa

filtering in the script instead of by using grep.

bin/ubuntu-core-initramfs: pull busybox first

07b3746

dracut-install installs the interpreters detected in shebangs (see resolve_deps() in dracut-install.c) - but we want busybox, not the system default.

bin/ubuntu-core-initramfs: use DEB_HOST_MULTIARCH instead of *_BUILD_*

c6fb236

as we are interested in the run system now.

bin/ubuntu-core-initramfs: add /usr prefix to files to include

45c73bd

we want to avoid symbolic links.

bin/ubuntu-core-initramfs: add TODO about fips feature

86c7aa4

bin/ubuntu-core-initramfs: address review comments

913abb8

alfonsosanchezbeato force-pushed the systemd-run-time branch from 660aaf9 to 913abb8 Compare January 10, 2024 15:12

xnox approved these changes Jan 23, 2024

View reviewed changes

xnox merged commit dc9e042 into canonical:main Jan 23, 2024
3 checks passed

alfonsosanchezbeato deleted the systemd-run-time branch May 16, 2024 09:14

		@@ -1 +0,0 @@
		usr/lib

		@@ -1 +0,0 @@
		usr/lib

many: when building initramfs, pull files from the run system #225

many: when building initramfs, pull files from the run system #225

Conversation

alfonsosanchezbeato commented Nov 30, 2023

xnox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Dec 1, 2023

alfonsosanchezbeato commented Dec 1, 2023

valentindavid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Jan 2, 2024

valentindavid left a comment

Choose a reason for hiding this comment

xnox left a comment

Choose a reason for hiding this comment