Re: [PATCH v8 2/4] Acceptance test: add "boot_linux" tests

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/19/19 1:38 AM, Cleber Rosa wrote:
> On Thu, Dec 19, 2019 at 01:12:02AM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Cleber,
>>
>> Few minor questions...
>>
>> On 12/19/19 12:24 AM, Cleber Rosa wrote:
>>> This acceptance test, validates that a full blown Linux guest can
>>> successfully boot in QEMU.  In this specific case, the guest chosen is
>>> Fedora version 31.
>>>
>>>    * x86_64, pc and q35 machine types, with and without kvm as an
>>>      accelerator
>>>
>>>    * aarch64 and virt machine type, with and without kvm as an
>>>      accelerator
>>>
>>>    * ppc64 and pseries machine type
>>>
>>>    * s390x and s390-ccw-virtio machine type
>>>
>>> The Avocado vmimage utils library is used to download and cache the
>>> Linux guest images, and from those images a snapshot image is created
>>> and given to QEMU.  If a qemu-img binary is available in the build
>>> directory, it's used to create the snapshot image, so that matching
>>> qemu-system-* and qemu-img are used in the same test run.  If qemu-img
>>> is not available in the build tree, one is attempted to be found
>>> installed system-wide (in the $PATH).  If qemu-img is not found in the
>>> build dir or in the $PATH, the test is canceled.
>>>
>>> The method for checking the successful boot is based on "cloudinit"
>>> and its "phone home" feature.  The guest is given an ISO image with
>>> the location of the phone home server, and the information to post
>>> (the instance ID).  Upon receiving the correct information, from the
>>> guest, the test is considered to have PASSed.
>>>
>>> This test is currently limited to user mode networking only, and
>>> instructs the guest to connect to the "router" address that is hard
>>> coded in QEMU.
>>>
>>> To create the cloudinit ISO image that will be used to configure the
>>> guest, the pycdlib library is also required and has been added as
>>> requirement to the virtual environment created by "check-venv".
>>>
>>> The console output is read by a separate thread, by means of the
>>> Avocado datadrainer utility module.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>    .travis.yml                    |   2 +-
>>>    tests/acceptance/boot_linux.py | 180 +++++++++++++++++++++++++++++++++
>>>    tests/requirements.txt         |   3 +-
>>>    3 files changed, 183 insertions(+), 2 deletions(-)
>>>    create mode 100644 tests/acceptance/boot_linux.py
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 6cb8af6fa5..10c24330fd 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -264,7 +264,7 @@ matrix:
>>>        # Acceptance (Functional) tests
>>>        - env:
>>> -        - CONFIG="--python=/usr/bin/python3 --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
>>> +        - CONFIG="--python=/usr/bin/python3 --enable-tools --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
>>>            - TEST_CMD="make check-acceptance"
>>>          after_failure:
>>>            - cat tests/results/latest/job.log
>>> diff --git a/tests/acceptance/boot_linux.py b/tests/acceptance/boot_linux.py
>>> new file mode 100644
>>> index 0000000000..495ff2963c
>>> --- /dev/null
>>> +++ b/tests/acceptance/boot_linux.py
>>> @@ -0,0 +1,180 @@
>>> +# Functional test that boots a complete Linux system via a cloud image
>>> +#
>>> +# Copyright (c) 2018-2019 Red Hat, Inc.
>>> +#
>>> +# Author:
>>> +#  Cleber Rosa <crosa@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.
>>> +
>>> +import os
>>> +
>>> +from avocado_qemu import Test, BLD_DIR
>>> +
>>> +from qemu.accel import kvm_available
>>> +
>>> +from avocado.utils import cloudinit
>>> +from avocado.utils import network
>>> +from avocado.utils import vmimage
>>> +from avocado.utils import datadrainer
>>> +
>>> +
>>> +KVM_NOT_AVAILABLE = "KVM accelerator does not seem to be available"
>>> +
>>> +
>>> +class BootLinux(Test):
>>> +    """
>>> +    Boots a Linux system, checking for a successful initialization
>>> +    """
>>> +
>>> +    timeout = 900
>>> +    chksum = None
>>> +
>>> +    def setUp(self):
>>> +        super(BootLinux, self).setUp()
>>> +        self.prepare_boot()
>>> +        self.vm.add_args('-smp', '2')
>>
>> Hmmm are we assuming everybody has multicore systems?
>>
> 
> Not really, but isn't it possible to have virtual CPUs > actual CPUs?
> IMO testing with smp > 1 is a better test than with smp == 1.

I guess it is unusual to run acceptance tests on non-multicore systems, 
so this is probably not an issue.

>>> +        self.vm.add_args('-m', '2048')
>>
>> We should not fail the test if this condition is not possible.
>>
> 
> You mean from the host side, right?  I have doubts about what to do
> here, in the sense that we can't easily and reliably set aside memory
> in the system to run qemu.  We could of course check the amount of
> physical or free memory in the system at the test start time, but
> there would still be somewhat of a race condition.

See:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg652535.html

On 15/10/2019 19:03, Peter Maydell wrote:
 > Hi. I just discovered that this makes 'make check' fail on
 > 32-bit systems, because you can't default to 2GB of RAM
 > for a board:
 >
 > (armhf)pmaydell@mustang-maydell:~/qemu$
 > ./build/all-a32/arm-softmmu/qemu-system-arm -M ast2600-evb
 > qemu-system-arm: at most 2047 MB RAM can be simulated

Looking at:
https://docs.python.org/2/library/platform.html#cross-platform

Maybe we can use:

   @skipUnless(sys.maxsize > 2**32, 'Require 64-bit host')

> 
> Also, with tests running in parallel (ie, avocado nrun
> tests/acceptance/), this would be even trickier...

Yeah this is a complex case. Maybe for now we can add a tag for the 
minimum of memory required for a test. Then later it might be easier 
figure something.

This class can help:
https://psutil.readthedocs.io/en/latest/#psutil.virtual_memory

>>> +        self.vm.add_args('-drive', 'file=%s' % self.boot.path)
>>> +        self.prepare_cloudinit()
>>> +
>>> +    def prepare_boot(self):
>>> +        self.log.info('Downloading/preparing boot image')
>>> +        # Fedora 31 only provides ppc64le images
>>> +        image_arch = self.arch
>>> +        if image_arch == 'ppc64':
>>> +            image_arch = 'ppc64le'
>>> +        # If qemu-img has been built, use it, otherwise the system wide one
>>> +        # will be used.  If none is available, the test will cancel.
>>> +        qemu_img = os.path.join(BLD_DIR, 'qemu-img')
>>> +        if os.path.exists(qemu_img):
>>> +            vmimage.QEMU_IMG = qemu_img
>>> +        try:
>>> +            self.boot = vmimage.get(
>>> +                'fedora', arch=image_arch, version='31',
>>> +                checksum=self.chksum,
>>> +                algorithm='sha256',
>>> +                cache_dir=self.cache_dirs[0],
>>> +                snapshot_dir=self.workdir)
>>> +        except:
>>> +            self.cancel('Failed to download/prepare boot image')
>>> +
>>> +    def prepare_cloudinit(self):
>>> +        self.log.info('Preparing cloudinit image')
>>> +        try:
>>> +            cloudinit_iso = os.path.join(self.workdir, 'cloudinit.iso')
>>> +            self.phone_home_port = network.find_free_port()
>>> +            cloudinit.iso(cloudinit_iso, self.name,
>>> +                          username='root',
>>> +                          password='password',
>>> +                          # QEMU's hard coded usermode router address
>>> +                          phone_home_host='10.0.2.2',
>>> +                          phone_home_port=self.phone_home_port)
>>> +            self.vm.add_args('-drive', 'file=%s,format=raw' % cloudinit_iso)
>>> +        except Exception:
>>> +            self.cancel('Failed to prepared cloudinit image')
>>> +
>>> +    def launch_and_wait(self):
>>> +        self.vm.set_console()
>>> +        self.vm.launch()
>>> +        console_drainer = datadrainer.LineLogger(self.vm.console_socket.fileno(),
>>> +                                                 logger=self.log.getChild('console'))
>>> +        console_drainer.start()
>>> +        self.log.info('VM launched, waiting for boot confirmation from guest')
>>> +        cloudinit.wait_for_phone_home(('0.0.0.0', self.phone_home_port), self.name)
>>> +
>>> +
>>> +class BootLinuxX8664(BootLinux):
>>> +    """
>>> +    :avocado: tags=arch:x86_64
>>> +    """
>>> +
>>> +    chksum = 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'
>>> +
>>> +    def test_pc(self):
>>
>> I'd name this test_pc_i440fx_tcg, but are you sure the default is tcg?
>>
> 
> Even if there's no matching machine type named "pc-i440fx"?

Hmm 'pc' is a confusing alias, kept for backward compatibility...
'q35' is also an alias, see:

$ qemu-system-x86_64 -M help
Supported machines are:
pc                   Standard PC (i440FX + PIIX, 1996) (alias of 
pc-i440fx-3.1)
pc-i440fx-3.1        Standard PC (i440FX + PIIX, 1996) (default)
pc-i440fx-3.0        Standard PC (i440FX + PIIX, 1996)
pc-i440fx-2.9        Standard PC (i440FX + PIIX, 1996)
...
q35                  Standard PC (Q35 + ICH9, 2009) (alias of pc-q35-3.1)
pc-q35-3.1           Standard PC (Q35 + ICH9, 2009)
pc-q35-3.0           Standard PC (Q35 + ICH9, 2009)
pc-q35-2.9           Standard PC (Q35 + ICH9, 2009)
pc-q35-2.8           Standard PC (Q35 + ICH9, 2009)
...
isapc                ISA-only PC
none                 empty machine
xenfv                Xen Fully-virtualized PC
xenpv                Xen Para-virtualized PC

Historically the 'pc' machine was aiming at modeling a 'PC compatible' 
machine, as described here:
https://en.wikipedia.org/wiki/IBM_PC_compatible

We currently have 3 emulated PC machines (excluding the virtual machines 
such MicroVM, Xen).

Some history:

- commit 0824d6fc6
   The first pc machine is introduced, inside main().
   It only use an ISA bus. Today we know this as 'isapc'.

- commit 80cabfad1
   Refactor, the pc machine is extracted to hw/pc.c

- commit 69b910399
   PCI support is added.

- commit bb0c6722b
   PCI is used by default. The machine becomes what we today
   calls 'pc'.

- commit b5ff2d6e2
   The machine is officially called 'pc'.

- commit 3dbbdc255
   The 'isapc' machine is added back: it is the 'pc' machine
   with PCI disabled.

- commit df2d8b3ed
   The 'q35' machine is added.
   Instead of a i440FX northbridge and PIIX3 southbridge
   chipsets, it uses a Q35 northbridge and ICH9 southbridge.
   See: https://wiki.qemu.org/Features/Q35

So 'pc' is more a 'family' of different machines.

You can verify the codebase, the abstract parent is in hw/i386/pc.c:

static const TypeInfo pc_machine_info = {
     .name = TYPE_PC_MACHINE,
     .parent = TYPE_X86_MACHINE,
     .abstract = true,
     .instance_size = sizeof(PCMachineState),

While the children implementations are hw/i386/pc_piix.c and 
hw/i386/pc_q35.c (pc_piix.c implements both isapc + i440fx).

I'm not sure why one is named by its southbridge chipset, and
the other one by its northbridge. I suppose because for a long
time the i440fx+piix were mixed in the same file. I cleaned that
recently in commits 0fd61a2d1..0f25d865a1:

$  git log --oneline --reverse 0fd61a2d1~..0f25d865a1
0fd61a2d1c hw/pci-host/piix: Move i440FX declarations to 
hw/pci-host/i440fx.h
553b4559dc hw/pci-host/piix: Fix code style issues
14a026dd58 hw/pci-host/piix: Extract PIIX3 functions to hw/isa/piix3.c
0f25d865a1 hw/pci-host: Rename incorrectly named 'piix' as 'i440fx'

>>> +        """
>>> +        :avocado: tags=machine:pc
>>> +        """
>>> +        self.launch_and_wait()
>>> +
>>> +    def test_kvm_pc(self):
>>
>> This test_pc_i440fx_kvm
>>
> 
> Ditto.
> 
>>> +        """
>>> +        :avocado: tags=machine:pc
>>> +        :avocado: tags=accel:kvm
>>> +        """
>>> +        if not kvm_available(self.arch, self.qemu_bin):
>>> +            self.cancel(KVM_NOT_AVAILABLE)
>>> +        self.vm.add_args("-accel", "kvm")
>>> +        self.launch_and_wait()
>>> +
>>> +    def test_q35(self):
>>
>> This one test_pc_q35_tcg
>>
> 
> Both *pc* and *q35*?  I'm missing something here.  +1 for the explicit
> "tcg" along with an also explicit check for the availability of the
> tcg accel.
> 
>>> +        """
>>> +        :avocado: tags=machine:q35
>>> +        """
>>> +        self.launch_and_wait()
>>> +
>>> +    def test_kvm_q35(self):
>>
>> Here test_pc_q35_kvm.
>>
> 
> I don't get the "pc" part here.
> 
>>> +        """
>>> +        :avocado: tags=machine:q35
>>> +        :avocado: tags=accel:kvm
>>> +        """
>>> +        if not kvm_available(self.arch, self.qemu_bin):
>>> +            self.cancel(KVM_NOT_AVAILABLE)
>>> +        self.vm.add_args("-accel", "kvm")
>>> +        self.launch_and_wait()
>>> +
>>> +
>>> +class BootLinuxAarch64(BootLinux):
>>> +    """
>>> +    :avocado: tags=arch:aarch64
>>> +    :avocado: tags=machine:virt
>>> +    """
>>> +
>>> +    chksum = '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'
>>> +
>>> +    def test_virt(self):
>>
>> We have other 'virt' machines:
>>
>> $ git grep '"virt"'
>> hw/arm/virt.c:83:            mc->alias = "virt"; \
>> hw/riscv/virt.c:613:    .name       = MACHINE_TYPE_NAME("virt"),
>> hw/xtensa/virt.c:135:DEFINE_MACHINE("virt", xtensa_virt_machine_init)
>>
>> Maybe rename test_aarch64_virt_tcg?
>>
> 
> The "test name", or "test ID" includes the class name, so that already
> makes it clear (IMO) that this test is about the "virt" machine type
> for the "aarch64" arch.

OK, good point. 'tcg' suffix still?

>>> +        self.vm.add_args('-cpu', 'cortex-a53')
>>> +        self.vm.add_args('-bios',
>>> +                         os.path.join(BLD_DIR, 'pc-bios',
>>> +                                      'edk2-aarch64-code.fd'))
>>> +        self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
>>> +        self.vm.add_args('-object', 'rng-random,id=rng0,filename=/dev/urandom')
>>> +        self.launch_and_wait()
>>> +
>>> +    def test_kvm_virt(self):
>>> +        """
>>> +        :avocado: tags=accel:kvm
>>> +        """
>>> +        if not kvm_available(self.arch, self.qemu_bin):
>>> +            self.cancel(KVM_NOT_AVAILABLE)
>>> +        self.vm.add_args("-accel", "kvm")
>>> +        self.test_virt()
>>> +
>>> +
>>> +class BootLinuxPPC64(BootLinux):
>>> +    """
>>> +    :avocado: tags=arch:ppc64
>>> +    """
>>> +
>>> +    chksum = '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'
>>> +
>>> +    def test_pseries(self):
>>
>> Rename as test_ppc64el_pseries_tcg?
>>
> 
> Same here (class name contains the arch).

Yes. 'tcg' suffix still?

>>> +        """
>>> +        :avocado: tags=machine:pseries
>>> +        """
>>> +        self.launch_and_wait()
>>> +
>>> +
>>> +class BootLinuxS390X(BootLinux):
>>> +    """
>>> +    :avocado: tags=arch:s390x
>>> +    """
>>> +
>>> +    chksum = '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'
>>> +
>>> +    def test_s390_ccw_virtio(self):
>>> +        """
>>> +        :avocado: tags=machine:s390-ccw-virtio
>>> +        """
>>> +        self.launch_and_wait()
>>> diff --git a/tests/requirements.txt b/tests/requirements.txt
>>> index a2a587223a..0192c352cd 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
>>> +avocado-framework==73.0
>>> +pycdlib==1.8.0
>>>