[libvirt] Xen device section defaults miss name='qemu'
Hi, we have found an issue caused by 321a28c6 "libxl: set default disk format in device post-parse" (and maybe more changes I couldn't identify yet). TL;DR: The auto-added driver section in xen changed and now causes issues. An example to to trigger: 1. Create a XEN xml with a disk device of type cdrom, but do not add a driver section, like. <disk type='file' device='cdrom'> <target dev='hdb' bus='ide'/> <readonly/> </disk> 2. on virsh define if that XML 2b. You could also "virsh edit" any working xml file, remove the <driver../> which will trigger the same 2c. You could also define via virt-manager as it does not explicitly specify the device section What happens is that before the changes this auto-added a driver section like: <driver name='qemu' type='raw'/> But now it does only add <driver type='raw'/> Which fails to verify like: Interestingly the same is not true for KVM, there the section is as it was in the past which made it more likely the post-parse step might be involved. Also rather expected, if one adds a full <driver name='qemu' type='raw'/> in the XML libvirt doesn't have to provide (broken) defaults and things work as they should - yet from a user with formerly working XMLs or using virt-manager this is a regression. I have beg your pardon I seem to not be experienced enough on this part of libvirt to provide a valid fix - I tried but nothing good came out so far. For now I reverted 321a28c6 which also affects a test later added by 4cd3f241. With those changes it will not add anything which lets us start Xen VMs for now, but it clearly isn't a solution. I wanted to ask you if you could take a look into this? -- Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd
Not sure how stupid it might be so clearly just a very humble RFC, but the following seems to work for me: Therefore no nicely polished patch, but just inline diff --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -367,8 +367,9 @@ int actual_type = virStorageSourceGetActualType(disk->src); int format = virDomainDiskGetFormat(disk); - /* for network-based disks, set 'qemu' as the default driver */ - if (actual_type == VIR_STORAGE_TYPE_NETWORK) { + /* for network-based disk and cdrom, set 'qemu' as the default driver */ + if (actual_type == VIR_STORAGE_TYPE_NETWORK || + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (!virDomainDiskGetDriver(disk) && virDomainDiskSetDriver(disk, "qemu") < 0) return -1; Opinions? If it seems remotely reasonable I'm totally fine submitting a patch in more style with proper headers and such.
On 07/12/2017 09:35 AM, Christian Ehrhardt wrote:
Not sure how stupid it might be so clearly just a very humble RFC, but the following seems to work for me: Therefore no nicely polished patch, but just inline diff
--- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -367,8 +367,9 @@ int actual_type = virStorageSourceGetActualType(disk->src); int format = virDomainDiskGetFormat(disk); - /* for network-based disks, set 'qemu' as the default driver */ - if (actual_type == VIR_STORAGE_TYPE_NETWORK) { + /* for network-based disk and cdrom, set 'qemu' as the default driver */ + if (actual_type == VIR_STORAGE_TYPE_NETWORK || + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (!virDomainDiskGetDriver(disk) && virDomainDiskSetDriver(disk, "qemu") < 0) return -1;
This might be useful regardless of the answer to my question about the disk/driver/@name attribute. AFAIK, the only backend in Xen that supports CDROM is qemu.
Opinions? If it seems remotely reasonable I'm totally fine submitting a patch in more style with proper headers and such.
Unless my statement above is incorrect, I think submitting a formal patch would be fine. Regards, Jim
In recent libvirt libxl does fill non-existant driver elements with type='raw', but lacks the name='qemu' attribute that it had in the past. Without that the xml fails to validate, therefore old XML files can't be edited/defined anymore. Since the cdrom device type only works with the qemu driver, add that as the default attribute just as it was added in the past. Example of the verification error: $ virt-xml-validate mytest.xml Relax-NG validity error : Extra element devices in interleave Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/libxl/libxl_domain.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 68a501c..70ddcea 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -367,8 +367,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, int actual_type = virStorageSourceGetActualType(disk->src); int format = virDomainDiskGetFormat(disk); - /* for network-based disks, set 'qemu' as the default driver */ - if (actual_type == VIR_STORAGE_TYPE_NETWORK) { + /* for network-based disk and cdrom, set 'qemu' as the default driver */ + if (actual_type == VIR_STORAGE_TYPE_NETWORK || + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (!virDomainDiskGetDriver(disk) && virDomainDiskSetDriver(disk, "qemu") < 0) return -1; -- 2.7.4
On 07/17/2017 10:42 AM, Christian Ehrhardt wrote:
In recent libvirt libxl does fill non-existant driver elements with type='raw', but lacks the name='qemu' attribute that it had in the past. Without that the xml fails to validate, therefore old XML files can't be edited/defined anymore.
Since the cdrom device type only works with the qemu driver, add that as the default attribute just as it was added in the past.
Example of the verification error: $ virt-xml-validate mytest.xml Relax-NG validity error : Extra element devices in interleave
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/libxl/libxl_domain.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 68a501c..70ddcea 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -367,8 +367,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, int actual_type = virStorageSourceGetActualType(disk->src); int format = virDomainDiskGetFormat(disk);
- /* for network-based disks, set 'qemu' as the default driver */ - if (actual_type == VIR_STORAGE_TYPE_NETWORK) { + /* for network-based disk and cdrom, set 'qemu' as the default driver */ + if (actual_type == VIR_STORAGE_TYPE_NETWORK || + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (!virDomainDiskGetDriver(disk) && virDomainDiskSetDriver(disk, "qemu") < 0) return -1;
So is it the case that libxl never uses this attribute, and it's just being added to the config in order to pass RNG validation? If so, then my opinion would be that the RNG is too strict, and that attribute should be made optional.
On 07/18/2017 11:10 AM, Laine Stump wrote:
On 07/17/2017 10:42 AM, Christian Ehrhardt wrote:
In recent libvirt libxl does fill non-existant driver elements with type='raw', but lacks the name='qemu' attribute that it had in the past. Without that the xml fails to validate, therefore old XML files can't be edited/defined anymore.
Since the cdrom device type only works with the qemu driver, add that as the default attribute just as it was added in the past.
Example of the verification error: $ virt-xml-validate mytest.xml Relax-NG validity error : Extra element devices in interleave
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/libxl/libxl_domain.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 68a501c..70ddcea 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -367,8 +367,9 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, int actual_type = virStorageSourceGetActualType(disk->src); int format = virDomainDiskGetFormat(disk);
- /* for network-based disks, set 'qemu' as the default driver */ - if (actual_type == VIR_STORAGE_TYPE_NETWORK) { + /* for network-based disk and cdrom, set 'qemu' as the default driver */ + if (actual_type == VIR_STORAGE_TYPE_NETWORK || + disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { if (!virDomainDiskGetDriver(disk) && virDomainDiskSetDriver(disk, "qemu") < 0) return -1;
So is it the case that libxl never uses this attribute, and it's just being added to the config in order to pass RNG validation? If so, then my opinion would be that the RNG is too strict, and that attribute should be made optional.
Yep, agreed. I'd rather relax the RNG than restrict CDROM devices to the qemu backend. Although IIRC qemu is currently the only usable backend wrt eject and other CDROM-specific operations, blkback should be able to read them just fine. And perhaps another CDROM backend will appear in Xen in the future. Instead of forcing the qemu backend here, I've sent a patch to relax the RNG https://www.redhat.com/archives/libvir-list/2017-July/msg00670.html Regards, Jim
On 07/11/2017 08:15 AM, Christian Ehrhardt wrote:
Hi,
Hi, Thanks for the report and sorry for delayed response. I'm slowly catching up on libvirt mail after being away for a while...
we have found an issue caused by 321a28c6 "libxl: set default disk format in device post-parse" (and maybe more changes I couldn't identify yet).
TL;DR: The auto-added driver section in xen changed and now causes issues.
An example to to trigger: 1. Create a XEN xml with a disk device of type cdrom, but do not add a driver section, like. <disk type='file' device='cdrom'> <target dev='hdb' bus='ide'/> <readonly/> </disk> 2. on virsh define if that XML 2b. You could also "virsh edit" any working xml file, remove the <driver../> which will trigger the same 2c. You could also define via virt-manager as it does not explicitly specify the device section
What happens is that before the changes this auto-added a driver section like: <driver name='qemu' type='raw'/> But now it does only add <driver type='raw'/> Which fails to verify like:
I think you forgot to paste the example here, but I assume it is something like # virt-xml-validate test.xml Relax-NG validity error : Extra element devices in interleave test.xml:21: element devices: Relax-NG validity error : Element domain failed to validate content test.xml fails to validate where test.xml contains disk config <disk type='file' device='cdrom'> <driver type='raw'/> <target dev='hdb' bus='ide'/> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> It is not clear to me if 'name' is a required attribute of <disk>. The doc says "If the hypervisor supports multiple backend drivers, then the name attribute selects the primary backend driver name". It doesn't mention "required" or "mandatory". We'd have a schema bug if 'name' is optional. I guess it is a good question for danpb. Dan, is disk/driver/@name a required attribute? Regards, Jim
On Sat, Jul 15, 2017 at 12:27 AM, Jim Fehlig <jfehlig@suse.com> wrote:
On 07/11/2017 08:15 AM, Christian Ehrhardt wrote:
What happens is that before the changes this auto-added a driver section like: <driver name='qemu' type='raw'/> But now it does only add <driver type='raw'/> Which fails to verify like:
I think you forgot to paste the example here, but I assume it is something like
# virt-xml-validate test.xml Relax-NG validity error : Extra element devices in interleave test.xml:21: element devices: Relax-NG validity error : Element domain failed to validate content test.xml fails to validate
Yes that was the case I've seen. where test.xml contains disk config
<disk type='file' device='cdrom'> <driver type='raw'/> <target dev='hdb' bus='ide'/> <readonly/> <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk>
As your example or even no <driver> at all - that in the past also got the defaults and no isn't.
It is not clear to me if 'name' is a required attribute of <disk>. The doc says "If the hypervisor supports multiple backend drivers, then the name attribute selects the primary backend driver name". It doesn't mention "required" or "mandatory". We'd have a schema bug if 'name' is optional. I guess it is a good question for danpb. Dan, is disk/driver/@name a required attribute?
Prepping my patch, but surely if the fix is to just relax the relaxng to not have it required I'm good as well.
participants (3)
-
Christian Ehrhardt -
Jim Fehlig -
Laine Stump