Install in initrd #169

valentindavid · 2023-06-12T14:38:39Z

No description provided.

alfonsosanchezbeato

Just a nitpick - also a question, how much is the initramfs increased with these changes?

Also, maybe expand a little bit the commit message and mention the binaries that are now added to the image, which seem to be:

cryptsetup
sfdisk
partx
tar
mkfs.ext4

alfonsosanchezbeato · 2023-06-13T10:49:04Z

debian/control

@@ -60,7 +60,7 @@ Build-Depends: debhelper-compat (= 12), dh-python, python3:any, dracut-core, qui
               squashfs-tools,
               snapd (>= 2.50+20.04),
               systemd-bootchart,
-               golang-go, indent, libapparmor-dev, libcap-dev, libfuse-dev, libglib2.0-dev, liblzma-dev, liblzo2-dev, libseccomp-dev, libudev-dev, openssh-client, pkg-config, python3, python3-docutils, python3-markdown, squashfs-tools, tzdata, udev, xfslibs-dev
+               golang-go, indent, libapparmor-dev, libcap-dev, libfuse-dev, libglib2.0-dev, liblzma-dev, liblzo2-dev, libseccomp-dev, libudev-dev, openssh-client, pkg-config, python3, python3-docutils, python3-markdown, squashfs-tools, tzdata, udev, xfslibs-dev, cryptsetup-bin


Please move cryptsetup-bin to a different line - the deps on this line come from the time when we had a fork of snap-bootstrap for cloud images, which fortunately is not the case anymore, and afaik were kept intentionally on just one line so they could be removed easily. So actually, hopefully the full line of deps can be dropped now (we can do that in another PR)

alfonsosanchezbeato · 2023-06-13T10:56:11Z

debian/rules

+	/usr/bin/unsquashfs						\
+	/usr/lib/snapd/info						\
+	/usr/lib/snapd/snap-bootstrap					\
+	/usr/sbin/mkfs.ext4


Thanks for reformatting this

kubiko

I would prefer to reverse logic and use
/run/mnt/data/system-data/var/lib/snapd/modeen as primary source of truth and only if not accessible to fallback to /proc/cmdline

this will make it also lot easier to address other modes like factory-reset

kubiko · 2023-06-13T15:14:14Z

factory/usr/lib/the-modeenv

@@ -5,10 +5,19 @@ cp /sysroot/run/image.fstab /run/
 # TODO:UC20: move this logic into snap-bootstrap to be incorporated with 
 # initramfs-mounts

+if grep -q snapd_recovery_mode=run /proc/cmdline; then


should we not use /run/mnt/ubuntu-data/system-data/var/lib/snapd/modeenv as primary source of truth?

That makes sense.

kubiko · 2023-06-13T15:14:19Z

factory/usr/lib/the-modeenv

+if grep -q snapd_recovery_mode=run /proc/cmdline; then
+    run_mode=1
+elif grep -q snapd_recovery_mode=install /proc/cmdline; then
+    if grep -q mode=run /run/mnt/ubuntu-data/system-data/var/lib/snapd/modeenv; then


this condition is too restrictive.
What if snapd_recovery_mode=factory-reset?

kubiko · 2023-06-13T15:15:08Z

factory/usr/lib/the-modeenv

+        run_mode=1
+    fi
+fi
+


how about using this instead:

# Always prefer to use modeevn, if it is available if [ -e "/run/mnt/data/system-data/var/lib/snapd/modeenv" ]; then run_mode="$(sed -n 's/mode=$[^[:space:]]*$/\1/p' /run/mnt/data/system-data/var/lib/snapd/modeenv)" else run_mode="$(sed 's/.*snapd_recovery_mode=$[^[:space:]]*$[[:space:]].*/\1/' /proc/cmdline))" fi

cmdline is more complex. It could be the last argument. In 22 branch we have a shell script that properly parses and we can use that there.

I will propose something different tomorrow.

do you mean /usr/libexec/core/get?
Yeah in 22 I used
run_mode="$(/usr/libexec/core/get-arg snapd_recovery_mode || true)"
If we can backport that script, of course even better, then you can do

if [ -e "/run/mnt/data/system-data/var/lib/snapd/modeenv" ]; then run_mode="$(sed -n 's/mode=$[^[:space:]]*$/\1/p' /run/mnt/data/system-data/var/lib/snapd/modeenv)" else run_mode="$(/usr/libexec/core/get-arg snapd_recovery_mode || true)" fi

I have changed it. Take a look.

kubiko · 2023-06-13T15:15:34Z

factory/usr/lib/the-modeenv

 # Always bind-mount all the host filesystems
 mkdir -p /run/mnt/host
 echo '/run/mnt/host /host none rbind 0 0' >> /run/image.fstab
-if grep -q snapd_recovery_mode=run /proc/cmdline; then
+if [ -n "${run_mode-}" ]; then


and then using
if [ "${run_mode}" = "run" ]; then

kubiko · 2023-06-13T15:31:00Z

@alfonsosanchezbeato with zstd compression this added 980kB on arm64
My test included libdbus... which we are adding regardless so even less than 980kB

mvo5 · 2023-06-14T13:40:03Z

Fwiw, this looks fine to me

valentindavid · 2023-06-14T15:47:43Z

Also, maybe expand a little bit the commit message and mention the binaries that are now added to the image, which seem to be:

I have split it into 2 commits. So now the patch show what is added.

valentindavid · 2023-06-14T15:48:54Z

I had to add kmod-nls-cp437 in the modules to be able to run fsck on vfat. Not sure why it worked before without it.

kubiko

Looks good, just missing /usr/libexec/core/get-mode file to review.

kubiko · 2023-06-14T16:38:57Z

factory/usr/lib/the-modeenv

@@ -5,10 +5,12 @@ cp /sysroot/run/image.fstab /run/
 # TODO:UC20: move this logic into snap-bootstrap to be incorporated with 
 # initramfs-mounts

+mode="$(/usr/libexec/core/get-mode mode)" || mode="$(/usr/libexec/core/get-arg snapd_recovery_mode)" || mode="unknown"


I like this.
I cannot find /usr/libexec/core/get-mode though. Have you forgot to include it in the commit?

kubiko

LGTM
thank you!

alfonsosanchezbeato

Looks very clean, thanks! Approving, but please check question.

alfonsosanchezbeato · 2023-06-15T07:10:19Z

factory/usr/lib/modules-load.d/ubuntu-core-initramfs.conf

@@ -8,6 +8,7 @@ ahci
 libahci
 usb-storage
 nls_iso8859-1
+kmod-nls-cp437


Why do we need this now? Maybe it should have its own commit.

That is fsck.vfat. I am not totally sure what change required that. But it does make sense since we mount the ESP. Not sure why we did not need it before. I will add some comment.

pedronis

thanks

valentindavid mentioned this pull request Jun 13, 2023

snap-boostrap: install in initrd canonical/snapd#12846

Merged

alfonsosanchezbeato reviewed Jun 13, 2023

View reviewed changes

pedronis self-requested a review June 13, 2023 13:54

kubiko reviewed Jun 13, 2023

View reviewed changes

valentindavid force-pushed the valentindavid/install-in-initrd branch 2 times, most recently from 03c4c56 to 047640b Compare June 14, 2023 15:47

valentindavid requested review from kubiko and alfonsosanchezbeato June 14, 2023 15:49

kubiko reviewed Jun 14, 2023

View reviewed changes

valentindavid force-pushed the valentindavid/install-in-initrd branch from 047640b to 1f2f111 Compare June 14, 2023 16:41

kubiko approved these changes Jun 14, 2023

View reviewed changes

alfonsosanchezbeato approved these changes Jun 15, 2023

View reviewed changes

valentindavid added 2 commits June 15, 2023 12:08

Refactor list of files added from host

02c1ff6

Install in initrd

2f3051d

valentindavid force-pushed the valentindavid/install-in-initrd branch from 1f2f111 to 2f3051d Compare June 15, 2023 10:08

pedronis approved these changes Jun 16, 2023

View reviewed changes

valentindavid marked this pull request as ready for review June 19, 2023 08:02

alfonsosanchezbeato merged commit 410b860 into canonical:core20 Jun 19, 2023

This was referenced Jun 19, 2023

[core22] Install in initrd #173

Merged

Install in initrd #180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install in initrd #169

Install in initrd #169

valentindavid commented Jun 12, 2023

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jun 13, 2023

valentindavid Jun 14, 2023

alfonsosanchezbeato Jun 13, 2023

kubiko left a comment

kubiko Jun 13, 2023 •

edited

Loading

valentindavid Jun 13, 2023

kubiko Jun 13, 2023

kubiko Jun 13, 2023

valentindavid Jun 13, 2023

kubiko Jun 14, 2023

valentindavid Jun 14, 2023

kubiko Jun 13, 2023

kubiko commented Jun 13, 2023

mvo5 commented Jun 14, 2023

valentindavid commented Jun 14, 2023

valentindavid commented Jun 14, 2023

kubiko left a comment

kubiko Jun 14, 2023

kubiko left a comment

alfonsosanchezbeato left a comment

alfonsosanchezbeato Jun 15, 2023

valentindavid Jun 15, 2023

pedronis left a comment

Install in initrd #169

Install in initrd #169

Conversation

valentindavid commented Jun 12, 2023

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

kubiko left a comment

Choose a reason for hiding this comment

kubiko Jun 13, 2023 • edited Loading

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

kubiko commented Jun 13, 2023

mvo5 commented Jun 14, 2023

valentindavid commented Jun 14, 2023

valentindavid commented Jun 14, 2023

kubiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubiko left a 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

pedronis left a comment

Choose a reason for hiding this comment

kubiko Jun 13, 2023 •

edited

Loading