[libvirt] [PATCH v2 3/7] tests: qemuxml2argv: Add infrastructure for testing with real qemuCaps

[Peter Krempa]

On Wed, Apr 18, 2018 at 14:43:55 +0200, Andrea Bolognani wrote:
> On Wed, 2018-04-18 at 13:44 +0200, Peter Krempa wrote:
> > On Wed, Apr 18, 2018 at 13:09:52 +0200, Ján Tomko wrote:
> > > Even introducting new capabilities could cause libvirt to take different
> > > code paths, therefore we should prefer DO_TEST_CAPS_VER.
> > 
> > This is actually the exact reason why I think DO_TEST_CAPS_NEW is better
> > than DO_TEST_CAPS_VER. If we do changes to code used in multiple places
> > which are gated on presence of a capability, the places using
> > DO_TEST_CAPS_VER could change without us being able to catch that.
> 
> A note on naming: both the macro and the output file should use the
> world "latest" rather than "new" to be more informative and avoid
> any confusion.

Oh right. I knew there is a better word for this but could not figure
it out ;)

> 
> > > I do not find DO_TEST_CAPS_NEW that beneficial - if QEMU introduced a
> > > new capability that needs to be handled by libvirt, the code author
> > > should introduce a new DO_TEST_CAPS_VER test for that.
> > 
> > In such reason, the author adding the code should fork the test by
> > adding a DO_TEST_CAPS_VER along with the existing DO_TEST_CAPS_NEW and
> > then add the new capability bit. With such approach it will be even
> > visible which options changed.
> > 
> > > Otherwise it would just be checking whether QEMU did not break something
> > > for us. And since the soonest we update capabilities is at the time of
> > 
> > Actually no. It will also check that something other will not break:
> > 
> > https://www.redhat.com/archives/libvir-list/2018-April/msg01066.html
> > tried to introduce a new property for the memory-backing-file object.
> > This object is used in multiple places. The property is gated by a
> > capability bit. Our tests were not able to catch, that this would add
> > the property to the NVDIMM device which needs to persist the data, which
> > would actually break it.
> > 
> > While I agree that DO_TEST_CAPS_VER is very useful for checking old
> > command line, I think that the true value is also in DO_TEST_CAPS_NEW
> > when used together with DO_TEST_CAPS_VER, so when a new addition would
> > change some tests using DO_TEST_CAPS_NEW we should fork them to cover
> > both recent and old versions.
> 
> I think using real-life capabilities instead of the syntetic data
> we use now is a step in the right direction.
> 
> I'm still a bit uneasy about the "moving target" factor, though
> I've spent some time trying to come up with possible failure
> scenarios without much success...

There are few churn-scenarios, some avoidable, some not so much:

- When adding something which is added for a lot of configurations or all
  of them (e.g. recent adding of the new seccomp code) the change will
  generate a lot of changes.

- Machine type. We can't use any of the alias machine types in the XML
  files as they will change when new capabilities with new machine types
  are added. This can easily be avoided by using an explicit machine
  type.

While the above may induce some churn in some scenarios, I think that
the benefit of at least seeing that something changed in places where
you did not expect any change may outweigh it.

> I think ideally with each new feature the author would introduce
> three tests: a negative one locked to a QEMU version that doesn't
> have the required bits, a positive one locked to a QEMU version
> that has them, and a positive one against the latest QEMU version.

I'm considering whether it's necessary to have the one locked to the
older-but-supporting version of qemu.

My idea was that if somebody were to add test which would change
anything, the test can be forked prior to that. But it seems that it may
be a better idea to lock-in the changes right away.

> This way, we would make sure both that new QEMU versions work
> correctly with the feature and that changes in libvirt don't
> affect the behavior for existing QEMU versions.
> 
> Does that sound reasonable?

Yes. I've actually modified the test code added in this series to do
this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180418/aec5abe8/attachment-0001.sig>