qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] tests/acceptance: Add boot vmlinux test
@ 2019-12-06 14:00 Wainer dos Santos Moschetta
  2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Wainer dos Santos Moschetta @ 2019-12-06 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: fam, alex.bennee, wrampazz, crosa, philmd, sgarzare

This series add a new acceptance test: boot an uncompressed
Linux kernel built with CONFIG_PVH, so checking the PVH
capability introduced in QEMU 4.0 works.

The test implementation depends on [1] which is likely released
on next Avocado. So that will need a version 2 of this
series to bump Avocado version.

Also I want to use this as an example of a scenario that test
assets could be better managed. As you see on patch 01 the
kernel is built at test time on localhost. While Avocado provides
an API to easily fetch and build it, the whole process takes
reasonable time - besides the fact that localhost must have
all build dependencies installed. How could it be done better?

Nonetheless I argue in favor of merging this as is, and
gradually implement corrections to improve the tests assets
management. Also if eventually the test is proven to unacceptable
slow down the Travis CI then I can add a guard to skip it.

[1] https://github.com/avocado-framework/avocado/pull/3389

Wainer dos Santos Moschetta (2):
  tests/acceptance: Add PVH boot test
  .travis.yml: Add kernel build deps for acceptance tests

 .travis.yml             |  2 ++
 tests/acceptance/pvh.py | 48 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 tests/acceptance/pvh.py

-- 
2.21.0



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] tests/acceptance: Add PVH boot test
  2019-12-06 14:00 [PATCH 0/2] tests/acceptance: Add boot vmlinux test Wainer dos Santos Moschetta
@ 2019-12-06 14:00 ` Wainer dos Santos Moschetta
  2019-12-06 15:04   ` Willian Rampazzo
                     ` (2 more replies)
  2019-12-06 14:00 ` [PATCH 2/2] .travis.yml: Add kernel build deps for acceptance tests Wainer dos Santos Moschetta
  2019-12-06 16:42 ` [PATCH 0/2] tests/acceptance: Add boot vmlinux test Cleber Rosa
  2 siblings, 3 replies; 11+ messages in thread
From: Wainer dos Santos Moschetta @ 2019-12-06 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: fam, alex.bennee, wrampazz, crosa, philmd, sgarzare

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
+    """
+    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'])
+            kbuild.build()
+        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)
+        self.vm.launch()
+        wait_for_console_pattern(self, 'Kernel command line: %s' %
+                                 kernel_command_line)
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] .travis.yml: Add kernel build deps for acceptance tests
  2019-12-06 14:00 [PATCH 0/2] tests/acceptance: Add boot vmlinux test Wainer dos Santos Moschetta
  2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
@ 2019-12-06 14:00 ` Wainer dos Santos Moschetta
  2019-12-12 12:22   ` Alex Bennée
  2019-12-06 16:42 ` [PATCH 0/2] tests/acceptance: Add boot vmlinux test Cleber Rosa
  2 siblings, 1 reply; 11+ messages in thread
From: Wainer dos Santos Moschetta @ 2019-12-06 14:00 UTC (permalink / raw)
  To: qemu-devel; +Cc: fam, alex.bennee, wrampazz, crosa, philmd, sgarzare

The tests/acceptance/pvh.py test builds the Linux kernel
at runtime so it needs dependencies installed in the
container. Current used container image misses only
libelf-dev, which is then added with this change.

Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
---
 .travis.yml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c1..d8fe98eb63 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -277,6 +277,8 @@ matrix:
             - python3.5-venv
             - tesseract-ocr
             - tesseract-ocr-eng
+            # Additional kernel build dependencies
+            - libelf-dev
 
 
     # Using newer GCC with sanitizers
-- 
2.21.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] tests/acceptance: Add PVH boot test
  2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
@ 2019-12-06 15:04   ` Willian Rampazzo
  2019-12-06 16:54   ` Cleber Rosa
  2019-12-10 11:16   ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 11+ messages in thread
From: Willian Rampazzo @ 2019-12-06 15:04 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: fam, Alex Bennée, qemu-devel, Cleber Rosa,
	Philippe Mathieu-Daudé,
	sgarzare

On Fri, Dec 6, 2019 at 11:00 AM Wainer dos Santos Moschetta
<wainersm@redhat.com> 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
> +    """
> +    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'])
> +            kbuild.build()
> +        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)
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Kernel command line: %s' %
> +                                 kernel_command_line)
> --
> 2.21.0
>

Tested-by: Willian Rampazzo <wrampazz@redhat.com>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] tests/acceptance: Add boot vmlinux test
  2019-12-06 14:00 [PATCH 0/2] tests/acceptance: Add boot vmlinux test Wainer dos Santos Moschetta
  2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
  2019-12-06 14:00 ` [PATCH 2/2] .travis.yml: Add kernel build deps for acceptance tests Wainer dos Santos Moschetta
@ 2019-12-06 16:42 ` Cleber Rosa
  2019-12-10 11:05   ` Alex Bennée
  2 siblings, 1 reply; 11+ messages in thread
From: Cleber Rosa @ 2019-12-06 16:42 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: fam, alex.bennee, qemu-devel, wrampazz, philmd, sgarzare

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

On Fri, Dec 06, 2019 at 09:00:10AM -0500, Wainer dos Santos Moschetta wrote:
> This series add a new acceptance test: boot an uncompressed
> Linux kernel built with CONFIG_PVH, so checking the PVH
> capability introduced in QEMU 4.0 works.
> 
> The test implementation depends on [1] which is likely released
> on next Avocado. So that will need a version 2 of this
> series to bump Avocado version.
>

Right, the Avocado bits have been merged, so unless a major reversal
comes (not expected at all), it will be on Avocado 74.0.

> Also I want to use this as an example of a scenario that test
> assets could be better managed. As you see on patch 01 the
> kernel is built at test time on localhost. While Avocado provides
> an API to easily fetch and build it, the whole process takes
> reasonable time - besides the fact that localhost must have
> all build dependencies installed. How could it be done better?
>

This is clearly not a "kernel build" test, so we should avoid building
the kernel as part of the "PVH boot" test.  The problem you expose
here is a very real, and each possible solution has its own problems,
unfortunately.

The best solution IMO would be to find a "well known" distribution of
such a kernel.  Maybe something maintained by the Xen project or one
of its commercial products?

The second best solution is to have a helper script (using the Avocado
utils API is fine) that will build a kernel and create/populate the
test's data directory (tests/acceptance/pvh.py.data/) with a vmlinux.
The test can cancel itself if it doesn't find a kernel there.

The third best solution IMO is for this test to require a parameter
telling where the CONFIG_PVH enabled vmlinux kernel image is.

Also notice that, with a custom built kernel image (a different one
for each user), the result of the test as a general indicator to the
QEMU codebase diminishes quite a bit.

> Nonetheless I argue in favor of merging this as is, and
> gradually implement corrections to improve the tests assets
> management. Also if eventually the test is proven to unacceptable
> slow down the Travis CI then I can add a guard to skip it.
>

I'm pretty sure that building the kernel will cause a major slow down
of the Travis CI results for the "acceptance" block (and thus for the
overall results).  But, it may also time it out (there's a 50 minutes
deadline).

I hope this brings some ideas, and thanks for coming up with
interesting problems! :)

- Cleber.

> [1] https://github.com/avocado-framework/avocado/pull/3389
> 
> Wainer dos Santos Moschetta (2):
>   tests/acceptance: Add PVH boot test
>   .travis.yml: Add kernel build deps for acceptance tests
> 
>  .travis.yml             |  2 ++
>  tests/acceptance/pvh.py | 48 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>  create mode 100644 tests/acceptance/pvh.py
> 
> -- 
> 2.21.0
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] tests/acceptance: Add PVH boot test
  2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
  2019-12-06 15:04   ` Willian Rampazzo
@ 2019-12-06 16:54   ` Cleber Rosa
  2019-12-09 14:43     ` Wainer dos Santos Moschetta
  2019-12-10 11:16   ` Philippe Mathieu-Daudé
  2 siblings, 1 reply; 11+ messages in thread
From: Cleber Rosa @ 2019-12-06 16:54 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: fam, alex.bennee, qemu-devel, wrampazz, philmd, sgarzare

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

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
> +    """
> +    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).

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.

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

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

Cheers,
- Cleber.

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

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] tests/acceptance: Add PVH boot test
  2019-12-06 16:54   ` Cleber Rosa
@ 2019-12-09 14:43     ` Wainer dos Santos Moschetta
  2019-12-10  2:21       ` Cleber Rosa
  0 siblings, 1 reply; 11+ messages in thread
From: Wainer dos Santos Moschetta @ 2019-12-09 14:43 UTC (permalink / raw)
  To: Cleber Rosa; +Cc: fam, alex.bennee, qemu-devel, wrampazz, philmd, sgarzare


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
>> +    """
>> +    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. :)

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

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

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] tests/acceptance: Add PVH boot test
  2019-12-09 14:43     ` Wainer dos Santos Moschetta
@ 2019-12-10  2:21       ` Cleber Rosa
  0 siblings, 0 replies; 11+ messages in thread
From: Cleber Rosa @ 2019-12-10  2:21 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: fam, alex.bennee, qemu-devel, wrampazz, philmd, sgarzare

[-- 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 --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] tests/acceptance: Add boot vmlinux test
  2019-12-06 16:42 ` [PATCH 0/2] tests/acceptance: Add boot vmlinux test Cleber Rosa
@ 2019-12-10 11:05   ` Alex Bennée
  0 siblings, 0 replies; 11+ messages in thread
From: Alex Bennée @ 2019-12-10 11:05 UTC (permalink / raw)
  To: Cleber Rosa
  Cc: fam, qemu-devel, Wainer dos Santos Moschetta, wrampazz, philmd, sgarzare


Cleber Rosa <crosa@redhat.com> writes:

> On Fri, Dec 06, 2019 at 09:00:10AM -0500, Wainer dos Santos Moschetta wrote:
>> This series add a new acceptance test: boot an uncompressed
>> Linux kernel built with CONFIG_PVH, so checking the PVH
>> capability introduced in QEMU 4.0 works.
>> 
>> The test implementation depends on [1] which is likely released
>> on next Avocado. So that will need a version 2 of this
>> series to bump Avocado version.
>>
>
> Right, the Avocado bits have been merged, so unless a major reversal
> comes (not expected at all), it will be on Avocado 74.0.
>
>> Also I want to use this as an example of a scenario that test
>> assets could be better managed. As you see on patch 01 the
>> kernel is built at test time on localhost. While Avocado provides
>> an API to easily fetch and build it, the whole process takes
>> reasonable time - besides the fact that localhost must have
>> all build dependencies installed. How could it be done better?
>>
>
> This is clearly not a "kernel build" test, so we should avoid building
> the kernel as part of the "PVH boot" test.  The problem you expose
> here is a very real, and each possible solution has its own problems,
> unfortunately.
>
> The best solution IMO would be to find a "well known" distribution of
> such a kernel.  Maybe something maintained by the Xen project or one
> of its commercial products?
>
> The second best solution is to have a helper script (using the Avocado
> utils API is fine) that will build a kernel and create/populate the
> test's data directory (tests/acceptance/pvh.py.data/) with a vmlinux.
> The test can cancel itself if it doesn't find a kernel there.
>
> The third best solution IMO is for this test to require a parameter
> telling where the CONFIG_PVH enabled vmlinux kernel image is.
>
> Also notice that, with a custom built kernel image (a different one
> for each user), the result of the test as a general indicator to the
> QEMU codebase diminishes quite a bit.

I can see a use case for making it easier for developers to build test
cases otherwise everyone has their own particular variant of the kernel
and buildroot/busybox which they have in their own farm. However as
Cleber has noted they make poor standardised tests given the variation
in kernel builds you can get.

That said I think there are better targets. kvm-unit-tests can be built
against a range of architectures and are fashioned as individual unit
tests for specific functionality. It would be useful to make it easy for
a developer to regenerate the test assets to re-run a test someone else
has found to fail.

>> Nonetheless I argue in favor of merging this as is, and
>> gradually implement corrections to improve the tests assets
>> management. Also if eventually the test is proven to unacceptable
>> slow down the Travis CI then I can add a guard to skip it.
>>
>
> I'm pretty sure that building the kernel will cause a major slow down
> of the Travis CI results for the "acceptance" block (and thus for the
> overall results).  But, it may also time it out (there's a 50 minutes
> deadline).
>
> I hope this brings some ideas, and thanks for coming up with
> interesting problems! :)
>
> - Cleber.
>
>> [1] https://github.com/avocado-framework/avocado/pull/3389
>> 
>> Wainer dos Santos Moschetta (2):
>>   tests/acceptance: Add PVH boot test
>>   .travis.yml: Add kernel build deps for acceptance tests
>> 
>>  .travis.yml             |  2 ++
>>  tests/acceptance/pvh.py | 48 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>  create mode 100644 tests/acceptance/pvh.py
>> 
>> -- 
>> 2.21.0
>> 


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] tests/acceptance: Add PVH boot test
  2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
  2019-12-06 15:04   ` Willian Rampazzo
  2019-12-06 16:54   ` Cleber Rosa
@ 2019-12-10 11:16   ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-12-10 11:16 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta, qemu-devel, crosa
  Cc: fam, alex.bennee, wrampazz, sgarzare

On 12/6/19 3:00 PM, 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

Why is it slow? Because of the time spent to build the kernel?

This should be split in "prepare test" (here: slow) VS "run test", like 
we do when downloading assets.

> +    """
> +    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'])
> +            kbuild.build()

I like this feature, but I don't think it should be default for all 
users, they might start complaining, such:
- test drained battery while using laptop on the move
- test filled $HOME

This should be configurable, like users suggested they don't want the 
"download required assets from internet" feature.

Is it possible to build this once (ideally on some CI) and add the 
result binary + SHA1 hash in the Avocado assets public repository?

So users can fetch the prebuilt image, and are free to rebuild it.

> +        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)
> +        self.vm.launch()
> +        wait_for_console_pattern(self, 'Kernel command line: %s' %
> +                                 kernel_command_line)
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] .travis.yml: Add kernel build deps for acceptance tests
  2019-12-06 14:00 ` [PATCH 2/2] .travis.yml: Add kernel build deps for acceptance tests Wainer dos Santos Moschetta
@ 2019-12-12 12:22   ` Alex Bennée
  0 siblings, 0 replies; 11+ messages in thread
From: Alex Bennée @ 2019-12-12 12:22 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: fam, qemu-devel, wrampazz, crosa, philmd, sgarzare


Wainer dos Santos Moschetta <wainersm@redhat.com> writes:

> The tests/acceptance/pvh.py test builds the Linux kernel
> at runtime so it needs dependencies installed in the
> container. Current used container image misses only
> libelf-dev, which is then added with this change.

Hmm I'm not sure about this. Do we really want to building a whole
kernel inside Travis? There must be stable PVH aware distro kernels we
could use for the CI?

>
> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
> ---
>  .travis.yml | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/.travis.yml b/.travis.yml
> index 445b0646c1..d8fe98eb63 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -277,6 +277,8 @@ matrix:
>              - python3.5-venv
>              - tesseract-ocr
>              - tesseract-ocr-eng
> +            # Additional kernel build dependencies
> +            - libelf-dev
>  
>  
>      # Using newer GCC with sanitizers


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-12-12 12:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06 14:00 [PATCH 0/2] tests/acceptance: Add boot vmlinux test Wainer dos Santos Moschetta
2019-12-06 14:00 ` [PATCH 1/2] tests/acceptance: Add PVH boot test Wainer dos Santos Moschetta
2019-12-06 15:04   ` Willian Rampazzo
2019-12-06 16:54   ` Cleber Rosa
2019-12-09 14:43     ` Wainer dos Santos Moschetta
2019-12-10  2:21       ` Cleber Rosa
2019-12-10 11:16   ` Philippe Mathieu-Daudé
2019-12-06 14:00 ` [PATCH 2/2] .travis.yml: Add kernel build deps for acceptance tests Wainer dos Santos Moschetta
2019-12-12 12:22   ` Alex Bennée
2019-12-06 16:42 ` [PATCH 0/2] tests/acceptance: Add boot vmlinux test Cleber Rosa
2019-12-10 11:05   ` Alex Bennée

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).