diff mbox series

[v2] wic: do not ignore ROOTFS_SIZE if the rootfs is modified

Message ID 20250410113626.9057-1-twoerner@gmail.com
State Accepted, archived
Commit 1c690aa046ebca13d7b29de50d42b5d8a4a8486c
Headers show
Series [v2] wic: do not ignore ROOTFS_SIZE if the rootfs is modified | expand

Commit Message

Trevor Woerner April 10, 2025, 11:36 a.m. UTC 
If the *.wks file contains a "--source rootfs" then
lib/wic/plugins/source/rootfs.py will be invoked to generate (what is assumed
to be) the rootfs partition. If the rootfs partition needs to be tweaked or
modified, the "rootfs.py" plugin will make a copy of the filesystem and then
perform the changes on that copy. In other words, if the "--source rootfs"
line of the *.wks file also contains any of:

	--exclude-path
	--include-path
	--change-directory
	--use-label (i.e. modify etc/fstab)

then the rootfs will be copied first, then the copy is modified.

If, for example, the unmodified IMAGE_ROOTFS is:

	.../tmp/work/qemuarm64_secureboot-oe-linux/core-image-base/1.0/rootfs

then the copy would be made at:

	.../tmp/work/qemuarm64_secureboot-oe-linux/core-image-base/1.0/tmp-wic/rootfs${LINENO}

where ${LINENO} is the line number where this "--source rootfs" line appears
in the *wks file.

When it comes time to make an actual partition of a specific filesystem type,
lib/wic/partition.py::prepare_rootfs() is called. It is in this function that
wic figures out if any extra size needs to be added. The bitbake variable used
to specify the ultimate rootfs size is ROOTFS_SIZE, and since this variable is
only valid for the rootfs (and not any other partitions), the code also
verifies that the partition being created is ${IMAGE_ROOTFS}:

	rsize_bb = get_bitbake_var('ROOTFS_SIZE')
	rdir = get_bitbake_var('IMAGE_ROOTFS')
	if rsize_bb and rdir == rootfs_dir:
		<use rsize_bb>
	else:
		<calculate the partition size using "du -ks $p">

As noted above, if lib/wic/plugins/source/rootfs.py has made a copy, then the
"rdir == rootfs_dir" clause will fail and the code will assume this partition
is not a rootfs since the strings do not compare equal.

Therefore, in order to determine if this is a rootfs, retain the existing
"rdir == rootfs_dir" comparison, but also add another one to check whether or
not this is a wic-generated copy of the rootfs.

STEPS TO REPRODUCE:
	- start with the following *wks file:
		bootloader --ptable gpt
		part /boot --size=100M --active --fstype=ext4 --label boot
		part /     --source rootfs      --fstype=ext4 --label root
	- and the following extra variable in conf/local.conf:
		IMAGE_ROOTFS_EXTRA_SPACE = "500000"
	- build an image
	- run it in qemu
		$ runqemu slirp nographic serial
	- verify the root partition has extra space:
		root@qemuarm64-secureboot:~# df -h
		Filesystem                Size      Used Available Use% Mounted on
		/dev/root               721.5M     67.4M    600.6M  10% /
		devtmpfs                477.7M         0    477.7M   0% /dev
		tmpfs                    40.0K         0     40.0K   0% /mnt
		tmpfs                   489.3M     92.0K    489.2M   0% /run
		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
	- modify the "/" line of the *wks file to be:
		part /     --source rootfs      --fstype=ext4 --label root --exclude-path boot/
	- build image

	when it fails:
		root@qemuarm64-secureboot:~# df -h
		Filesystem                Size      Used Available Use% Mounted on
		/dev/root                73.4M     41.9M     25.8M  62% /
		devtmpfs                477.7M         0    477.7M   0% /dev
		tmpfs                    40.0K         0     40.0K   0% /mnt
		tmpfs                   489.3M     92.0K    489.2M   0% /run
		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
		/dev/vda1               120.4M     19.9M     91.4M  18% /boot

	after this fix:
		root@qemuarm64-secureboot:~# df -h
		Filesystem                Size      Used Available Use% Mounted on
		/dev/root               721.5M     47.4M    620.6M   7% /
		devtmpfs                477.7M         0    477.7M   0% /dev
		tmpfs                    40.0K         0     40.0K   0% /mnt
		tmpfs                   489.3M     92.0K    489.2M   0% /run
		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
		/dev/vda1               120.4M     19.9M     91.4M  18% /boot

Doing the math we see that the /boot partition is ~20MB and in the first image
the / partition contains this ~20MB in addition to the rest of the rootfs.
This ~20MB is completely wasted since it is used in the / partition, but then
the /boot partition is mounted on top of it, making the /boot directory of /
inaccessible. After the fix the / partition has an additional ~20MB since the
/boot portion is excluded.

Fixes [YOCTO #15555]

Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
changes in v2:
- fix bug to not process the root partition removing the requirement to
  define the IMAGE_BOOT_FILES variable
- reword commit message so the wks reproducer lines better match what is in
  the test
---
 meta/lib/oeqa/selftest/cases/wic.py | 32 +++++++++++++++++++++++++++++
 scripts/lib/wic/partition.py        |  2 +-
 2 files changed, 33 insertions(+), 1 deletion(-)

Comments

Mathieu Dubois-Briand April 14, 2025, 11:40 a.m. UTC | #1 
On Thu Apr 10, 2025 at 1:36 PM CEST, Trevor Woerner via lists.openembedded.org wrote:
> If the *.wks file contains a "--source rootfs" then
> lib/wic/plugins/source/rootfs.py will be invoked to generate (what is assumed
> to be) the rootfs partition. If the rootfs partition needs to be tweaked or
> modified, the "rootfs.py" plugin will make a copy of the filesystem and then
> perform the changes on that copy. In other words, if the "--source rootfs"
> line of the *.wks file also contains any of:
>
> 	--exclude-path
> 	--include-path
> 	--change-directory
> 	--use-label (i.e. modify etc/fstab)
>
> then the rootfs will be copied first, then the copy is modified.
>
> If, for example, the unmodified IMAGE_ROOTFS is:
>
> 	.../tmp/work/qemuarm64_secureboot-oe-linux/core-image-base/1.0/rootfs
>
> then the copy would be made at:
>
> 	.../tmp/work/qemuarm64_secureboot-oe-linux/core-image-base/1.0/tmp-wic/rootfs${LINENO}
>
> where ${LINENO} is the line number where this "--source rootfs" line appears
> in the *wks file.
>
> When it comes time to make an actual partition of a specific filesystem type,
> lib/wic/partition.py::prepare_rootfs() is called. It is in this function that
> wic figures out if any extra size needs to be added. The bitbake variable used
> to specify the ultimate rootfs size is ROOTFS_SIZE, and since this variable is
> only valid for the rootfs (and not any other partitions), the code also
> verifies that the partition being created is ${IMAGE_ROOTFS}:
>
> 	rsize_bb = get_bitbake_var('ROOTFS_SIZE')
> 	rdir = get_bitbake_var('IMAGE_ROOTFS')
> 	if rsize_bb and rdir == rootfs_dir:
> 		<use rsize_bb>
> 	else:
> 		<calculate the partition size using "du -ks $p">
>
> As noted above, if lib/wic/plugins/source/rootfs.py has made a copy, then the
> "rdir == rootfs_dir" clause will fail and the code will assume this partition
> is not a rootfs since the strings do not compare equal.
>
> Therefore, in order to determine if this is a rootfs, retain the existing
> "rdir == rootfs_dir" comparison, but also add another one to check whether or
> not this is a wic-generated copy of the rootfs.
>
> STEPS TO REPRODUCE:
> 	- start with the following *wks file:
> 		bootloader --ptable gpt
> 		part /boot --size=100M --active --fstype=ext4 --label boot
> 		part /     --source rootfs      --fstype=ext4 --label root
> 	- and the following extra variable in conf/local.conf:
> 		IMAGE_ROOTFS_EXTRA_SPACE = "500000"
> 	- build an image
> 	- run it in qemu
> 		$ runqemu slirp nographic serial
> 	- verify the root partition has extra space:
> 		root@qemuarm64-secureboot:~# df -h
> 		Filesystem                Size      Used Available Use% Mounted on
> 		/dev/root               721.5M     67.4M    600.6M  10% /
> 		devtmpfs                477.7M         0    477.7M   0% /dev
> 		tmpfs                    40.0K         0     40.0K   0% /mnt
> 		tmpfs                   489.3M     92.0K    489.2M   0% /run
> 		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
> 		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
> 	- modify the "/" line of the *wks file to be:
> 		part /     --source rootfs      --fstype=ext4 --label root --exclude-path boot/
> 	- build image
>
> 	when it fails:
> 		root@qemuarm64-secureboot:~# df -h
> 		Filesystem                Size      Used Available Use% Mounted on
> 		/dev/root                73.4M     41.9M     25.8M  62% /
> 		devtmpfs                477.7M         0    477.7M   0% /dev
> 		tmpfs                    40.0K         0     40.0K   0% /mnt
> 		tmpfs                   489.3M     92.0K    489.2M   0% /run
> 		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
> 		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
>
> 	after this fix:
> 		root@qemuarm64-secureboot:~# df -h
> 		Filesystem                Size      Used Available Use% Mounted on
> 		/dev/root               721.5M     47.4M    620.6M   7% /
> 		devtmpfs                477.7M         0    477.7M   0% /dev
> 		tmpfs                    40.0K         0     40.0K   0% /mnt
> 		tmpfs                   489.3M     92.0K    489.2M   0% /run
> 		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
> 		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
>
> Doing the math we see that the /boot partition is ~20MB and in the first image
> the / partition contains this ~20MB in addition to the rest of the rootfs.
> This ~20MB is completely wasted since it is used in the / partition, but then
> the /boot partition is mounted on top of it, making the /boot directory of /
> inaccessible. After the fix the / partition has an additional ~20MB since the
> /boot portion is excluded.
>
> Fixes [YOCTO #15555]
>
> Signed-off-by: Trevor Woerner <twoerner@gmail.com>
> ---

Hi Trevor,

Thanks for the v2, but I believe we still have an issue. Maybe a missing dependency?

2025-04-13 19:18:05,421 - oe-selftest - INFO - FAIL: wic.Wic.test_exclude_path_with_extra_space (subunit.RemotedTestCase)
2025-04-13 19:18:05,421 - oe-selftest - INFO - ----------------------------------------------------------------------
2025-04-13 19:18:05,421 - oe-selftest - INFO - testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/lib/oeqa/selftest/cases/wic.py", line 567, in test_exclude_path_with_extra_space
    size_of_root_partition = int(runCmd("wic ls %s" % wicout).output.split('\n')[2].split()[3])
                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/lib/oeqa/utils/commands.py", line 214, in runCmd
    raise AssertionError("Command '%s' returned non-zero exit status %d:\n%s" % (command, result.status, exc_output))
AssertionError: Command 'wic ls /srv/pokybuild/yocto-worker/oe-selftest-debian/build/build-st-2511849/tmp/deploy/images/qemux86-64/core-image-minimal-qemux86-64.rootfs-20250413181535.wic' returned non-zero exit status 1:
ERROR: Can't find executable parted

https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1350
Trevor Woerner April 14, 2025, 9:06 p.m. UTC | #2 
On Mon 2025-04-14 @ 01:40:34 PM, Mathieu Dubois-Briand wrote:
> On Thu Apr 10, 2025 at 1:36 PM CEST, Trevor Woerner via lists.openembedded.org wrote:
> > If the *.wks file contains a "--source rootfs" then
> > lib/wic/plugins/source/rootfs.py will be invoked to generate (what is assumed
> > to be) the rootfs partition. If the rootfs partition needs to be tweaked or
> > modified, the "rootfs.py" plugin will make a copy of the filesystem and then
> > perform the changes on that copy. In other words, if the "--source rootfs"
> > line of the *.wks file also contains any of:
> >
> > 	--exclude-path
> > 	--include-path
> > 	--change-directory
> > 	--use-label (i.e. modify etc/fstab)
> >
> > then the rootfs will be copied first, then the copy is modified.
> >
> > If, for example, the unmodified IMAGE_ROOTFS is:
> >
> > 	.../tmp/work/qemuarm64_secureboot-oe-linux/core-image-base/1.0/rootfs
> >
> > then the copy would be made at:
> >
> > 	.../tmp/work/qemuarm64_secureboot-oe-linux/core-image-base/1.0/tmp-wic/rootfs${LINENO}
> >
> > where ${LINENO} is the line number where this "--source rootfs" line appears
> > in the *wks file.
> >
> > When it comes time to make an actual partition of a specific filesystem type,
> > lib/wic/partition.py::prepare_rootfs() is called. It is in this function that
> > wic figures out if any extra size needs to be added. The bitbake variable used
> > to specify the ultimate rootfs size is ROOTFS_SIZE, and since this variable is
> > only valid for the rootfs (and not any other partitions), the code also
> > verifies that the partition being created is ${IMAGE_ROOTFS}:
> >
> > 	rsize_bb = get_bitbake_var('ROOTFS_SIZE')
> > 	rdir = get_bitbake_var('IMAGE_ROOTFS')
> > 	if rsize_bb and rdir == rootfs_dir:
> > 		<use rsize_bb>
> > 	else:
> > 		<calculate the partition size using "du -ks $p">
> >
> > As noted above, if lib/wic/plugins/source/rootfs.py has made a copy, then the
> > "rdir == rootfs_dir" clause will fail and the code will assume this partition
> > is not a rootfs since the strings do not compare equal.
> >
> > Therefore, in order to determine if this is a rootfs, retain the existing
> > "rdir == rootfs_dir" comparison, but also add another one to check whether or
> > not this is a wic-generated copy of the rootfs.
> >
> > STEPS TO REPRODUCE:
> > 	- start with the following *wks file:
> > 		bootloader --ptable gpt
> > 		part /boot --size=100M --active --fstype=ext4 --label boot
> > 		part /     --source rootfs      --fstype=ext4 --label root
> > 	- and the following extra variable in conf/local.conf:
> > 		IMAGE_ROOTFS_EXTRA_SPACE = "500000"
> > 	- build an image
> > 	- run it in qemu
> > 		$ runqemu slirp nographic serial
> > 	- verify the root partition has extra space:
> > 		root@qemuarm64-secureboot:~# df -h
> > 		Filesystem                Size      Used Available Use% Mounted on
> > 		/dev/root               721.5M     67.4M    600.6M  10% /
> > 		devtmpfs                477.7M         0    477.7M   0% /dev
> > 		tmpfs                    40.0K         0     40.0K   0% /mnt
> > 		tmpfs                   489.3M     92.0K    489.2M   0% /run
> > 		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
> > 		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
> > 	- modify the "/" line of the *wks file to be:
> > 		part /     --source rootfs      --fstype=ext4 --label root --exclude-path boot/
> > 	- build image
> >
> > 	when it fails:
> > 		root@qemuarm64-secureboot:~# df -h
> > 		Filesystem                Size      Used Available Use% Mounted on
> > 		/dev/root                73.4M     41.9M     25.8M  62% /
> > 		devtmpfs                477.7M         0    477.7M   0% /dev
> > 		tmpfs                    40.0K         0     40.0K   0% /mnt
> > 		tmpfs                   489.3M     92.0K    489.2M   0% /run
> > 		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
> > 		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
> >
> > 	after this fix:
> > 		root@qemuarm64-secureboot:~# df -h
> > 		Filesystem                Size      Used Available Use% Mounted on
> > 		/dev/root               721.5M     47.4M    620.6M   7% /
> > 		devtmpfs                477.7M         0    477.7M   0% /dev
> > 		tmpfs                    40.0K         0     40.0K   0% /mnt
> > 		tmpfs                   489.3M     92.0K    489.2M   0% /run
> > 		tmpfs                   489.3M     68.0K    489.2M   0% /var/volatile
> > 		/dev/vda1               120.4M     19.9M     91.4M  18% /boot
> >
> > Doing the math we see that the /boot partition is ~20MB and in the first image
> > the / partition contains this ~20MB in addition to the rest of the rootfs.
> > This ~20MB is completely wasted since it is used in the / partition, but then
> > the /boot partition is mounted on top of it, making the /boot directory of /
> > inaccessible. After the fix the / partition has an additional ~20MB since the
> > /boot portion is excluded.
> >
> > Fixes [YOCTO #15555]
> >
> > Signed-off-by: Trevor Woerner <twoerner@gmail.com>
> > ---
> 
> Hi Trevor,
> 
> Thanks for the v2, but I believe we still have an issue. Maybe a missing dependency?
> 
> 2025-04-13 19:18:05,421 - oe-selftest - INFO - FAIL: wic.Wic.test_exclude_path_with_extra_space (subunit.RemotedTestCase)
> 2025-04-13 19:18:05,421 - oe-selftest - INFO - ----------------------------------------------------------------------
> 2025-04-13 19:18:05,421 - oe-selftest - INFO - testtools.testresult.real._StringException: Traceback (most recent call last):
>   File "/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/lib/oeqa/selftest/cases/wic.py", line 567, in test_exclude_path_with_extra_space
>     size_of_root_partition = int(runCmd("wic ls %s" % wicout).output.split('\n')[2].split()[3])
>                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   File "/srv/pokybuild/yocto-worker/oe-selftest-debian/build/meta/lib/oeqa/utils/commands.py", line 214, in runCmd
>     raise AssertionError("Command '%s' returned non-zero exit status %d:\n%s" % (command, result.status, exc_output))
> AssertionError: Command 'wic ls /srv/pokybuild/yocto-worker/oe-selftest-debian/build/build-st-2511849/tmp/deploy/images/qemux86-64/core-image-minimal-qemux86-64.rootfs-20250413181535.wic' returned non-zero exit status 1:
> ERROR: Can't find executable parted

Ouch, this might end up taking a couple of iterations. I don't want to blindly
install every wic native dependency, but I'm would be just guessing which
native utilities the buildbots have available to run the "wic ls" command.

Thanks for the feedback!
diff mbox series

Patch

diff --git a/meta/lib/oeqa/selftest/cases/wic.py b/meta/lib/oeqa/selftest/cases/wic.py
index 59fd99a78836..bac06c9b374a 100644
--- a/meta/lib/oeqa/selftest/cases/wic.py
+++ b/meta/lib/oeqa/selftest/cases/wic.py
@@ -535,6 +535,38 @@  part /mnt --source rootfs --ondisk mmcblk0 --fstype=ext4 --exclude-path bin/whoa
         finally:
             os.environ['PATH'] = oldpath
 
+    def test_exclude_path_with_extra_space(self):
+        """Test having --exclude-path with IMAGE_ROOTFS_EXTRA_SPACE. [Yocto #15555]"""
+
+        with NamedTemporaryFile("w", suffix=".wks") as wks:
+            wks.writelines(
+                ['bootloader --ptable gpt\n',
+                 'part /boot --size=100M --active --fstype=ext4 --label boot\n',
+                 'part /     --source rootfs      --fstype=ext4 --label root --exclude-path boot/\n'])
+            wks.flush()
+            config = 'IMAGE_ROOTFS_EXTRA_SPACE = "500000"\n'\
+                     'IMAGE_FSTYPES += "wic"\n'\
+                     'WKS_FILE = "%s"\n' % wks.name
+            self.append_config(config)
+            bitbake('core-image-minimal')
+
+        """
+        the output of "wic ls <image>.wic" will look something like:
+            Num     Start        End          Size      Fstype
+             1         17408    136332287    136314880  ext4
+             2     136332288    171464703     35132416  ext4
+        we are looking for the size of partition 2
+        i.e. in this case the number 35,132,416
+        without the fix the size will be around 85,403,648
+        with the fix the size should be around 799,960,064
+        """
+        bb_vars = get_bb_vars(['DEPLOY_DIR_IMAGE', 'MACHINE'], 'core-image-minimal')
+        deploy_dir = bb_vars['DEPLOY_DIR_IMAGE']
+        machine = bb_vars['MACHINE']
+        wicout = glob(os.path.join(deploy_dir, "core-image-minimal-%s.rootfs-*.wic" % machine))[0]
+        size_of_root_partition = int(runCmd("wic ls %s" % wicout).output.split('\n')[2].split()[3])
+        self.assertGreater(size_of_root_partition, 500000000)
+
     def test_include_path(self):
         """Test --include-path wks option."""
 
diff --git a/scripts/lib/wic/partition.py b/scripts/lib/wic/partition.py
index bf2c34d5940f..b18431d8fb84 100644
--- a/scripts/lib/wic/partition.py
+++ b/scripts/lib/wic/partition.py
@@ -244,7 +244,7 @@  class Partition():
             # from bitbake variable
             rsize_bb = get_bitbake_var('ROOTFS_SIZE')
             rdir = get_bitbake_var('IMAGE_ROOTFS')
-            if rsize_bb and rdir == rootfs_dir:
+            if rsize_bb and (rdir == rootfs_dir or (rootfs_dir.split('/')[-2] == "tmp-wic" and rootfs_dir.split('/')[-1][:6] == "rootfs")):
                 # Bitbake variable ROOTFS_SIZE is calculated in
                 # Image._get_rootfs_size method from meta/lib/oe/image.py
                 # using IMAGE_ROOTFS_SIZE, IMAGE_ROOTFS_ALIGNMENT,