qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 0/5] Add "boot_linux" acceptance test


From: Wainer dos Santos Moschetta
Subject: Re: [Qemu-devel] [PATCH v5 0/5] Add "boot_linux" acceptance test
Date: Thu, 14 Mar 2019 14:36:49 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2


On 03/13/2019 05:46 PM, Cleber Rosa wrote:
This adds an acceptance test that validates that a full blown Linux
guest can successfully boot in QEMU.

Changes from v4:
================

  * New commit "Acceptance tests: use relative location for tests"

  * New commit "Acceptance tests: keep a stable reference to the QEMU build dir"

  * Pinned the Fedora 29 image by adding a checksum.  The goal is to
    never allow more than one component to change at a time (the one
    allowed to change is QEMU itself).  Updates to the image should be
    manual. (Based on comments from Cornelia)

  * Moved the downloading of the Fedora 29 cloud image to the test
    setUp() method, canceling the test if the image can not be
    downloaded.

  * Removed the ":avocado: enable" tag, given that Avocado versions
    68.0 and later operate on a "recursive by default" manner, that
    is able to correctly identify this as an Avocado test.

Changes from v3:
================

  * New patch "Acceptance tests: depend on qemu-img"

Known Issues on v3 (no longer applicable):
==========================================

  * A recent TCG performance regression[1] affects this test in a
    number of ways:
    - The test execution may timeout by itself
    - The generation of SSH host keys in the guest's first boot is also
      affected (possibly also a timeout)
    - The cloud-init "phone home" feature attempts to read the host keys
      and fails, causing the test to timeout and fail

    These are not observed anymore once the fix[2] is applied.

[1] - https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg00338.html
[2] - https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01129.html

Changes from v2:
================

  * Updated the tag to include the "arch:" key, in a similar fashion as to
    the tests in the "Acceptance Tests: target architecture support":
    - https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg00369.html

  * Renamed the test method name to test_x86_64_pc, again, similarly to the
    boot_linux_console.py tests in the series mentioned before.

  * Set the machine type explicitly, again similarly to the
    boot_linux_console.py tests in the series mentioned before.

  * Added messages after the launch of the VM, to let test runners know
    the test know waits for a boot confirmation from the the guest (Eduardo).

  * Updated commit message to reflect the fact that this version does
    not allow for parameterization of the guest OS, version, etc.

  * Dropped the RFC prefix on patch "RFC: Acceptance tests: add the
    build directory to the system PATH"

  * Changed the comments on "RFC: Acceptance tests: add the build
    directory to the system PATH" to make it clear the addition of a
    the build directory to the PATH may influence other utility code.

Changes from v1:
================

  * The commit message was adjusted, removing the reference to the
    avocado.utils.vmimage encoding issue on previous Avocado versions
    (<= 64.0) and the fix that would (and was) included in Avocado
    version 65.0.

  * Effectively added pycdlib==1.6.0 to the requirements.txt file,
    added on a56931eef3, and adjusted the commit message was also
    to reflect that.

  * Updated the default version of the guest OS, from Fedora 28 to 29.
    Besides possible improvements in the (virtual) hardware coverage,
    it brings a performance improvement in the order of 20% to the
    test.

  * Removed all direct parameters usage.  Because some parameters and
    its default values implemented in the test would prevent it from
    running on some environments.  Example: the "accel" parameter had a
    default value of "kvm", which would prevent this test, that boots a
    x86_64 OS, from running on a host arch different than x86_64.  I
    recognize that it's desirable to make tests reusable and
    parameterized (that was the reason for the first version doing so),
    but the mechanism to be used to define the architectures that a
    given test should support is still an open issue, and has been
    discussed in other threads.  I'll follow up those discussions with
    a proposal, and until then, removing those aspects from this test
    implementation seemed to be the best option.  A caveat: this test
    currently adds the same tag (x86_64) and follows other assumptions
    made on "boot_linux_console.py", that is, that a x86_64 target
    binary will be used to run it.  If a user is in an environment that
    does not have a x86_64 target binary, it could filter those tests
    out with: "avocado run --filter-by-tags='-x86_64' tests/acceptance".

  * Removed most arguments to the QEMU command line for pretty much the
    same reasons described above, and by following the general
    perception that I could grasp from other discussions that QEMU
    defaults should preferrably be used.  This test, as well as others,
    can and should be extended later to allow for different test
    scenarios by passing well documented parameter values.  That is,
    they should respect well-known parameters such as "accel" mentioned
    above, so that the same test can run with KVM or TCG.

  * Changed the value of the memory argument to 1024, which based on
    my experimentations and observations is the minimum amount of RAM
    for the Fedora 29 cloud image to sucessfully boot on QEMU.  I know
    there's no such thing as a "one size fits all", specially for QEMU,
    but this makes me wonder wether a x86_64 machine type shouldn't
    have its default_ram_size bumped to a number practical enough to
    run modern operating systems.

  * Added a new patch "RFC: Acceptance tests: add the build directory
    to the system PATH", which is supposed to gather feedback on how to
    enable the use of built binaries, such as qemu-img, to code used by
    the test code.  The specific situation here is that the vmimage,
    part of the avocado.utils libraries, makes use of qemu-img to create
    snapshot files.  Even though we could require qemu-img to be installed
    as a dependency of tests, system wide, it actually goes against the
    goal of testing all QEMU things from the source/build tree.  This
    became aparent with tests running on environments such as Travis CI,
    which don't necessarily have qemu-img available elsewhere.

Some hopefully useful pointers for reviewers:
=============================================

Git Info:
   - URI: https://github.com/clebergnu/qemu/tree/sent/test_boot_linux_v5
   - Remote: https://github.com/clebergnu/qemu
   - Branch: sent/test_boot_linux_v5

Travis CI Info:
   - Build: https://travis-ci.org/clebergnu/qemu/builds/505970935

Previous version:
   - v4: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg02032.html
   - v3: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg01677.html
   - v2: https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg04318.html
   - v1: http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg02530.html

Cleber Rosa (5):
   Acceptance tests: use relative location for tests
   Acceptance tests: keep a stable reference to the QEMU build dir
   Acceptance tests: add the build directory to the system PATH
   Acceptance tests: depend on qemu-img
   Add "boot_linux" acceptance test for x86_64 and pc machine type

  tests/Makefile.include                    |  4 +-
  tests/acceptance/avocado_qemu/__init__.py |  8 +++-
  tests/acceptance/boot_linux.py            | 58 +++++++++++++++++++++++
  tests/requirements.txt                    |  1 +
  4 files changed, 68 insertions(+), 3 deletions(-)
  create mode 100644 tests/acceptance/boot_linux.py


The make check-acceptance now finished with success in my system, that does not have qemu-img installed at system-wide (fixed by patches 03 and 04).
Also the code looks good to me. So:

Reviewed-by: Wainer dos Santos Moschetta <address@hidden>





reply via email to

[Prev in Thread] Current Thread [Next in Thread]