Re: [PATCH v3 4/4] ci: test partition layout of splitted-rootfs image

[Anton Mikanovich <amikan@ilbers.de>]

12.05.2022 15:15, Felix Moessbauer wrote:
> This adds a test that checks if splitting the rootfs
> across multiple partitions correctly works.
>
> The following is checked:
>
> file-permissions (based on wic internal check):
> WIC already has a built-in check for correct
> file permissions. In case the permission db is missing,
> a WIC warning is emitted. We simply check for this warning.
>
> partition layout and excluded paths:
> Specifying exclude paths is error prone.
> Hence, we check that all generated partitions do not contain
> excluded paths. Further, we check that the expected paths are there.
>
> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> ---
>   testsuite/cibase.py    | 58 ++++++++++++++++++++++++++++++++++++++++++
>   testsuite/cibuilder.py |  6 ++++-
>   testsuite/citest.py    | 14 ++++++++++
>   3 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/testsuite/cibase.py b/testsuite/cibase.py
> index b25c356e..fd272ef5 100755
> --- a/testsuite/cibase.py
> +++ b/testsuite/cibase.py
> @@ -164,3 +164,61 @@ class CIBaseTest(CIBuilder):
>                       ['do_rootfs_install_setscene', '!do_rootfs_install'])
>               ]):
>               self.fail("Failed rebuild package and image")
> +
> +    def perform_partition_layout_test(self, mc_target, **kwargs):
> +        def _mount(device, mountpoint):
> +            process.run(f'mount -o ro {device} {mountpoint}', sudo=True, ignore_status=False)
> +
> +        def _umount(mountpoint):
> +            process.run(f'umount -l {mountpoint}', sudo=True, ignore_status=True)
Do not introduce new lazy umounts, please.
If umount fails we need to fix the reason, but do not mask it.

You can maybe use avocado.utils.partition.Partition class for all
mount, umount and mountpoint check operations.
> +
> +        def check_output_for_warnings(target, machine):
> +            wic_output_files = glob.glob(f'{self.build_dir}/tmp/work/*/{target}-{machine}/*/temp/log.do_image_wic')
> +            if len(wic_output_files) == 0:
> +                self.fail('could not find WIC output file')
> +            with open(wic_output_files[0], 'r') as output:
> +                # in case we had permission problems, a WIC warning is in the logs
> +                for line in output:
> +                    token = line.split()
> +                    if len(token) > 0 and token[0] == 'WARNING:':
> +                        self.fail(f'WIC issue found: {line}')
> +
> +        def check_part_layout(target, machine):
> +            pattern = f'{self.build_dir}/tmp/deploy/images/{machine}/{target}-*-{machine}.wic.p*'
> +            partitions = sorted(glob.glob(pattern))
> +            if len(partitions) != 3:
Can you make partitions count and mount names to be configurable?
It will make this code easier to maintain in case any target changes.
> +                self.fail(f'expected 3 partitions, but got {len(partitions)} (in {pattern})')
> +            mountpoints = [os.path.join(self.build_dir, mp) for mp in 'mnt_efi mnt_root mnt_home'.split()]
> +            for i in range(3):
> +                os.makedirs(mountpoints[i], exist_ok=True)
> +                _umount(mountpoints[i])
Why not to check for mountpoint here?
Unmounting without check will cause unnecessary errors in job logs.
Moreover, what is the case when this umount will be really needed?
> +
> +        def check_output_for_warnings(target, machine):
> +            wic_output_files = glob.glob(f'{self.build_dir}/tmp/work/*/{target}-{machine}/*/temp/log.do_image_wic')
> +            if len(wic_output_files) == 0:
> +                self.fail('could not find WIC output file')
> +            with open(wic_output_files[0], 'r') as output:
> +                # in case we had permission problems, a WIC warning is in the logs
> +                for line in output:
> +                    token = line.split()
> +                    if len(token) > 0 and token[0] == 'WARNING:':
> +                        self.fail(f'WIC issue found: {line}')
> +
> +        def check_part_layout(target, machine):
> +            pattern = f'{self.build_dir}/tmp/deploy/images/{machine}/{target}-*-{machine}.wic.p*'
> +            partitions = sorted(glob.glob(pattern))
> +            if len(partitions) != 3:
> +                self.fail(f'expected 3 partitions, but got {len(partitions)} (in {pattern})')
> +            mountpoints = [os.path.join(self.build_dir, mp) for mp in 'mnt_efi mnt_root mnt_home'.split()]
> +            for i in range(3):
> +                os.makedirs(mountpoints[i], exist_ok=True)
> +                _umount(mountpoints[i])
> +                _mount(partitions[i], mountpoints[i])
> +
> +            # in boot partition, we expect /boot
> +            if not os.path.isdir(os.path.join(mountpoints[0], 'EFI')):
> +                self.fail('boot partition does not provide /EFI')
> +            # in root partition, boot and home should be excluded
> +            if not os.path.isdir(os.path.join(mountpoints[1], 'etc')) or \
> +               os.path.isdir(os.path.join(mountpoints[1], 'home')) or \
> +               os.path.isdir(os.path.join(mountpoints[1], 'boot')):
> +                self.fail('root partition does not contain expected dirs')
> +            # home partition should contain home of user "user"
> +            if not os.path.isdir(os.path.join(mountpoints[2], 'user')):
> +                self.fail('home partition does not contain home of user')
> +
> +            [_umount(mnt) for mnt in mountpoints]
Please cover mount-umount stuff with try-finally to be sure there will be no
mounts left in case of failure.
> +
> +        mc_spec = mc_target.split(':')
> +        target = mc_spec[2]
> +        machine = mc_spec[1].split('-')[0]
> +
> +        self.configure(
> +            sstate=False,
> +            compat_arch=False,
> +            interactive_user=True,
> +            **kwargs)
> +
> +        self.bitbake(mc_target, **kwargs)
> +
> +        check_output_for_warnings(target, machine)
> +        check_part_layout(target, machine)
> diff --git a/testsuite/cibuilder.py b/testsuite/cibuilder.py
> index bc48d47f..e3fbb859 100755
> --- a/testsuite/cibuilder.py
> +++ b/testsuite/cibuilder.py
> @@ -54,7 +54,7 @@ class CIBuilder(Test):
>   
>       def configure(self, compat_arch=True, cross=None, debsrc_cache=False,
>                     container=False, ccache=False, sstate=False, offline=False,
> -                  gpg_pub_key=None, **kwargs):
> +                  gpg_pub_key=None, interactive_user=False, **kwargs):
>           # write configuration file and set bitbake_args
>           # can run multiple times per test case
>           self.check_init()
> @@ -107,6 +107,10 @@ class CIBuilder(Test):
>                   f.write('BASE_REPO_KEY="file://' + gpg_pub_key + '"\n')
>               if distro_apt_premir:
>                   f.write('DISTRO_APT_PREMIRRORS = "%s"\n' % distro_apt_premir)
> +            if interactive_user:
> +                f.write('USERS += "user"\n')
> +                f.write('USER_user[home] = "/home/user"\n')
> +                f.write('USER_user[flags] = "create-home"\n')
>   
>           # include ci_build.conf in local.conf
>           with open(self.build_dir + '/conf/local.conf', 'r+') as f:
> diff --git a/testsuite/citest.py b/testsuite/citest.py
> index 8d00054a..26101010 100755
> --- a/testsuite/citest.py
> +++ b/testsuite/citest.py
> @@ -318,3 +318,17 @@ class VmBootTestFull(CIBaseTest):
>       def test_amd64_focal(self):
>           self.init()
>           self.vm_start('amd64','focal')
> +
> +class SplittedRootfsTest(CIBaseTest):
> +
> +    """
> +    Test partition layout of splitted rootfs image
> +
> +    :avocado: tags=splittedrootfs,full
> +    """
> +
> +    def test_wic_partition_layout(self):
> +        mc_target = 'mc:qemuamd64-bookworm:isar-image-base'
> +
> +        self.init('build-wic-rootfs')
> +        self.perform_partition_layout_test(mc_target)
Do we need the logs from build-wic-rootfs for analyzing in case CI fails?
If yes - this path should be added into 'artifacts' part of .gitlab-ci.yml