[PATCH 0/2] More snapshot bugfixes
Crístian Viana (2): Only allow VM snapshots to be taken on 'qcow2' disks Revert "snapshot: Clone snapshots when cloning a VM" src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 29 +---------------------------- src/kimchi/model/vmsnapshots.py | 17 +++++++++++++++-- tests/test_model.py | 11 ----------- tests/test_rest.py | 19 ++----------------- 5 files changed, 19 insertions(+), 58 deletions(-) -- 1.9.3
VM snapshot support in libvirt may not work correctly with some image types, like 'raw'. To make sure snapshots work correctly, only allow them to be taken on VMs with 'qcow2' images, which is the most supported image type for that operation. Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 1 + src/kimchi/model/vmsnapshots.py | 17 +++++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 58d1f36..e43fe9c 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -319,6 +319,7 @@ messages = { "KCHSNAP0006E": _("Unable to delete snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"), "KCHSNAP0008E": _("Unable to retrieve current snapshot on virtual machine '%(vm)s'. Details: %(err)s"), "KCHSNAP0009E": _("Unable to revert virtual machine '%(vm)s' to snapshot '%(name)s'. Details: %(err)s"), + "KCHSNAP0010E": _("Unable to create snapshot on virtual machine '%(vm)s' because it contains a disk with format '%(format)s'; only 'qcow2' is supported."), "KCHCPUINF0001E": _("The number of vCPUs is too large for this system."), "KCHCPUINF0002E": _("Invalid vCPU/topology combination."), diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py index 0da9dc3..bb9bfaa 100644 --- a/src/kimchi/model/vmsnapshots.py +++ b/src/kimchi/model/vmsnapshots.py @@ -27,6 +27,7 @@ from lxml.builder import E from kimchi.exception import InvalidOperation, NotFoundError, OperationFailed from kimchi.model.tasks import TaskModel from kimchi.model.vms import DOM_STATE_MAP, VMModel +from kimchi.model.vmstorages import VMStorageModel, VMStoragesModel from kimchi.utils import add_task @@ -35,12 +36,14 @@ class VMSnapshotsModel(object): self.conn = kargs['conn'] self.objstore = kargs['objstore'] self.task = TaskModel(**kargs) + self.vmstorages = VMStoragesModel(**kargs) + self.vmstorage = VMStorageModel(**kargs) def create(self, vm_name, params={}): """Create a snapshot with the current domain state. - The VM must be stopped before creating a snapshot on it; otherwise, an - exception will be raised. + The VM must be stopped and contain only disks with format 'qcow2'; + otherwise an exception will be raised. Parameters: vm_name -- the name of the VM where the snapshot will be created. @@ -55,6 +58,16 @@ class VMSnapshotsModel(object): if DOM_STATE_MAP[vir_dom.info()[0]] != u'shutoff': raise InvalidOperation('KCHSNAP0001E', {'vm': vm_name}) + # if the VM has a non-CDROM disk with type 'raw', abort. + for storage_name in self.vmstorages.get_list(vm_name): + storage = self.vmstorage.lookup(vm_name, storage_name) + type = storage['type'] + format = storage['format'] + + if type != u'cdrom' and format != u'qcow2': + raise InvalidOperation('KCHSNAP0010E', {'vm': vm_name, + 'format': format}) + name = params.get('name', unicode(int(time.time()))) task_params = {'vm_name': vm_name, 'name': name} -- 1.9.3
This reverts commit c087f65f12af7394230c300ca63e3fd79fe65498. Conflicts: src/kimchi/mockmodel.py --- src/kimchi/model/vms.py | 29 +---------------------------- tests/test_model.py | 11 ----------- tests/test_rest.py | 19 ++----------------- 3 files changed, 3 insertions(+), 56 deletions(-) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 4b112fe..3aa1145 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -374,15 +374,10 @@ class VMModel(object): # create new guest cb('defining new VM') try: - vir_new_dom = vir_conn.defineXML(xml) + vir_conn.defineXML(xml) except libvirt.libvirtError, e: raise OperationFailed('KCHVM0035E', {'name': name, 'err': e.message}) - rollback.prependDefer(vir_new_dom.undefine) - - # copy snapshots - cb('copying VM snapshots') - self._clone_copy_snapshots(name, new_name) rollback.commitAll() @@ -546,28 +541,6 @@ class VMModel(object): # remove the new object store entry should an error occur later rollback.prependDefer(_rollback_objstore) - def _clone_copy_snapshots(self, vm_name, new_vm_name): - dom = self.get_vm(new_vm_name, self.conn) - flags = libvirt.VIR_DOMAIN_XML_SECURE - - # libvirt's Test driver does not support the function - # "virDomainListAllSnapshots", so "VMSnapshots.get_list" will raise - # "OperationFailed" in that case. - try: - snapshot_names = self.vmsnapshots.get_list(vm_name) - except OperationFailed, e: - kimchi_log.error('cannot list snapshots: %s; ' - 'skipping snapshot cloning...' % e.message) - else: - for s in snapshot_names: - vir_snap = self.vmsnapshot.get_vmsnapshot(vm_name, s) - try: - snap_xml = vir_snap.getXMLDesc(flags).decode('utf-8') - dom.snapshotCreateXML(snap_xml) - except libvirt.libvirtError, e: - raise OperationFailed('KCHVM0035E', {'name': vm_name, - 'err': e.message}) - def _build_access_elem(self, users, groups): auth = config.get("authentication", "method") auth_elem = E.auth(type=auth) diff --git a/tests/test_model.py b/tests/test_model.py index 1355295..319408b 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -161,17 +161,6 @@ class ModelTests(unittest.TestCase): self.assertEquals(sorted([params['name'], snap_name], key=unicode.lower), snaps) - # Clone the VM and check whether the snapshots have been cloned - # as well - task = inst.vm_clone(u'kimchi-vm') - clone_name = task['target_uri'].split('/')[-1] - rollback.prependDefer(inst.vm_delete, clone_name) - inst.task_wait(task['id']) - task = inst.task_lookup(task['id']) - self.assertEquals('finished', task['status']) - clone_snaps = inst.vmsnapshots_get_list(clone_name) - self.assertEquals(snaps, clone_snaps) - snap = inst.vmsnapshot_lookup(u'kimchi-vm', snap_name) current_snap = inst.currentvmsnapshot_lookup(u'kimchi-vm') self.assertEquals(snap, current_snap) diff --git a/tests/test_rest.py b/tests/test_rest.py index 2e5cff1..fa15ef6 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -428,8 +428,8 @@ class RestTests(unittest.TestCase): resp = self.request('/vms/test-vm/snapshots', '{}', 'GET') self.assertEquals(200, resp.status) - orig_snaps = json.loads(resp.read()) - self.assertEquals(2, len(orig_snaps)) + snaps = json.loads(resp.read()) + self.assertEquals(2, len(snaps)) # Look up current snapshot (the one created above) resp = self.request('/vms/test-vm/snapshots/current', '{}', 'GET') @@ -451,21 +451,6 @@ class RestTests(unittest.TestCase): current_snap = json.loads(resp.read()) self.assertEquals(snap, current_snap) - # Clone a VM with snapshot - resp = self.request('/vms/test-vm/clone', '{}', 'POST') - self.assertEquals(202, resp.status) - task = json.loads(resp.read()) - wait_task(self._task_lookup, task['id']) - resp = self.request('/tasks/%s' % task['id'], '{}', 'GET') - self.assertEquals(200, resp.status) - task = json.loads(resp.read()) - self.assertEquals('finished', task['status']) - vm_name = task['target_uri'].split('/')[-1] - resp = self.request('/vms/%s/snapshots' % vm_name, '{}', 'GET') - self.assertEquals(200, resp.status) - clone_snaps = json.loads(resp.read()) - self.assertEquals(orig_snaps, clone_snaps) - # Delete a snapshot resp = self.request('/vms/test-vm/snapshots/%s' % params['name'], '{}', 'DELETE') -- 1.9.3
participants (2)
-
Aline Manera -
Crístian Viana