Re: [RFC 0/3] qtest: pick tests that require KVM at runtime

[Philippe Mathieu-Daudé]

+Daniel (sorry Daniel)

On 6/22/21 12:36 PM, Igor Mammedov wrote:
> On Tue, 22 Jun 2021 10:22:19 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>> On 6/22/21 10:07 AM, Alex Bennée wrote:
>>> Igor Mammedov <imammedo@redhat.com> writes:  
>>>> On Fri, 18 Jun 2021 14:43:46 +0200
>>>> Claudio Fontana <cfontana@suse.de> wrote:  
>>>>> On 6/18/21 1:26 PM, Igor Mammedov wrote:  
>>>>>> On Thu, 17 Jun 2021 18:49:17 +0200
>>>>>> Claudio Fontana <cfontana@suse.de> wrote:  
>>>>>>> On 6/16/21 5:24 PM, Igor Mammedov wrote:    
>>>>>>>>
>>>>>>>> Sometimes it's necessary to execute a test that depends on KVM,
>>>>>>>> however qtest is not aware if tested QEMU binary supports KVM
>>>>>>>> on the host it the test is executed.      
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> It seems to me that we are constantly re-implementing the same feature 
>>>>>>> with slight variations?
>>>>>>>
>>>>>>> Didn't we have a generic series to introduce qtest_has_accel() from 
>>>>>>> Philippe before?    
>>>>>> It's mentioned in cover letter (PS: part) and in [1/3] with rationale
>>>>>> why this was posted.    
>>>>>
>>>>> Thought it was separate, but now I see that it uses query-accel 
>>>>> underneath.
>>>>>
>>>>> Seems strange to add another check to do the same thing, it may point to 
>>>>> qtest_has_accel() needing some update?
>>>>> You mention it is time consuming to use qtest_has_accel(), have you 
>>>>> measured an important overhead?
>>>>> With qtest_has_accel() not even being committed yet, is it already 
>>>>> necessary to work around it because it's too slow?   
>>>>
>>>> Tests are already take a lot of time as is, so I'd try to avoid slowing
>>>> them down.
>>>>
>>>> proposed qtest_has_accel() requires spawning QEMU to probe, which is slow.
>>>> Worst case would be:
>>>>  = qemu startup time * number of checks * number of targets
>>>>
>>>> It's fine to run occasionally, I can take a coffee break while tests run.
>>>> But put it in context of CI and it multiplies by the number of push 
>>>> requests
>>>> and starts to eat not only time but also limited CI resources.
>>>>
>>>> In current form qtest_has_accel() is only marginally better functionality
>>>> wise, as it reports all built in accelerators while qtest_has_kvm() 
>>>> accounts
>>>> only for KVM.
>>>>
>>>> qtest_has_kvm() is collecting info about built-in accelerators at
>>>> configure/build time and that probably could be extended to other
>>>> accelerators (not a thing that I'm interested in at the moment).
>>>> So it could be extended to support the same accelerators
>>>> as currently proposed qtest_has_accel().  
>>>
>>> One minor downside is this forever ties the tests to the build. I have
>>> spoken with people before about the idea of separating the test
>>> artefacts from the build so they can be used either as a) cached test
>>> objects or b) other testing environments, for example verifying the
>>> kernel has not regressed. However we don't do either of those things at
>>> the moment so it's not a major concern.  
>>
>> This is the feature that is interesting RedHat QE too, run the latest
>> qtests on various released binaries to compare performances between
>> releases.
> 
> Currently qtest is only a build only and hard tied to concrete release tests.
> And it's usually mercilessly altered to fit any QEMU interface changes
> new version brings, which in turn breaks idea of testing other QEMU versions
> with it.
> 
> I'd guess it would take *a lot* of effort to make and maintain it
> as external test harness for various QEMU versions.

I had the understanding the outcome of the last 6 months discussions
was "do not tie testing with built binary, we have a machine protocol
to introspect the binary" and "tests should focus on the feature to
test, let's remove the build system burden".

> I think build time qtest used as public CI and external test suite
> are conflicting goals, for the former we probably minimize used
> compute resources while the later is probably run by QA in a more
> controlled manner (unless one integrates that into another CI)

I think we are mixing orthogonal topics here:

* CI time is a constraint, no doubt, we don't want to waste it.

* Tests being buildsys-agnostic

If CI can't run all our tests, it is certainly not a reason for
not adding more tests... We should be able to add as many tests
as we want to the repository. Then we should select which tests
we want to run, because we can't run them all.

Otherwise let's declare no more tests can be added in tests/qtest/
because our CI time is already full ;)

> If I needed something that were to be usable as external test suite,
> I'd look towards acceptance tests which are less depended on QEMU
> internals and versions.
>  
>>> If the worry is about extending test times by having an extra round trip
>>> of a spawn and query step for every test could we not consider caching
>>> the information somewhere? Really any given binary should only need to
>>> be queried once per run, not per test.  
>>
>> Good idea.
> 
> Yep, it should be less taxing, than the currently posted qtest_has_accel()
> version.
> It will reduce worst complexity little bit to
>    probbing_time * #test_binaries * number_targets * #push_req (all_forks)
> but that's still explosive path.