[libvirt] New libvirt-tck failure with libvirt 5.2.0

[Daniel P. Berrangé]

On Wed, Apr 10, 2019 at 01:33:11PM -0600, Jim Fehlig wrote:
> On 4/10/19 1:17 PM, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 4/10/19 3:33 PM, Jim Fehlig wrote:
> > > On 4/10/19 12:25 PM, Daniel Henrique Barboza wrote:
> > > > 
> > > > 
> > > > On 4/10/19 3:08 PM, Jim Fehlig wrote:
> > > > > I noticed libvirt-tck test domain/207-disk-media-change.t
> > > > > started failing after updating to libvirt 5.2.0. A bisection
> > > > > fingered commit f1d65853
> > > > > 
> > > > > commit f1d6585300001c7b23b8796a0faa4411c3531996
> > > > > Author: Daniel Henrique Barboza <danielhb413 at gmail.com>
> > > > > Date:   Fri Mar 15 18:06:45 2019 -0300
> > > > > 
> > > > >     domain_conf: check device address before attach
> > > > > 
> > > > > This commit prevents a media change on CDROM devices.
> > > > 
> > > > Thanks for letting us know. I'll run libvirt-tck (any pointers are appreciated -
> > > > first time I've heard about this test suite) and see if I can get a fix going.
> > > 
> > > It's easy to reproduce outside of libvirt-tck. Simply start a VM
> > > with a cdrom device and then try to update the <source> element with
> > > 'virsh attach-device ...'. E.g. with the VM running try to "eject"
> > > the cdrom
> > > 
> > > # cat update-empty-cdrom.xml
> > > <disk type='file' device='cdrom'>
> > >   <target dev='hdc'/>
> > > </disk>
> > > 
> > > # virsh attach-device test update-empty-cdrom.xml
> > > error: Failed to attach device from update-empty-cdrom.xml
> > > error: Requested operation is not valid: Domain already contains a
> > > device with the same address
> > 
> > 
> > To give a background on this patch, this was intended to avoid a situation
> > where a hotplug of a device with the same ID would fall back to a cleanup
> > code that ended up unplugging the previous existing device in the guest.
> > This was happening when using the QEMU driver.
> 
> Understood. And fixing that bug is a good thing!
> 
> > What I didn't expect is for a CD-ROM media to be changed using a
> > attach-device command. I think that the CD-ROM type has specific rules of not
> > hot-unplugging itself (i.e. the driver) in the situation I described above. What
> > I was expecting for CDROM change media was a VIR_DOMAIN_DEVICE_ACTION_UPDATE
> > action instead, so this usage completely went under the radar for that
> > patch.
> 
> I don't particularly like overloading the meaning of attach-device in this
> way, but prohibiting it may be an unexpected change in behavior for some
> user. However if folks agree with the change we can simply adjust the test.

We must not break API back-compatibility in this way, so must fix the
regression. 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|