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

["Moessbauer, Felix" <felix.moessbauer@siemens.com>]

> -----Original Message-----
> From: Anton Mikanovich <amikan@ilbers.de>
> Sent: Monday, May 23, 2022 4:19 PM
> To: Moessbauer, Felix (T CED SES-DE) <felix.moessbauer@siemens.com>; isar-
> users@googlegroups.com
> Cc: Schild, Henning (T CED SES-DE) <henning.schild@siemens.com>; Kiszka, Jan
> (T CED) <jan.kiszka@siemens.com>; Bezdeka, Florian (T CED SES-DE)
> <florian.bezdeka@siemens.com>
> Subject: Re: [PATCH v3 4/4] ci: test partition layout of splitted-rootfs image
> 
> 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.

Looks like the avocado partition module has bugs.
Whenever I mount a partition with the module, I'm no longer able to unmount it (resource busy...).
The python process still has a handle somewhere on the partition.

By that, I'll stick to the umount logic but remove the laziness.

> 
> 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.

Yes.

> > +
> > +        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

Will be covered in v4.

Felix