Re: [PATCH for-10.0] tests/functional: Convert the intel_iommu avocado test

[Daniel P . Berrangé]

On Mon, Dec 09, 2024 at 09:26:21AM +0100, Thomas Huth wrote:
> On 09/12/2024 09.12, Eric Auger wrote:
> > Hi Thomas,
> > 
> > On 12/6/24 19:17, Thomas Huth wrote:
> > > Convert the intel_iommu test to the new functional framework.
> > > This test needs some changes since we neither support the old 'LinuxTest'
> > > class in the functional framework yet, nor a way to use SSH for running
> > > commands in the guest. So we now directly download a Fedora kernel and
> > > initrd and set up the serial console for executing the commands and for
> > > looking for the results.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   MAINTAINERS                                   |   1 +
> > >   tests/functional/meson.build                  |   1 +
> > >   .../test_intel_iommu.py}                      | 119 ++++++++----------
> > >   3 files changed, 51 insertions(+), 70 deletions(-)
> > >   rename tests/{avocado/intel_iommu.py => functional/test_intel_iommu.py} 
> > > (41%)
> > >   mode change 100644 => 100755
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index a62659b330..2ca452dbf9 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -3679,6 +3679,7 @@ S: Supported
> > >   F: hw/i386/intel_iommu.c
> > >   F: hw/i386/intel_iommu_internal.h
> > >   F: include/hw/i386/intel_iommu.h
> > > +F: tests/functional/test_intel_iommu.py
> > >   AMD-Vi Emulation
> > >   S: Orphan
> > > diff --git a/tests/functional/meson.build b/tests/functional/meson.build
> > > index 30c3eda7e4..dfc95fd904 100644
> > > --- a/tests/functional/meson.build
> > > +++ b/tests/functional/meson.build
> > > @@ -238,6 +238,7 @@ tests_x86_64_system_quick = [
> > >   tests_x86_64_system_thorough = [
> > >     'acpi_bits',
> > > +  'intel_iommu',
> > >     'x86_64_tuxrun',
> > >     'linux_initrd',
> > >     'multiprocess',
> > > diff --git a/tests/avocado/intel_iommu.py 
> > > b/tests/functional/test_intel_iommu.py
> > > old mode 100644
> > > new mode 100755
> > > similarity index 41%
> > > rename from tests/avocado/intel_iommu.py
> > > rename to tests/functional/test_intel_iommu.py
> > > index 992583fa7d..6e47b1e9de
> > > --- a/tests/avocado/intel_iommu.py
> > > +++ b/tests/functional/test_intel_iommu.py
> > > @@ -1,3 +1,5 @@
> > > +#!/usr/bin/env python3
> > > +#
> > >   # INTEL_IOMMU Functional tests
> > >   #
> > >   # Copyright (c) 2021 Red Hat, Inc.
> > > @@ -7,116 +9,93 @@
> > >   #
> > >   # This work is licensed under the terms of the GNU GPL, version 2 or
> > >   # later.  See the COPYING file in the top-level directory.
> > > -import os
> > > -from avocado import skipUnless
> > > -from avocado_qemu.linuxtest import LinuxTest
> > > +from qemu_test import LinuxKernelTest, Asset, 
> > > exec_command_and_wait_for_pattern
> > > +
> > > +class IntelIOMMU(LinuxKernelTest):
> > > -@skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable on 
> > > GitLab')
> > > -class IntelIOMMU(LinuxTest):
> > > -    """
> > > -    :avocado: tags=arch:x86_64
> > > -    :avocado: tags=distro:fedora
> > > -    :avocado: tags=distro_version:31
> > > -    :avocado: tags=machine:q35
> > > -    :avocado: tags=accel:kvm
> > > -    :avocado: tags=intel_iommu
> > > -    :avocado: tags=flaky
> > > -    """
> > > +    ASSET_KERNEL = Asset(
> > > +        ('https://archives.fedoraproject.org/pub/archive/fedora/linux/'
> > > +         'releases/39/Server/x86_64/os/images/pxeboot/vmlinuz'),
> > > +        
> > > '5f2ef0de47f8d79d5ee9bf8b0ee6d5ba4d987c2f9a16b8b511a7c69e53931fe3')
> > > +
> > > +    ASSET_INITRD = Asset(
> > > +        ('https://archives.fedoraproject.org/pub/archive/fedora/linux/'
> > > +         'releases/39/Server/x86_64/os/images/pxeboot/initrd.img'),
> > > +        
> > > '5bc29e2d872ceeb39a9698d42da3fb0afd7583dc7180de05a6b78bcc726674bb')
> > >       IOMMU_ADDON = 
> > > ',iommu_platform=on,disable-modern=off,disable-legacy=on'
> > > +    default_kernel_params = 'console=ttyS0 rd.rescue quiet '
> > >       kernel_path = None
> > >       initrd_path = None
> > >       kernel_params = None
> > > -    def set_up_boot(self):
> > > -        path = self.download_boot()
> > > -        self.vm.add_args('-device', 'virtio-blk-pci,bus=pcie.0,' +
> > > -                         'drive=drv0,id=virtio-disk0,bootindex=1,'
> > > -                         'werror=stop,rerror=stop' + self.IOMMU_ADDON)
> > > -        self.vm.add_args('-device', 'virtio-gpu-pci' + self.IOMMU_ADDON)
> > > -        self.vm.add_args('-drive',
> > > -                         'file=%s,if=none,cache=writethrough,id=drv0' % 
> > > path)
> > > -
> > > -    def setUp(self):
> > > -        super(IntelIOMMU, self).setUp(None, 'virtio-net-pci' + 
> > > self.IOMMU_ADDON)
> > > -
> > >       def add_common_args(self):
> > >           self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
> > >           self.vm.add_args('-object',
> > >                            'rng-random,id=rng0,filename=/dev/urandom')
> > > +        self.vm.add_args('-device', 'virtio-net-pci' + self.IOMMU_ADDON)
> > > +        self.vm.add_args('-device', 'virtio-gpu-pci' + self.IOMMU_ADDON)
> > > +        self.vm.add_args("-m", "1G")
> > > -    def common_vm_setup(self, custom_kernel=None):
> > > +    def common_vm_setup(self):
> > > +        self.set_machine('q35')
> > >           self.require_accelerator("kvm")
> > >           self.add_common_args()
> > >           self.vm.add_args("-accel", "kvm")
> > > -        if custom_kernel is None:
> > > -            return
> > > -
> > > -        kernel_url = self.distro.pxeboot_url + 'vmlinuz'
> > > -        kernel_hash = '5b6f6876e1b5bda314f93893271da0d5777b1f3c'
> > > -        initrd_url = self.distro.pxeboot_url + 'initrd.img'
> > > -        initrd_hash = 'dd0340a1b39bd28f88532babd4581c67649ec5b1'
> > > -        self.kernel_path = self.fetch_asset(kernel_url, 
> > > asset_hash=kernel_hash)
> > > -        self.initrd_path = self.fetch_asset(initrd_url, 
> > > asset_hash=initrd_hash)
> > > +        self.kernel_path = self.ASSET_KERNEL.fetch()
> > > +        self.initrd_path = self.ASSET_INITRD.fetch()
> > > +        self.kernel_params = self.default_kernel_params
> > >       def run_and_check(self):
> > >           if self.kernel_path:
> > >               self.vm.add_args('-kernel', self.kernel_path,
> > >                                '-append', self.kernel_params,
> > >                                '-initrd', self.initrd_path)
> > > -        self.launch_and_wait()
> > > -        self.ssh_command('cat /proc/cmdline')
> > > -        self.ssh_command('dmesg | grep -e DMAR -e IOMMU')
> > > -        self.ssh_command('find /sys/kernel/iommu_groups/ -type l')
> > > -        self.ssh_command('dnf -y install numactl-devel')
> > I understand you cannot use ssh yet but the bulk of the test was the
> > execution of the dnf install meaning we lose the main substance of it
> > through the conversion.
> 
> Ah, I see, so this was exercising the virtio-net device with the IOMMU ...
> and I already wondered why there was this "dnf install" at the end without
> doing anything with  the numactl-devel package ... (a comment would have
> been helpful here)

FYI, I find 'dnf instal' to be a *highly* undesirable thing todo in
our test functional. Its performance is highly non-deterministic
depending on what mirror you happen to get sent to, such that it could
easily push us over the timeouts. It is also susceptible to periodic
broken mirrors, and instability around time of Fefdora EOL. I can't
remember if it was this test case, or a different one, but I've seen
problems before in avocado with 'dnf install'.

If we want to test working networking, then can we arrange for something
more simple & targetted to run, with better worst case performance.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|