Remove writable paths #25
Conversation
| Where=/boot/uboot | ||
| What=/run/mnt/ubuntu-boot/uboot/ubuntu | ||
| Type=none | ||
| Options=bind |
There was a problem hiding this comment.
To support booting classic systems (like the cloudimg-rootfs feature) it would be nice if such units would be generated by snap-bootstrap itself, instead of being hardcoded in the core-initrd.
However, migrating such mounts to snap-bootstrap is a time consuming task.
| @@ -0,0 +1,49 @@ | |||
| /run/mnt/base / none bind,x-initrd.mount,ro 0 0 | |||
There was a problem hiding this comment.
Previously the paths that are needed by a given base was encoded inside a gitven base, instead of initrd. Thus allowing changes to base paths as needed inside i.e. core22 to be self-contained inside core22 snap.
With this change it become more coupled, where for example a package change in core22 snap requires simultanious change in core-initrd.
Also consider that a given kernel (core-initrd) might be booting and rolling back across different core22 revisions in recovery mode & run mode.
This also makes it harder to boot classic systems, as most of these mounts must not be done when booting classic systems with for example cloudimg-rootfs feature.
Do you think we could keep this file inside core22 snap at all? I understand that it might be harder to change fstab dynammically, and change what it does.
However, I was under the impression that systemd in the initrd can read $(rootfs)/etc/fstab and do things as defined there, no? Meaning it should be possible to keep this inside core22 snap "by magic".
There was a problem hiding this comment.
I am not sure I understand your comment.
This PR is for core-base. The corresponding PR for core-initrd in canonical/core-initrd#74. I have removed from core-initrd as much as possible to move it to core-base. Now only sysroot.mount and sysroot-writable.mount are left in core-initrd. /usr/lib/modules, /usr/lib/firmware, /boot/*are moved to core-base, where they should be.
So the coupling is actually reduced by this change.
However, I was under the impression that systemd in the initrd can read $(rootfs)/etc/fstab and do things as defined there, no? Meaning it should be possible to keep this inside core22 snap "by magic".
This is exactly what canonical/core-initrd#74 does.
There was a problem hiding this comment.
I was confused indeed. I thought this PR was for core-initrd. This is great!
| @@ -78,9 +78,6 @@ test: | |||
| if !(cd $(TESTDIR) && $$f); then \ | |||
| exit 1; \ | |||
| fi; \ | |||
| done; \ | |||
| set -ex; for f in $$(pwd)/tests/test_*.sh; do \ | |||
| sh -e $$f; \ | |||
There was a problem hiding this comment.
Maybe now it would be good to add some test in hook-tests folder to cover the changes.
| mkdir -p /usr/share/factory/writable/user-data | ||
| cp -aT /home /usr/share/factory/writable/user-data/home | ||
|
|
||
| dirs=( |
There was a problem hiding this comment.
iiuc this is these are the auto/persistent/transition entries from the old writable-paths file, and I see two misses:
/etc/default/crda
/var/lib/apparmor
I see that actually those folders do not exist in UC20, so probably this is fine, but I'd like to understand what happened with them, if somebody knows. Especially with crda which is actually a file in classic.
There was a problem hiding this comment.
And same for /etc/apparmor.d/cache, I guess.
There was a problem hiding this comment.
Right, those were ignored by writable-paths because they do not exist. But now we have errors if it does not exist. Which I think is better because we can track what directories are removed.
There was a problem hiding this comment.
I'm not sure but i think these paths used to be needed.
crda => if one sets a region on their wifi card it would write region setting there, and then on every boot apply it (?! this needs to be verified if that was ever the case, and if that is still in use today).
/var/lib/apparmor => isn't it where cached binary profiles are compiled to, or does snapd move them elsewhere these days? I.e. for some custom apparmor rules that one self-creates in /etc/apparmor.d. Needs verification.
|
|
||
| /run/mnt/data/system-data/etc/systemd /etc/systemd none bind,x-initrd.mount 0 0 | ||
| /run/mnt/data/system-data/etc/writable /etc/writable none bind,x-initrd.mount 0 0 | ||
| /run/mnt/data/system-data/etc/dbus-1 /etc/dbus-1 none bind,x-initrd.mount 0 0 |
There was a problem hiding this comment.
It would be good to have lines sorted in this file, does not seem to be fully the case
hooks/993-factory-files.chroot
Outdated
| @@ -0,0 +1,51 @@ | |||
| #!/bin/bash | |||
| # tmpfiles.d will copy files from /usr/share/factory So file that | |||
| # needs to be populated in /writable have to be copied there. | |||
There was a problem hiding this comment.
I think it would be good to add a more extensive comment mentioning core-writable.conf and a quick mention of what "C" and "d" options do, and s/tmpfiles.d/tmpfiles.d(5)/ so it is clear that the man page can be consulted for details.
| for dir in "${dirs[@]}"; do | ||
| parent="$(dirname "${dir}")" | ||
| mkdir -p "/usr/share/factory/writable/system-data/${parent}" | ||
| cp -aT "${dir}" "/usr/share/factory/writable/system-data/${dir}" |
There was a problem hiding this comment.
How much does the base grow with these copies? Do we really need this? Reading tmpfiles.d(5) it seems that we can specify the source for where to copy files from
There was a problem hiding this comment.
mksquashfs detects duplicated files. The overhead should be marginal. Just directories.
valentindavid
force-pushed
the
valentindavid/remove-writable-paths
branch
2 times, most recently
from
f3a9875 to
216296c
Compare
I have added that. |
valentindavid
force-pushed
the
valentindavid/remove-writable-paths
branch
22 times, most recently
from
63f12b2 to
0762632
Compare
valentindavid
force-pushed
the
valentindavid/remove-writable-paths
branch
3 times, most recently
from
88501b6 to
a0a6987
Compare
Instead we can use /etc/fstab and tmpfiles.d This requires change in core-initrd to pre-run tmpfiles on core-writable.conf before switch of root.
valentindavid
force-pushed
the
valentindavid/remove-writable-paths
branch
from
a0a6987 to
cd765b7
Compare
pedronis
added
the
blocked
Instead we can use /etc/fstab and tmpfiles.d
This requires change in core-initrd to pre-run tmpfiles on
core-writable.conf before switch of root.
See canonical/core-initrd#74