[libvirt] [RFC PATCH 0/3] Implement mockup capabilities cache in QEMU tests

[Martin Kletzander]

On Tue, Sep 01, 2015 at 05:19:39PM -0400, Cole Robinson wrote:
>On 08/18/2015 05:40 AM, Pavel Fedin wrote:
>> Since commit e8d55172544c1fafe31a9e09346bdebca4f0d6f9 qemu driver checks
>> emulator capabilities during domain XML post-parse. However, test suite
>> does not initialize it, therefore a condition to skip all checks if there
>> is no cache supplied was added. This is actually a hack, whose sole
>> purpose is to make existing test suite working. Additionally, it prevents
>> from writing new tests for this particular functionality.
>>
>> This series attempts to solve this problem by implementing proper cache
>> mockup in test suite. The main idea is to create a cache in standard way
>> and put there a pre-defined capabilities set (which tests already have).
>>
>> The main problem here is to know emulator binary name, which is contained
>> in the source XML. However, we have to create our cache before reading the
>> XML. The simplest way to resolve this is to assume particular binary name
>> from test name. Currently tests which assume cross-architecture binary are
>> all prefixed with the architecture name (with one exception of "keywrap"
>> tests which all assume /usr/bin/qemu-system-s390x and do not have "s390-"
>> prefix in their name).
>>
>> This scheme works fine, unless we use "native" emulator binary. Here we
>> have a mess. Most newer tests use /usr/bin/qemu, however there is a large
>> number of tests which use /usr/libexec/qemu-kvm or /usr/bin/kvm (i guess
>> these are leftovers from the epoch when qemu-kvm was a separate fork of
>> qemu). This is currently not handled in any way, and these tests may
>> report errors due to missing binaries (because virQEMUCapsCacheLookup()
>> attempts to populate the cache automatically by querying the binary if
>> not already known).
>>
>> There are several possible ways to resolve this:
>> a) Add all possible names as aliases for /usr/bin/qemu
>> b) Forbid to use oldstyle names at all in these tests
>> c) Declare some prefix like "kvm-" for those tests who want to use
>>    /usr/libexec/qemu-kvm. Again, this would ban /usr/bin/kvm and
>>    /usr/bin/qemu-kvm (if not using aliases like in (b)
>> d) Hardcode (optional) emulator name per test. IMHO a bad idea because
>>    number of tests is huge.
>> e) Do some preparsing of the XML and extract binary name from it. Again,
>>    i disliked it for not being simple enough.
>>
>
>With the current approach I think e) is the way to go... yeah it's redundant
>parsing but better than trying to maintain a whitelist or reformat a bunch of
>test cases to match this. And for example these patches as is cause breakage
>in other tests, like qemuargv2xml, and I think it's related to the incomplete
>whitelist.
>
>Using libvirt helpers it shouldn't be too hard to parse out the emulator path,
>basically just virXMLParseStringCtxt and then
>
>emulator = virXPathString("string(./devices/emulator[1])", ctxt);
>
>Maybe other people have better ideas about how to mock this out though.
>
>> I also thought about an alternate implementation which would patch
>> postParseCallback and insert own function there which builds a cache. At
>> this point binary name is already known from the XML. However, such a
>> design looks like an ugly  hack by itself, so i stopped going in this
>> direction.
>>
>
>Yeah that sounds quite intense. Maybe there's some way to override a function
>with our own definition from the test suite, but that's outside my realm of
>knowledge.
>

The current xmlopt structure has two callbacks in parser
configuration, one for post-parsing of the whole domain XML and one
for device XML.  We could add new (optional) callback in this
structure that would be called before other post-parse callbacks, but
still after the xml is parsed.  This would not be utilized anywhere
else than in tests, currently, but I don't see it as ugly and it might
be beneficial e.g. for some drivers or in the future.  Another benefit
is that it does not add that much of a code as it would be called only
in virDomainDefPostParse() and virDomainDeviceDefParse(), plus the
time complexity can be kept at minimum if the function called is no-op
in case the cache is already built.

>Actually populating the cache does exercise the most qemu code though, so
>that's one benefit.
>
>Another minor bit to consider is possibly adding an assert in
>qemu_capabilities.c to ensure that we never try to actually probe the host FS
>from the test suite, since we don't want the test suite to accidentally become
>dependent on host state. Maybe there's a test suite environment variable we
>can check for.
>

We should be really careful about not depending on the host in tests,
so any assert like this would be much appreciated.

>- Cole
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150902/51f49c0e/attachment-0001.sig>