diff mbox series

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

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

Commit Message

Trevor Woerner April 15, 2025, 2:04 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 v3:
- add a dependency on wic-tools so everything that might be needed in
  order to run wic (e.g. parted-native) is available

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

Comments

Richard Purdie April 22, 2025, 6:21 a.m. UTC | #1 
On Mon, 2025-04-14 at 22:04 -0400, 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>
> ---
> changes in v3:
> - add a dependency on wic-tools so everything that might be needed in
>   order to run wic (e.g. parted-native) is available
> 
> changes in v2:
> - fix bug to not process the root partition removing the requirement to
>   define the IMAGE_BOOT_FILES variable
> - reword the commit message so the wks reproducer lines better match what
>   is in the test
> ---
>  meta/lib/oeqa/selftest/cases/wic.py | 33 +++++++++++++++++++++++++++++
>  scripts/lib/wic/partition.py        |  2 +-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/lib/oeqa/selftest/cases/wic.py b/meta/lib/oeqa/selftest/cases/wic.py
> index 59fd99a78836..d98af8713a19 100644
> --- a/meta/lib/oeqa/selftest/cases/wic.py
> +++ b/meta/lib/oeqa/selftest/cases/wic.py
> @@ -535,6 +535,39 @@ 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'\
> +                     'DEPENDS:pn-core-image-minimal += "wic-tools"\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,
> 

This did merge and I appreciate the test being added but we're seeing
an intermittent failure in it, e.g.:

https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1393
https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1389
https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1385
https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1381
https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1376
https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1371


2025-04-18 01:09:23,876 - oe-selftest - INFO - FAIL: wic.Wic.test_exclude_path_with_extra_space (subunit.RemotedTestCase)
2025-04-18 01:09:23,876 - oe-selftest - INFO - ----------------------------------------------------------------------
2025-04-18 01:09:23,876 - 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 568, 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-4185923/tmp/deploy/images/qemux86-64/core-image-minimal-qemux86-64.rootfs-20250417235256.wic' returned non-zero exit status 1:
ERROR: Can't find executable parted

I think you may need something like:

    oldpath = os.environ['PATH']
    os.environ['PATH'] = get_bb_var("PATH", "wic-tools")
    try:
        <wic lc>
     finally:
        os.environ['PATH'] = oldpath

in the test.

Cheers,

Richard
diff mbox series

Patch

diff --git a/meta/lib/oeqa/selftest/cases/wic.py b/meta/lib/oeqa/selftest/cases/wic.py
index 59fd99a78836..d98af8713a19 100644
--- a/meta/lib/oeqa/selftest/cases/wic.py
+++ b/meta/lib/oeqa/selftest/cases/wic.py
@@ -535,6 +535,39 @@  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'\
+                     'DEPENDS:pn-core-image-minimal += "wic-tools"\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,