Dan Kenigsberg has uploaded a new change for review.
Change subject: libvirtvm: allow acpi again ......................................................................
libvirtvm: allow acpi again
Commit c3e2272f has mistakenly changed
<features> <acpi/> </features>
into
<features> acpi </features>
which caused VMs to start up silently with no acpi support. This patch reverts the offending part of the above commit.
Bug initially reported by Itzik Brown.
Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Signed-off-by: Dan Kenigsberg danken@redhat.com --- M vdsm/libvirtvm.py 1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/33/7533/1
diff --git a/vdsm/libvirtvm.py b/vdsm/libvirtvm.py index a975206..c29a1a9 100644 --- a/vdsm/libvirtvm.py +++ b/vdsm/libvirtvm.py @@ -726,7 +726,8 @@ <features/> """ if utils.tobool(self.conf.get('acpiEnable', 'true')): - self.appendChildWithText('features', 'acpi') + self.dom.appendChild(self.doc.createElement('features')) \ + .appendChild(self.doc.createElement('acpi'))
def appendCpu(self): """
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: libvirtvm: allow acpi again ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/718/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: libvirtvm: allow acpi again ......................................................................
Patch Set 1: Looks good to me, but someone else must approve
This bug is introduced in my refactor patch. I open the gerrit review page of that patch, and find that in the test result, there is no ACPI in the generated XML. This is my fault, I should check the generated XML is exactly the same as the one generated by the old code.
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: libvirtvm: allow acpi again ......................................................................
Patch Set 1:
Maybe we could add a unit test to check these kind of error. I can give it a try.
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has posted comments on this change.
Change subject: libvirtvm: allow acpi again ......................................................................
Patch Set 1:
Zhou, it is also the fault of the reviewer...
Please verify this patch, too.
Yes, a unit test for the generated domxml may be a good idea, which is pretty hard to implement without re-implementing libvirtvm within the test framework. I have no idea how to design a robust and complete test like that.
BTW, it could have been nice if libvirt's domxml verification had warned about the freetext withing <features>.
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Zhou Zheng Sheng has posted comments on this change.
Change subject: libvirtvm: allow acpi again ......................................................................
Patch Set 1: Verified
Run through ./autobuild.sh, then I cd to ~/builder and verify the output XML as follow.
[root@zhshzhouf17 builder]# PYTHONPATH=$(pwd)/share/vdsm:$(pwd)/lib64/python2.7/site-packages:/usr/lib64/python2.7/site-packages python Python 2.7.3 (default, Jul 24 2012, 10:05:38) [GCC 4.7.0 20120507 (Red Hat 4.7.0-5)] on linux2 Type "help", "copyright", "credits" or "license" for more information.
from libvirtvm import _DomXML as DomXML d = {'acpiEnable': 'True', 'smp': '2', 'memSize': 128, 'vmName': 'rhel6vdsm', 'vmId': '209b27e4-aed3-11e1-a547-00247edb4743'} domxml = DomXML(d, None) domxml.appendFeatures() print domxml.toxml()
<?xml version="1.0" encoding="utf-8"?> <domain type="kvm"> <name>rhel6vdsm</name> <uuid>209b27e4-aed3-11e1-a547-00247edb4743</uuid> <memory>131072</memory> <currentMemory>131072</currentMemory> <vcpu>2</vcpu> <devices/> <features> <acpi/> </features> </domain>
From the above conversation we can see, we have to invoke appendFeatures() to let domxml generate the acpi sub node. In my refactor commit which introduced the bug, I examined the output XML as well but forgot to call appendFeatures() to verify the <acpi> node, so I did not realize the acpi node was not generated properly. This patch fix the problem.
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Igor Lvovsky has posted comments on this change.
Change subject: libvirtvm: allow acpi again ......................................................................
Patch Set 1: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
Dan Kenigsberg has submitted this change and it was merged.
Change subject: libvirtvm: allow acpi again ......................................................................
libvirtvm: allow acpi again
Commit c3e2272f has mistakenly changed
<features> <acpi/> </features>
into
<features> acpi </features>
which caused VMs to start up silently with no acpi support. This patch reverts the offending part of the above commit.
Bug initially reported by Itzik Brown.
Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Signed-off-by: Dan Kenigsberg danken@redhat.com --- M vdsm/libvirtvm.py 1 file changed, 2 insertions(+), 1 deletion(-)
Approvals: Dan Kenigsberg: Igor Lvovsky: Looks good to me, approved Zhou Zheng Sheng: Verified; Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/7533 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I591c40678f030f1bcd4cb7cb42c21907420f7cf1 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Itzik Brown itzikb@mellanox.com Gerrit-Reviewer: Meni Yakove myakove@redhat.com Gerrit-Reviewer: Zhou Zheng Sheng zhshzhou@linux.vnet.ibm.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org