Re: [PATCH 1/2] tests/acceptance: Add PVH boot test

[Cleber Rosa <crosa@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7681 bytes --]

On Mon, Dec 09, 2019 at 12:43:22PM -0200, Wainer dos Santos Moschetta wrote:
> 
> On 12/6/19 2:54 PM, Cleber Rosa wrote:
> > On Fri, Dec 06, 2019 at 09:00:11AM -0500, Wainer dos Santos Moschetta wrote:
> > > QEMU 4.0 onward is able to boot an uncompressed kernel
> > > image by using the x86/HVM direct boot ABI. It needs
> > > Linux >= 4.21 built with CONFIG_PVH=y.
> > > 
> > > This introduces an acceptance test which checks an
> > > uncompressed Linux kernel image boots properly.
> > > 
> > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > ---
> > >   tests/acceptance/pvh.py | 48 +++++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 48 insertions(+)
> > >   create mode 100644 tests/acceptance/pvh.py
> > > 
> > > diff --git a/tests/acceptance/pvh.py b/tests/acceptance/pvh.py
> > > new file mode 100644
> > > index 0000000000..c68489c273
> > > --- /dev/null
> > > +++ b/tests/acceptance/pvh.py
> > > @@ -0,0 +1,48 @@
> > > +# Copyright (c) 2019 Red Hat, Inc.
> > > +#
> > > +# Author:
> > > +#  Wainer dos Santos Moschetta <wainersm@redhat.com>
> > > +#
> > > +# This work is licensed under the terms of the GNU GPL, version 2 or
> > > +# later.  See the COPYING file in the top-level directory.
> > > +
> > > +"""
> > > +x86/HVM direct boot acceptance tests.
> > > +"""
> > > +
> > > +from avocado.utils.kernel import KernelBuild
> > > +from avocado_qemu import Test
> > > +from avocado_qemu import wait_for_console_pattern
> > > +
> > > +
> > > +class Pvh(Test):
> > > +    """
> > > +    Test suite for x86/HVM direct boot feature.
> > > +
> > > +    :avocado: tags=slow,arch=x86_64,machine=q35

This should be:

   :avocado: tags=slow,arch:x86_64,machine:q35

That is, the separator of key/val is ':', because the equal sign is
used to separate the docstring directive type (here it's "tags") from
their content.  `avocado list -V` should show you the tag keys with
all their values inside a parenthesis.  That is, for the following
docstring directive:

   :avocado: tags=slow,arch:x86_64,machine:q35,machine:pc

You'd get:

   slow,arch(x86_64),machine(q35,pc)

> > > +    """
> > > +    def test_boot_vmlinux(self):
> > > +        """
> > > +        Boot uncompressed kernel image.
> > > +        """
> > > +        # QEMU can boot a vmlinux image for kernel >= 4.21 built
> > > +        # with CONFIG_PVH=y
> > > +        kernel_version = '5.4.1'
> > > +        kbuild = KernelBuild(kernel_version, work_dir=self.workdir)
> > > +        try:
> > > +            kbuild.download()
> > > +            kbuild.uncompress()
> > > +            kbuild.configure(targets=['defconfig', 'kvmconfig'],
> > > +                             extra_configs=['CONFIG_PVH=y'])
> > I'm aware of the reason why this uses APIs not fulfilled by
> > tests/requirements.txt, but, for the general public reviewing/testing
> > code with extra requirements, it's a good idea to bump the
> > requirements to a version that fulfills the requirement, and comment
> > out clearly on the temporary nature of the change (marking the patch).
> 
> Good idea, thanks for the tip.
> 
> > 
> > For instance, for this requirement, we could have:
> > 
> > diff --git a/tests/requirements.txt b/tests/requirements.txt
> > index a2a587223a..5498d67bc1 100644
> > --- a/tests/requirements.txt
> > +++ b/tests/requirements.txt
> > @@ -1,4 +1,5 @@
> >   # Add Python module requirements, one per line, to be installed
> >   # in the tests/venv Python virtual environment. For more info,
> >   # refer to: https://pip.pypa.io/en/stable/user_guide/#id1
> > -avocado-framework==72.0
> > +# [REMOVE ME] use post 73.0 Avocado containing the new kernel build APIs
> > +-e git+https://github.com/avocado-framework/avocado@d6fb24edcf847f09c312b55df3c674c64c79793e#egg=avocado_framework
> > 
> > This will not only help people to test it, but should also make
> > it work transparently on CI.
> 
> True. It could had helped me to check the missing packages on Travis to
> build the kernel. I'm ashamed to tell how I did it. :)
>

Don't be, because you did check it. :)

> > 
> > > +            kbuild.build()
> > As stated in my response to the cover letter, I think we need to move
> > this elsewhere.  The *very* minimum is to have this in a setUp()
> > method, but we should strongly consider other solutions.
> 
> On the proposed implementation the kernel is built only once and only for
> this test case. If I move the code to setUp() it will attempt to build the
> vmlinux for every case even when not needed (suppose I add a 'boot not
> CONFIG_PVH vmlinux to check it properly handle error' case which uses
> distro's kernel). Unless I put a guard like 'do not build if already
> present' which IMHO is weird. In other words, IMHO setUp() should hold only
> code that is share-able across cases.
>

I was thinking of *this* test setUp(), not avocado_qemu.Test.setUp().

Anyway, looking at the other options we talked about, I was able to
boot a vmlinux image from a "mainstream distro" kernel package[1] that
already has CONFIG_PVH enabled[2] with recent QEMU (and also tested
that I wasn't able to do so with older QEMU).  Other distros also
provide a vmlinux image, but as part of the debuginfo packages and
they can be HUGE, so not recommended here.

If we go with this route, compilation would be a non-issue, and this
test would be just like the other "boot_linux_console.py" tests.

> > 
> > > +        except:
> > > +            self.cancel("Unable to build vanilla kernel %s" % kernel_version)
> > > +
> > > +        self.vm.set_machine('q35')
> > > +        self.vm.set_console()
> > > +        kernel_command_line = 'printk.time=0 console=ttyS0'
> > > +        self.vm.add_args('-kernel', kbuild.vmlinux,
> > > +                         '-append', kernel_command_line)
> > And just for being thorough (and purist? idealistic? Utopian? :), if
> > we stop and think about it, the following two lines are really what
> > this test is all about.  Everything else should be the test's setup.
> > 
> > I'm not arguing in favor of being radical and reject anything that is
> > not perfect, but just reminding ourselves (myself very much included)
> > of this general direction.
> 
> IMHO we should merge tests which are "good enough" then interactively
> improve them. At least they will run with some frequency and eventually
> catch regressions while infra bits are improved. Now, what's 'good enough'
> for an acceptance test? Perhaps a test that run consistently?
>

Right.  But even though this test can be proven stable (I can't
disprove it), we also have to watch for the overall user experience.
Like I've said before, I don't think users running this test are
interested in building a kernel, but asserting a QEMU feature, and
that can be a source of "test distrust" IMO.

> > 
> > Cheers,
> > - Cleber.
> > 
> > > +        self.vm.launch()
> > > +        wait_for_console_pattern(self, 'Kernel command line: %s' %
> > > +                                 kernel_command_line)
> > > -- 
> > > 2.21.0
> > >

Please let me know what you think of reusing an available kernel instead
of building one.

Cheers,
- Cleber.

[1] https://download.opensuse.org/repositories/openSUSE:/Factory/standard/x86_64/kernel-vanilla-5.3.12-1.1.x86_64.rpm
[2] https://kernel.opensuse.org/cgit/kernel-source/tree/config/x86_64/vanilla?h=linux-next&id=03bbea2f5521b0fe7bae800297509e9ca4c23117#n331
[3] http://mirrors.syringanetworks.net/fedora/linux/releases/31/Everything/x86_64/debug/tree/Packages/k/


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]