[PATCH] Issue #526 (partial)
This patch partially fixes the issue #526. There's still a known bug which happens when the user reverts a VM to a snapshot when the VM's name has been changed since the snapshot was taken. In other words, the following behavior will still cause a bug: - Create a VM named "foo". - Create a snapshot of VM "foo" named "snap1". - Rename VM "foo" to "bar". - Revert VM "bar" to snapshot "snap1". The last step works correctly under the hood (i.e. the VM is reverted to that snapshot) but Kimchi cannot return from its REST request because the VM changed its name during the revert action. Zongmei Gou will send another patch which fixes the bug described above as the patch is already done. Crístian Viana (1): issue #526: Support updating name for VMs with snapshots src/kimchi/model/vms.py | 56 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/test_model.py | 3 +++ 2 files changed, 57 insertions(+), 2 deletions(-) -- 2.1.0
In order to update the name, the corresponding VM must be removed and then added again. And a VM cannot be removed if it has snapshots. So, a VM with snapshots fails to have its name changed. Before removing a VM, a backup of the existing snapshots will be taken so they can be recreated when the VM is added again. Fix issue #526 ("Cannot edit VM with snapshots"). --- src/kimchi/model/vms.py | 56 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/test_model.py | 3 +++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 3aa1145..463c350 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -649,6 +649,39 @@ class VMModel(object): libvirt.VIR_DOMAIN_AFFECT_LIVE) return xml + def _backup_snapshots(self, snap, all_info): + """ Append "snap" and the children of "snap" to the list "all_info". + + The list *must* always contain the parent snapshots before their + children so the function "_redefine_snapshots" can work correctly. + + Arguments: + snap -- a native domain snapshot. + all_info -- a list of dict keys: + "{'xml': <snap XML>, 'current': <is snap current?>'}" + """ + all_info.append({'xml': snap.getXMLDesc(0), + 'current': snap.isCurrent(0)}) + + for child in snap.listAllChildren(0): + self._backup_snapshots(child, all_info) + + def _redefine_snapshots(self, dom, all_info): + """ Restore the snapshots stored in "all_info" to the domain "dom". + + Arguments: + dom -- the domain which will have its snapshots restored. + all_info -- a list of dict keys, as described in "_backup_snapshots", + containing the original snapshot information. + """ + for info in all_info: + flags = libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE + + if info['current']: + flags |= libvirt.VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT + + dom.snapshotCreateXML(info['xml'], flags) + def _static_vm_update(self, dom, params): state = DOM_STATE_MAP[dom.info()[0]] old_xml = new_xml = dom.XMLDesc(0) @@ -667,13 +700,26 @@ class VMModel(object): if 'graphics' in params: new_xml = self._update_graphics(dom, new_xml, params) + snapshots_info = [] + vm_name = dom.name() conn = self.conn.get() try: if 'name' in params: if state == 'running': - msg_args = {'name': dom.name(), 'new_name': params['name']} + msg_args = {'name': vm_name, 'new_name': params['name']} raise InvalidParameter("KCHVM0003E", msg_args) + lflags = libvirt.VIR_DOMAIN_SNAPSHOT_LIST_ROOTS + dflags = (libvirt.VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + libvirt.VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) + + for virt_snap in dom.listAllSnapshots(lflags): + snapshots_info.append({'xml': virt_snap.getXMLDesc(0), + 'current': virt_snap.isCurrent(0)}) + self._backup_snapshots(virt_snap, snapshots_info) + + virt_snap.delete(dflags) + # Undefine old vm, only if name is going to change dom.undefine() @@ -681,10 +727,16 @@ class VMModel(object): currentMem = root.find('.currentMemory') if currentMem is not None: root.remove(currentMem) + dom = conn.defineXML(ET.tostring(root, encoding="utf-8")) + if 'name' in params: + self._redefine_snapshots(dom, snapshots_info) except libvirt.libvirtError as e: dom = conn.defineXML(old_xml) - raise OperationFailed("KCHVM0008E", {'name': dom.name(), + if 'name' in params: + self._redefine_snapshots(dom, snapshots_info) + + raise OperationFailed("KCHVM0008E", {'name': vm_name, 'err': e.get_error_message()}) return dom diff --git a/tests/test_model.py b/tests/test_model.py index 0820386..b77545c 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -983,6 +983,9 @@ class ModelTests(unittest.TestCase): vms = inst.vms_get_list() self.assertTrue('kimchi-vm1' in vms) + # make sure "vm_update" works when the domain has a snapshot + inst.vmsnapshots_create(u'kimchi-vm1') + # update vm graphics when vm is not running inst.vm_update(u'kimchi-vm1', {"graphics": {"passwd": "123456"}}) -- 2.1.0
Applied. Thanks. Regards, Aline Manera
I need to apply the framework patch prior to apply this one. So once Zongmei Gou's patch get accepted I will apply it too. On 13/01/2015 18:07, Crístian Viana wrote:
This patch partially fixes the issue #526. There's still a known bug which happens when the user reverts a VM to a snapshot when the VM's name has been changed since the snapshot was taken. In other words, the following behavior will still cause a bug:
- Create a VM named "foo". - Create a snapshot of VM "foo" named "snap1". - Rename VM "foo" to "bar". - Revert VM "bar" to snapshot "snap1".
The last step works correctly under the hood (i.e. the VM is reverted to that snapshot) but Kimchi cannot return from its REST request because the VM changed its name during the revert action.
Zongmei Gou will send another patch which fixes the bug described above as the patch is already done.
Crístian Viana (1): issue #526: Support updating name for VMs with snapshots
src/kimchi/model/vms.py | 56 +++++++++++++++++++++++++++++++++++++++++++++++-- tests/test_model.py | 3 +++ 2 files changed, 57 insertions(+), 2 deletions(-)
participants (2)
-
Aline Manera -
Crístian Viana