Bug 1159219
| Summary: | [RFE] Update-device support update startupPolicy option to domain XML | ||
|---|---|---|---|
| Product: | Red Hat Enterprise Linux 7 | Reporter: | Pei Zhang <pzhang> |
| Component: | libvirt | Assignee: | Michal Privoznik <mprivozn> |
| Status: | CLOSED ERRATA | QA Contact: | Virtualization Bugs <virt-bugs> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | 7.1 | CC: | dyuan, jdenemar, lmiksik, mprivozn, mzhan, rbalakri, xuzhang |
| Target Milestone: | rc | Keywords: | FutureFeature, Upstream |
| Target Release: | --- | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | libvirt-1.3.1-1.el7 | Doc Type: | Enhancement |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2016-11-03 18:11:01 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
I've just pushed the patch upstream:
commit 11e058ca589808bd25bf1e15f0acbd4ec517c271
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Oct 28 19:50:43 2014 +0100
Commit: Michal Privoznik <mprivozn>
CommitDate: Wed Nov 5 18:34:08 2014 +0100
qemuDomainUpdateDeviceConfig: Allow startupPolicy update
https://bugzilla.redhat.com/show_bug.cgi?id=1159219
Users might want to update startupPolicy via the
virDomainUpdateDeviceFlags API too. This patch
implements the feature on config layer.
Signed-off-by: Michal Privoznik <mprivozn>
v1.2.10-20-g11e058c
Hi Michal,
I found perhaps this function is not working well as expect. Could you help to check if I missing something or this bug need further modified.
Thanks a lot in advance.
Verified version :
libvirt-1.2.17-8.el7.x86_64
qemu-kvm-rhev-2.3.0-22.el7.x86_64
steps:
1.define a guest like following with startupPolicy :
# virsh dumpxml r72 | grep disk -A 9
......
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw' cache='none'/>
<source file='/var/lib/libvirt/images/b.iso' startupPolicy='mandatory'/>
<target dev='hda' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
......
2. prepare a disk xml with different source file.
# cat up-cd.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw' cache='none'/>
<source file='/var/lib/libvirt/images/a.iso' startupPolicy='optional'/>
<backingStore/>
<target dev='hda' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
3. do update-device
# virsh update-device r72 up-cd.xml --config
Device updated successfully
4.check again, the source file has been changed , but the startupPolicy has not.
# virsh dumpxml r72 --inactive | grep disk -A 9
......
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw' cache='none'/>
<source file='/var/lib/libvirt/images/a.iso' startupPolicy='mandatory'/>
<target dev='hda' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0' target='0' unit='0'/>
</disk>
......
Yes, it's broken again. While I committed the patch in Nov 2014, later, when Peter was reworking the code around the lines I fixed, he accidentally removed my contribution:
commit d0dc6c036914da4905c3f483be3036014a38fe9b
Author: Peter Krempa <pkrempa>
AuthorDate: Thu Mar 12 17:12:12 2015 +0100
Commit: Peter Krempa <pkrempa>
CommitDate: Tue Mar 17 17:11:37 2015 +0100
qemu: driver: Fix cold-update of removable storage devices
Only selected fields from the disk source were copied when cold updating
source in a CDROM drive. When such drive was backed by a network file
this resulted into corruption of the definition:
<disk type='network' device='cdrom'>
<driver name='qemu' type='raw' cache='none'/>
<source protocol='gluster' name='gluster-vol1(null)'>
<host name='localhost'/>
</source>
<target dev='vdc' bus='virtio'/>
<readonly/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/>
</disk>
Update the whole source instead of cherry-picking elements.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1166024
However, in order to obey the RHEL process, this must go back to Assigned.
And I've just pushed the patches upstream:
commit 0751786d3ce3e0f7a8c198b3bf188a3ec2c67ab0
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Sep 15 17:13:01 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Mon Sep 21 07:57:55 2015 +0200
qemuDomainChangeDiskLive: Allow startupPolicy change
https://bugzilla.redhat.com/show_bug.cgi?id=1159219
Signed-off-by: Michal Privoznik <mprivozn>
commit 08573b632da1000c5ecf4deb7c9d52181bb84558
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Sep 15 15:41:18 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Fri Sep 18 15:51:19 2015 +0200
qemuDomainDiskChangeSupported: Fill in missing checks
So far this function was not kept in sync with changing
virDomainDiskDef. Fill in all the missing checks and reorganize
their order so it's easier to track which items are not being
checked for.
Signed-off-by: Michal Privoznik <mprivozn>
commit 127328a07e5f3e69eee73e771beac05533d4c2ab
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Sep 15 15:25:18 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Fri Sep 18 15:51:18 2015 +0200
qemu: s/virDomainDiskDiffersSourceOnly/qemuDomainDiskChangeSupported/
I always felt like this function is qemu specific rather than
libvirt-wide. Other drivers may act differently on virDomainDef
change and in fact may require talking to underlying hypervisor
even if something else's than disk->src has changed. I know that
the function is still incomplete, but lets break that into two
commits that are easier to review. This one is pure code
movement.
Signed-off-by: Michal Privoznik <mprivozn>
commit c08475824be3d506614efd26d6354270baea10e3
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Sep 15 14:37:21 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Fri Sep 18 15:51:18 2015 +0200
qemuDomainChangeDiskLive: rework slightly
Firstly, our coding guidelines suggest using 'cleanup' label
instead of 'end'. Then, @ret should be set to value representing
success as the last statement before the 'cleanup' label.
And while I am at this function, lets enumerate all the possible
enum items (virDomainDiskDevice) and avoid using 'default' in
switch(). Pooh. Also, nothing bad happens if we look up the disk
to change in the domain upfront. In fact, it's going to be
helpful later when we want to keep some old values for performing
a rollback.
Signed-off-by: Michal Privoznik <mprivozn>
commit cb2ed632e6c5b67e06722c79849cce05cb079950
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Sep 15 13:41:42 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Fri Sep 18 15:51:18 2015 +0200
qemu_domain: Introduce qemuDomainDiskSourceDiffers
This new private API should return true iff sources of two disks
differs in sense that qemu should be instructed to change the
disk backend. For instance, ejecting a CDROM is such case, or
pointing disk into a different ISO location, and so on.
Signed-off-by: Michal Privoznik <mprivozn>
commit 8fca346c97eda62cff51db7f06b1c455af2e9a18
Author: Michal Privoznik <mprivozn>
AuthorDate: Tue Sep 15 13:16:24 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Fri Sep 18 15:51:18 2015 +0200
qemu: s/qemuDomainChangeDiskMediaLive/qemuDomainChangeDiskLive/
While we currently only allow changing a media in a disk, this is
going to change in a while, so the function name would be
invalid. Moreover, the old name does not match the pattern laid
out by other update functions.
Signed-off-by: Michal Privoznik <mprivozn>
commit 9af84477202c22ee0919ba07fbd51a0f54ae1674
Author: Michal Privoznik <mprivozn>
AuthorDate: Mon Sep 14 12:59:53 2015 +0200
Commit: Michal Privoznik <mprivozn>
CommitDate: Fri Sep 18 15:51:18 2015 +0200
qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again
https://bugzilla.redhat.com/show_bug.cgi?id=1159219
So, in 11e058ca589808bd I've tried to make UpdateDevice update
startupPolicy too. And it worked well until somebody came around
and pushed d0dc6c036914da which accidentally removed my
contribution. Redo my commit.
Signed-off-by: Michal Privoznik <mprivozn>
v1.2.19-81-g0751786
Verify version :
libvirt-1.3.4-1.el7.x86_64
Verify steps:
Test with three different kinds of startupPolicy(mandatory/requisite/optional). Following steps just list one kind of them.
1.For a running guest, test update source of cdrom and startupPolicy with different values.
1.1 keep source of value and change startupPolicy value.
# virsh dumpxml r72|grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso'>
......
</disk>
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='optional'>
......
</disk>
# virsh update-device r72 cdrom.xml
Device updated successfully
# virsh dumpxml r72|grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='optional'>
......
</disk>
2. change both source of cdrom and startupPolicy value.
# virsh dumpxml r72|grep cdrom -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='mandatory'>
2.1 prepare xml to change source and startupPolicy
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='optional'>
......
</disk>
# virsh update-device r72 cdrom.xml --persistent
Device updated successfully
# virsh destroy r72;virsh start r72
Domain r72 destroyed
Domain r72 started
# virsh dumpxml r72|grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='optional'>
......
</disk>
Also test with other options.
3. delete startupPolicy element from xml
# virsh dumpxml r72 |grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='mandatory'>
......
</disk>
3.1 prepare xml without startupPolicy.
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso'>
......
</disk>
# virsh update-device r72 cdrom.xml
Device updated successfully
# virsh dumpxml r72 |grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso'>
......
</disk>
4.update with non-exist source
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/c.iso' startupPolicy='optional'>
......
4.1 change with --persistent
# virsh update-device r72 cdrom.xml --persistent
error: Failed to update device from cdrom.xml
error: Cannot access storage file '/mnt/at-dt/change-media/c.iso' (as uid:1002, gid:1002): No such file or directory
4.2 check nothing was changed.
# virsh dumpxml r72|grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso'>
......
4.3 change with --config and start
# virsh update-device r72 cdrom.xml --config
Device updated successfully
# virsh destroy r72;virsh start r72
Domain r72 destroyed
Domain r72 started
4.4 check again, startupPolicy='optional' works well.
# virsh dumpxml r72 |grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source startupPolicy='optional'>
......
5. For shutoff guest :
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='requisite'>
......
</disk>
# virsh update-device r72 cdrom.xml --config
Device updated successfully
# virsh start r72
Domain r72 started
# virsh dumpxml r72 |grep cdrom -A 10
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='requisite'>
......
</disk>
Now we can update startupPolicy into domain xml.
Hi Michal,
I met another question about startupPolicy='requisite' during my testing.
Can you help check if it needs a fix. Thanks a lot in advance!
It says that when startupPolicy='requisite' it will drop source if missing on restore.
Now it will drop the source file, but keep startupPolicy='requisite' element in <source/>,
then this cdrom cannot be changed with other operations.
Details as following :
# virsh dumpxml r72|grep cdrom -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='requisite'>
......
</disk>
move the media away and save guest.
#mv a.iso a.iso.bak
# virsh save r72 r72.save
Domain r72 saved to r72.save
# virsh restore r72.save
Domain restored from r72.save
# virsh dumpxml r72|grep cdrom -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source startupPolicy='requisite'>
......
</disk>
Should we delete all <source/> here since startupPolicy='requisite'?
Now, it cannot update cdrom device and report unknow error.
1. update a new device directly.
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso'>
</source>
......
</disk>
# virsh update-device r72 cdrom.xml
error: Failed to update device from cdrom.xml
error: internal error: unable to execute QEMU command 'eject': Tray of device 'drive-ide0-0-0' is not open
# virsh update-device r72 cdrom.xml
error: Failed to update device from cdrom.xml
error: An error occurred, but the cause is unknown
2. update startupPolicy to 'optional', then try to update device
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source startupPolicy='optional'>
......
# virsh update-device r72 cdrom.xml
Device updated successfully
# virsh dumpxml r72|grep cdrom -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source startupPolicy='optional'>
# cat cdrom.xml
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='//mnt/at-dt/change-media/b.iso' startupPolicy='optional'>
......
# virsh update-device r72 cdrom.xml
error: Failed to update device from cdrom.xml
error: An error occurred, but the cause is unknown
(In reply to Pei Zhang from comment #13) > Hi Michal, > I met another question about startupPolicy='requisite' during my testing. > Can you help check if it needs a fix. Thanks a lot in advance! There are two problems here. The first one is that libvirt returns an error but sets no error message. For that there's already a patch upstream, I just reviewed it: https://www.redhat.com/archives/libvir-list/2016-May/msg01220.html And it's already pushed in as v1.3.4-323-g1fad65d. But Cole finds out as well, there's a bug in qemu somewhere, that when libvirt instructs qemu to open tray, qemu opens it without emitting corresponding event and thus leaving libvirt hanging in the air waiting for that event. Libvirt timeouts after a while and claims that whole operation failed. I think we should file a bug against qemu for this. Verified version :
libvirt-2.0.0-5.el7.x86_64
qemu-kvm-rhev-2.6.0-21.el7.x86_64
steps:
1. For a shutoff guest, using update-device to change startupPolocy
# virsh dumpxml r73 | grep disk -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='mandatory'/>
<target dev='hdc' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
# virsh update-device r73 cdrom.xml --config
Device updated successfully
# virsh dumpxml r73 | grep disk -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='requisite'/>
<target dev='hdc' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
# virsh update-device r73 cdrom.xml --config
Device updated successfully
# virsh dumpxml r73 | grep disk -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='optional'/>
<target dev='hdc' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
2. For a running guest, using update-device to change startupPolicy
# virsh list
Id Name State
----------------------------------------------------
12 r73 running
# virsh dumpxml r73 | grep disk -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='optional'>
<seclabel model='selinux' labelskip='yes'/>
</source>
<backingStore/>
<target dev='hdc' bus='ide'/>
<readonly/>
<alias name='ide0-1-0'/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
# virsh update-device r73 cdrom.xml --persistent
Device updated successfully
# virsh dumpxml r73 | grep disk -A 9
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/a.iso' startupPolicy='mandatory'>
<seclabel model='selinux' labelskip='yes'/>
</source>
<backingStore/>
<target dev='hdc' bus='ide'/>
<readonly/>
<alias name='ide0-1-0'/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
# virsh update-device r73 cdrom.xml --persistent
Device updated successfully
# virsh dumpxml r73 | grep disk -A 9
<suspend-to-disk enabled='no'/>
</pm>
<devices>
<emulator>/usr/libexec/qemu-kvm</emulator>
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='requisite'>
<seclabel model='selinux' labelskip='yes'/>
</source>
<backingStore/>
<target dev='hdc' bus='ide'/>
<readonly/>
<alias name='ide0-1-0'/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
# virsh update-device r73 cdrom.xml --persistent
Device updated successfully
# virsh dumpxml r73 | grep disk -A 9
<suspend-to-disk enabled='no'/>
</pm>
<devices>
<emulator>/usr/libexec/qemu-kvm</emulator>
<disk type='file' device='cdrom'>
<driver name='qemu' type='raw'/>
<source file='/mnt/at-dt/change-media/b.iso' startupPolicy='optional'>
<seclabel model='selinux' labelskip='yes'/>
</source>
<backingStore/>
<target dev='hdc' bus='ide'/>
<readonly/>
<alias name='ide0-1-0'/>
<address type='drive' controller='0' bus='1' target='0' unit='0'/>
</disk>
3. using an non-exist iso, it should report error.
# virsh update-device r73 cdrom.xml --persistent
error: Failed to update device from cdrom.xml
error: Cannot access storage file '/mnt/at-dt/change-media/c.iso' (as uid:1002, gid:1002): No such file or directory
According to steps in comment 12 and this comment, move this bug to verified.
File another bug to track issue in comment 13
https://bugzilla.redhat.com/show_bug.cgi?id=1368368
Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://rhn.redhat.com/errata/RHSA-2016-2577.html |
Description of problem: StartupPolicy option should be update to domain XML via using update-device. Version-Release number of selected component (if applicable): libvirt-1.2.8-5.el7.x86_64 qemu-kvm-rhev-2.1.2-4.el7.x86_64 kernel-3.10.0-191.el7.x86_64 How reproducible: 100% Steps to Reproduce: 1. try to add startupPolicy option using update-device 1.1> cdrom.xml with startupPolicy option [root@184 images]# cat cdrom.xml <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/cdrom.iso' startupPolicy='requisite'/> ----> startupPolicy option <target dev='hdc' bus='ide'/> <readonly/> </disk> 1.2>.update-device [root@184 images]# virsh update-device r7new cdrom.xml Device updated successfully 1.3> dumpxml to check the startupPolicy option was not be updated. [root@184 images]# virsh dumpxml r7new | grep cdrom -A 6 <disk type='file' device='cdrom'> <driver name='qemu' type='raw' cache='none'/> <source file='/var/lib/libvirt/images/cdrom.iso'/> ------> startupPolicy option will miss <backingStore/> <target dev='hdc' bus='ide'/> <readonly/> <alias name='ide0-1-0'/> <address type='drive' controller='0' bus='1' target='0' unit='0'/> </disk> 2.try to change startupPolicy using update-device 2.1>in domain XML with device startupPolicy=requisite #virsh dumpxml r7new | grep cdrom -A 7 <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source startupPolicy='requisite'/> <backingStore/> <target dev='hdb' bus='ide'/> <readonly/> <alias name='ide0-0-1'/> <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> 2.2> try to change the startupPolicy from requisite to optional [root@184 images]# cat cdrom.xml <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/cdrom.iso' startupPolicy='optional'/> <target dev='hdb' bus='ide'/> <readonly/> </disk> 2.3> check domain XML after update-device successfully , startupPolicy was not changed. <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/cdrom.iso' startupPolicy='requisite'/> <backingStore/> <target dev='hdb' bus='ide'/> <readonly/> <alias name='ide0-0-1'/> <address type='drive' controller='0' bus='0' target='0' unit='1'/> </disk> Actual results: As step1 and step2 , startupPolicy option can not be updated to domain XML via using update-device Expected results: StartupPolicy option should be updated to domain XML via using update-device.