[PATCH 0/3] Snapshot enhancements
Crístian Deives (3): snapshot: Handle non-existing snapshots in mock lookup snapshot: Change the action 'revert' to a Task snapshot: Allow creating a snapshot of a domain in any state src/kimchi/control/vm/snapshots.py | 2 +- src/kimchi/i18n.py | 1 - src/kimchi/mockmodel.py | 14 ++++++++++++++ src/kimchi/model/vmsnapshots.py | 36 ++++++++++++++++++++++-------------- tests/test_model.py | 15 +++++++++++---- tests/test_rest.py | 13 +++++++++++++ 6 files changed, 61 insertions(+), 20 deletions(-) -- 2.1.0
The function "_mock_vmsnapshot_lookup" doesn't handle the case when the provided snapshot name doesn't exist. In that case, it returns None. Fix the function mentioned above so when the provided snapshot name doesn't exist, a proper exception is raised. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/mockmodel.py | 3 +++ tests/test_rest.py | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 413ac5d..dcb8bc1 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -31,6 +31,7 @@ from lxml.builder import E from kimchi import config from kimchi import imageinfo from kimchi import osinfo +from kimchi.exception import NotFoundError from kimchi.model.debugreports import DebugReportsModel from kimchi.model.host import DeviceModel from kimchi.model.libvirtstoragepool import IscsiPoolDef, NetfsPoolDef @@ -419,6 +420,8 @@ class MockModel(Model): if sn.name == name: return sn.info + raise NotFoundError('KCHSNAP0003E', {'name': name, 'vm': vm_name}) + def _mock_vmsnapshot_delete(self, vm_name, name): snapshots = MockModel._mock_snapshots.get(vm_name, []) for sn in snapshots: diff --git a/tests/test_rest.py b/tests/test_rest.py index ee350b2..c1b81e8 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -334,6 +334,10 @@ class RestTests(unittest.TestCase): task = json.loads(self.request('/tasks/%s' % task['id']).read()) self.assertEquals('finished', task['status']) + # Look up a non-existing snapshot + resp = self.request('/vms/test-vm/snapshots/snap404', '{}', 'GET') + self.assertEquals(404, resp.status) + # Look up a snapshot resp = self.request('/vms/test-vm/snapshots/%s' % params['name'], '{}', 'GET') -- 2.1.0
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 20/04/2015 15:58, Crístian Deives wrote:
The function "_mock_vmsnapshot_lookup" doesn't handle the case when the provided snapshot name doesn't exist. In that case, it returns None.
Fix the function mentioned above so when the provided snapshot name doesn't exist, a proper exception is raised.
Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/mockmodel.py | 3 +++ tests/test_rest.py | 4 ++++ 2 files changed, 7 insertions(+)
diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 413ac5d..dcb8bc1 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -31,6 +31,7 @@ from lxml.builder import E from kimchi import config from kimchi import imageinfo from kimchi import osinfo +from kimchi.exception import NotFoundError from kimchi.model.debugreports import DebugReportsModel from kimchi.model.host import DeviceModel from kimchi.model.libvirtstoragepool import IscsiPoolDef, NetfsPoolDef @@ -419,6 +420,8 @@ class MockModel(Model): if sn.name == name: return sn.info
+ raise NotFoundError('KCHSNAP0003E', {'name': name, 'vm': vm_name}) + def _mock_vmsnapshot_delete(self, vm_name, name): snapshots = MockModel._mock_snapshots.get(vm_name, []) for sn in snapshots: diff --git a/tests/test_rest.py b/tests/test_rest.py index ee350b2..c1b81e8 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -334,6 +334,10 @@ class RestTests(unittest.TestCase): task = json.loads(self.request('/tasks/%s' % task['id']).read()) self.assertEquals('finished', task['status'])
+ # Look up a non-existing snapshot + resp = self.request('/vms/test-vm/snapshots/snap404', '{}', 'GET') + self.assertEquals(404, resp.status) + # Look up a snapshot resp = self.request('/vms/test-vm/snapshots/%s' % params['name'], '{}', 'GET')
I will apply only this patch. As the other ones need to have the UI updated to be merged. On 20/04/2015 15:58, Crístian Deives wrote:
The function "_mock_vmsnapshot_lookup" doesn't handle the case when the provided snapshot name doesn't exist. In that case, it returns None.
Fix the function mentioned above so when the provided snapshot name doesn't exist, a proper exception is raised.
Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/mockmodel.py | 3 +++ tests/test_rest.py | 4 ++++ 2 files changed, 7 insertions(+)
diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 413ac5d..dcb8bc1 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -31,6 +31,7 @@ from lxml.builder import E from kimchi import config from kimchi import imageinfo from kimchi import osinfo +from kimchi.exception import NotFoundError from kimchi.model.debugreports import DebugReportsModel from kimchi.model.host import DeviceModel from kimchi.model.libvirtstoragepool import IscsiPoolDef, NetfsPoolDef @@ -419,6 +420,8 @@ class MockModel(Model): if sn.name == name: return sn.info
+ raise NotFoundError('KCHSNAP0003E', {'name': name, 'vm': vm_name}) + def _mock_vmsnapshot_delete(self, vm_name, name): snapshots = MockModel._mock_snapshots.get(vm_name, []) for sn in snapshots: diff --git a/tests/test_rest.py b/tests/test_rest.py index ee350b2..c1b81e8 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -334,6 +334,10 @@ class RestTests(unittest.TestCase): task = json.loads(self.request('/tasks/%s' % task['id']).read()) self.assertEquals('finished', task['status'])
+ # Look up a non-existing snapshot + resp = self.request('/vms/test-vm/snapshots/snap404', '{}', 'GET') + self.assertEquals(404, resp.status) + # Look up a snapshot resp = self.request('/vms/test-vm/snapshots/%s' % params['name'], '{}', 'GET')
Applied. Thanks. Regards, Aline Manera
The action 'revert' is currently synchronous; that means that there's no way for the user to run it in background and check its status periodically. In some cases, reverting a snapshot can take a a few seconds/minutes to finish, so the best option is to change it to an asynchronous call. Change the implementation of the action 'revert' so it returns an asynchronous Task. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/control/vm/snapshots.py | 2 +- src/kimchi/mockmodel.py | 11 +++++++++++ src/kimchi/model/vmsnapshots.py | 28 ++++++++++++++++++++-------- tests/test_model.py | 6 ++++-- tests/test_rest.py | 9 +++++++++ 5 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/kimchi/control/vm/snapshots.py b/src/kimchi/control/vm/snapshots.py index bbebc9a..4847d59 100644 --- a/src/kimchi/control/vm/snapshots.py +++ b/src/kimchi/control/vm/snapshots.py @@ -39,7 +39,7 @@ class VMSnapshot(Resource): self.ident = ident self.model_args = [self.vm, self.ident] self.uri_fmt = '/vms/%s/snapshots/%s' - self.revert = self.generate_action_handler('revert') + self.revert = self.generate_action_handler_task('revert') @property def data(self): diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index dcb8bc1..286f903 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -431,6 +431,15 @@ class MockModel(Model): MockModel._mock_snapshots[vm_name] = snapshots def _mock_vmsnapshot_revert(self, vm_name, name): + params = {'vm_name': vm_name, 'name': name} + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name), + self._vmsnapshot_revert_task, self.objstore, params) + return self.task_lookup(taskid) + + def _vmsnapshot_revert_task(self, cb, params): + vm_name = params['vm_name'] + name = params['name'] + snapshots = MockModel._mock_snapshots.get(vm_name, []) for sn in snapshots: if sn.current: @@ -440,6 +449,8 @@ class MockModel(Model): if sn.name == name: sn.current = True + cb('OK', True) + class MockStorageVolumes(object): def __init__(self): diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py index 3a92cdc..331f532 100644 --- a/src/kimchi/model/vmsnapshots.py +++ b/src/kimchi/model/vmsnapshots.py @@ -29,7 +29,6 @@ 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 -from kimchi.xmlutils.utils import xpath_get_text class VMSnapshotsModel(object): @@ -120,6 +119,8 @@ class VMSnapshotsModel(object): class VMSnapshotModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore'] + self.task = TaskModel(**kargs) def lookup(self, vm_name, name): vir_snap = self.get_vmsnapshot(vm_name, name) @@ -153,19 +154,30 @@ class VMSnapshotModel(object): 'err': e.message}) def revert(self, vm_name, name): + vir_dom = VMModel.get_vm(vm_name, self.conn) + vir_snap = self.get_vmsnapshot(vm_name, name) + + task_params = {'name': name, 'vm_name': vm_name, + 'dom': vir_dom, 'snap': vir_snap} + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name), + self._revert_task, self.objstore, task_params) + return self.task.lookup(taskid) + + def _revert_task(self, cb, params): + vir_dom = params['dom'] + vir_snap = params['snap'] + try: - vir_dom = VMModel.get_vm(vm_name, self.conn) - vir_snap = self.get_vmsnapshot(vm_name, name) + cb('reverting to snapshot') vir_dom.revertToSnapshot(vir_snap, 0) - - # get vm name recorded in the snapshot and return new uri params - vm_new_name = xpath_get_text(vir_snap.getXMLDesc(0), - 'domain/name')[0] - return [vm_new_name, name] except libvirt.libvirtError, e: + name = params['name'] + vm_name = params['vm_name'] + raise OperationFailed('KCHSNAP0009E', {'name': name, 'vm': vm_name, 'err': e.message}) + cb('OK', True) def get_vmsnapshot(self, vm_name, name): vir_dom = VMModel.get_vm(vm_name, self.conn) diff --git a/tests/test_model.py b/tests/test_model.py index c25929f..b53822a 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -198,8 +198,10 @@ class ModelTests(unittest.TestCase): snap = inst.vmsnapshot_lookup(u'kimchi-vm-new', params['name']) # snapshot revert to the first created vm - result = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name']) - self.assertEquals(result, [u'kimchi-vm', snap['name']]) + task = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name']) + inst.task_wait(task['id']) + task = inst.task_lookup(task['id']) + self.assertEquals('finished', task['status']) vm = inst.vm_lookup(u'kimchi-vm') self.assertEquals(vm['state'], snap['state']) diff --git a/tests/test_rest.py b/tests/test_rest.py index c1b81e8..3b592d6 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -382,6 +382,15 @@ class RestTests(unittest.TestCase): # Revert to snapshot resp = self.request('/vms/test-vm/snapshots/%s/revert' % params['name'], '{}', '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') + task = json.loads(resp.read()) + self.assertEquals('finished', task['status']) + + resp = self.request('/vms/test-vm/snapshots/%s' % params['name'], + '{}', 'GET') self.assertEquals(200, resp.status) snap = json.loads(resp.read()) resp = self.request('/vms/test-vm', '{}', 'GET') -- 2.1.0
The UI needs to be changed accordingly, right? Otherwise, the user will get an unstable behaviour while reverting a snapshot with the current upstream UI On 20/04/2015 15:58, Crístian Deives wrote:
The action 'revert' is currently synchronous; that means that there's no way for the user to run it in background and check its status periodically. In some cases, reverting a snapshot can take a a few seconds/minutes to finish, so the best option is to change it to an asynchronous call.
Change the implementation of the action 'revert' so it returns an asynchronous Task.
Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/control/vm/snapshots.py | 2 +- src/kimchi/mockmodel.py | 11 +++++++++++ src/kimchi/model/vmsnapshots.py | 28 ++++++++++++++++++++-------- tests/test_model.py | 6 ++++-- tests/test_rest.py | 9 +++++++++ 5 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/src/kimchi/control/vm/snapshots.py b/src/kimchi/control/vm/snapshots.py index bbebc9a..4847d59 100644 --- a/src/kimchi/control/vm/snapshots.py +++ b/src/kimchi/control/vm/snapshots.py @@ -39,7 +39,7 @@ class VMSnapshot(Resource): self.ident = ident self.model_args = [self.vm, self.ident] self.uri_fmt = '/vms/%s/snapshots/%s' - self.revert = self.generate_action_handler('revert') + self.revert = self.generate_action_handler_task('revert')
@property def data(self): diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index dcb8bc1..286f903 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -431,6 +431,15 @@ class MockModel(Model): MockModel._mock_snapshots[vm_name] = snapshots
def _mock_vmsnapshot_revert(self, vm_name, name): + params = {'vm_name': vm_name, 'name': name} + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name), + self._vmsnapshot_revert_task, self.objstore, params) + return self.task_lookup(taskid) + + def _vmsnapshot_revert_task(self, cb, params): + vm_name = params['vm_name'] + name = params['name'] + snapshots = MockModel._mock_snapshots.get(vm_name, []) for sn in snapshots: if sn.current: @@ -440,6 +449,8 @@ class MockModel(Model): if sn.name == name: sn.current = True
+ cb('OK', True) +
class MockStorageVolumes(object): def __init__(self): diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py index 3a92cdc..331f532 100644 --- a/src/kimchi/model/vmsnapshots.py +++ b/src/kimchi/model/vmsnapshots.py @@ -29,7 +29,6 @@ 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 -from kimchi.xmlutils.utils import xpath_get_text
class VMSnapshotsModel(object): @@ -120,6 +119,8 @@ class VMSnapshotsModel(object): class VMSnapshotModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore'] + self.task = TaskModel(**kargs)
def lookup(self, vm_name, name): vir_snap = self.get_vmsnapshot(vm_name, name) @@ -153,19 +154,30 @@ class VMSnapshotModel(object): 'err': e.message})
def revert(self, vm_name, name): + vir_dom = VMModel.get_vm(vm_name, self.conn) + vir_snap = self.get_vmsnapshot(vm_name, name) + + task_params = {'name': name, 'vm_name': vm_name, + 'dom': vir_dom, 'snap': vir_snap} + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name), + self._revert_task, self.objstore, task_params) + return self.task.lookup(taskid) + + def _revert_task(self, cb, params): + vir_dom = params['dom'] + vir_snap = params['snap'] + try: - vir_dom = VMModel.get_vm(vm_name, self.conn) - vir_snap = self.get_vmsnapshot(vm_name, name) + cb('reverting to snapshot') vir_dom.revertToSnapshot(vir_snap, 0) - - # get vm name recorded in the snapshot and return new uri params - vm_new_name = xpath_get_text(vir_snap.getXMLDesc(0), - 'domain/name')[0] - return [vm_new_name, name] except libvirt.libvirtError, e: + name = params['name'] + vm_name = params['vm_name'] + raise OperationFailed('KCHSNAP0009E', {'name': name, 'vm': vm_name, 'err': e.message}) + cb('OK', True)
def get_vmsnapshot(self, vm_name, name): vir_dom = VMModel.get_vm(vm_name, self.conn) diff --git a/tests/test_model.py b/tests/test_model.py index c25929f..b53822a 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -198,8 +198,10 @@ class ModelTests(unittest.TestCase): snap = inst.vmsnapshot_lookup(u'kimchi-vm-new', params['name'])
# snapshot revert to the first created vm - result = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name']) - self.assertEquals(result, [u'kimchi-vm', snap['name']]) + task = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name']) + inst.task_wait(task['id']) + task = inst.task_lookup(task['id']) + self.assertEquals('finished', task['status'])
vm = inst.vm_lookup(u'kimchi-vm') self.assertEquals(vm['state'], snap['state']) diff --git a/tests/test_rest.py b/tests/test_rest.py index c1b81e8..3b592d6 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -382,6 +382,15 @@ class RestTests(unittest.TestCase): # Revert to snapshot resp = self.request('/vms/test-vm/snapshots/%s/revert' % params['name'], '{}', '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') + task = json.loads(resp.read()) + self.assertEquals('finished', task['status']) + + resp = self.request('/vms/test-vm/snapshots/%s' % params['name'], + '{}', 'GET') self.assertEquals(200, resp.status) snap = json.loads(resp.read()) resp = self.request('/vms/test-vm', '{}', 'GET')
Right. We should only apply this patch when the UI gets updated accordingly. On Mon, Apr 27, 2015 at 12:37 PM Aline Manera <alinefm@linux.vnet.ibm.com> wrote:
The UI needs to be changed accordingly, right? Otherwise, the user will get an unstable behaviour while reverting a snapshot with the current upstream UI
The action 'revert' is currently synchronous; that means that there's no way for the user to run it in background and check its status periodically. In some cases, reverting a snapshot can take a a few seconds/minutes to finish, so the best option is to change it to an asynchronous call.
Change the implementation of the action 'revert' so it returns an asynchronous Task.
Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/control/vm/snapshots.py | 2 +- src/kimchi/mockmodel.py | 11 +++++++++++ src/kimchi/model/vmsnapshots.py | 28 ++++++++++++++++++++-------- tests/test_model.py | 6 ++++-- tests/test_rest.py | 9 +++++++++ 5 files changed, 45 insertions(+), 11 deletions(-)
diff --git a/src/kimchi/control/vm/snapshots.py b/src/kimchi/control/vm/snapshots.py index bbebc9a..4847d59 100644 --- a/src/kimchi/control/vm/snapshots.py +++ b/src/kimchi/control/vm/snapshots.py @@ -39,7 +39,7 @@ class VMSnapshot(Resource): self.ident = ident self.model_args = [self.vm, self.ident] self.uri_fmt = '/vms/%s/snapshots/%s' - self.revert = self.generate_action_handler('revert') + self.revert = self.generate_action_handler_task('revert')
@property def data(self): diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index dcb8bc1..286f903 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -431,6 +431,15 @@ class MockModel(Model): MockModel._mock_snapshots[vm_name] = snapshots
def _mock_vmsnapshot_revert(self, vm_name, name): + params = {'vm_name': vm_name, 'name': name} + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name), + self._vmsnapshot_revert_task, self.objstore,
+ return self.task_lookup(taskid) + + def _vmsnapshot_revert_task(self, cb, params): + vm_name = params['vm_name'] + name = params['name'] + snapshots = MockModel._mock_snapshots.get(vm_name, []) for sn in snapshots: if sn.current: @@ -440,6 +449,8 @@ class MockModel(Model): if sn.name == name: sn.current = True
+ cb('OK', True) +
class MockStorageVolumes(object): def __init__(self): diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py index 3a92cdc..331f532 100644 --- a/src/kimchi/model/vmsnapshots.py +++ b/src/kimchi/model/vmsnapshots.py @@ -29,7 +29,6 @@ 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 -from kimchi.xmlutils.utils import xpath_get_text
class VMSnapshotsModel(object): @@ -120,6 +119,8 @@ class VMSnapshotsModel(object): class VMSnapshotModel(object): def __init__(self, **kargs): self.conn = kargs['conn'] + self.objstore = kargs['objstore'] + self.task = TaskModel(**kargs)
def lookup(self, vm_name, name): vir_snap = self.get_vmsnapshot(vm_name, name) @@ -153,19 +154,30 @@ class VMSnapshotModel(object): 'err': e.message})
def revert(self, vm_name, name): + vir_dom = VMModel.get_vm(vm_name, self.conn) + vir_snap = self.get_vmsnapshot(vm_name, name) + + task_params = {'name': name, 'vm_name': vm_name, + 'dom': vir_dom, 'snap': vir_snap} + taskid = add_task(u'/vms/%s/snapshots/%s/revert' % (vm_name, name), + self._revert_task, self.objstore, task_params) + return self.task.lookup(taskid) + + def _revert_task(self, cb, params): + vir_dom = params['dom'] + vir_snap = params['snap'] + try: - vir_dom = VMModel.get_vm(vm_name, self.conn) - vir_snap = self.get_vmsnapshot(vm_name, name) + cb('reverting to snapshot') vir_dom.revertToSnapshot(vir_snap, 0) - - # get vm name recorded in the snapshot and return new uri
- vm_new_name = xpath_get_text(vir_snap.getXMLDesc(0), - 'domain/name')[0] - return [vm_new_name, name] except libvirt.libvirtError, e: + name = params['name'] + vm_name = params['vm_name'] + raise OperationFailed('KCHSNAP0009E', {'name': name, 'vm': vm_name, 'err': e.message}) + cb('OK', True)
def get_vmsnapshot(self, vm_name, name): vir_dom = VMModel.get_vm(vm_name, self.conn) diff --git a/tests/test_model.py b/tests/test_model.py index c25929f..b53822a 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -198,8 +198,10 @@ class ModelTests(unittest.TestCase): snap = inst.vmsnapshot_lookup(u'kimchi-vm-new',
On 20/04/2015 15:58, Crístian Deives wrote: params) params params['name'])
# snapshot revert to the first created vm - result = inst.vmsnapshot_revert(u'kimchi-vm-new',
- self.assertEquals(result, [u'kimchi-vm', snap['name']]) + task = inst.vmsnapshot_revert(u'kimchi-vm-new',
+ inst.task_wait(task['id']) + task = inst.task_lookup(task['id']) + self.assertEquals('finished', task['status'])
vm = inst.vm_lookup(u'kimchi-vm') self.assertEquals(vm['state'], snap['state']) diff --git a/tests/test_rest.py b/tests/test_rest.py index c1b81e8..3b592d6 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -382,6 +382,15 @@ class RestTests(unittest.TestCase): # Revert to snapshot resp = self.request('/vms/test-vm/snapshots/%s/revert' % params['name'], '{}', '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') + task = json.loads(resp.read()) + self.assertEquals('finished', task['status']) + + resp = self.request('/vms/test-vm/snapshots/%s' %
params['name']) params['name']) params['name'],
+ '{}', 'GET') self.assertEquals(200, resp.status) snap = json.loads(resp.read()) resp = self.request('/vms/test-vm', '{}', 'GET')
Currently, Kimchi only allows snapshots to be taken of shutoff VMs. Now that the infrastructure for taking snapshots of running VMs is done, we can allow that feature as well. Do not restrict the snapshot creation based on the VM's state, and allow it to be created when the VM is running as well. Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/i18n.py | 1 - src/kimchi/model/vmsnapshots.py | 8 ++------ tests/test_model.py | 9 +++++++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 18e84bc..91e543e 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -320,7 +320,6 @@ messages = { "KCHREPOS0029E": _("Repository metalink must be an http://, ftp:// or file:// URL."), "KCHREPOS0030E": _("Cannot specify mirrorlist and metalink at the same time."), - "KCHSNAP0001E": _("Virtual machine '%(vm)s' must be stopped before creating a snapshot of it."), "KCHSNAP0002E": _("Unable to create snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"), "KCHSNAP0003E": _("Snapshot '%(name)s' does not exist on virtual machine '%(vm)s'."), "KCHSNAP0004E": _("Unable to retrieve snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"), diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py index 331f532..85ecb24 100644 --- a/src/kimchi/model/vmsnapshots.py +++ b/src/kimchi/model/vmsnapshots.py @@ -26,7 +26,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.vms import VMModel from kimchi.model.vmstorages import VMStorageModel, VMStoragesModel from kimchi.utils import add_task @@ -42,7 +42,7 @@ class VMSnapshotsModel(object): def create(self, vm_name, params={}): """Create a snapshot with the current domain state. - The VM must be stopped and contain only disks with format 'qcow2'; + The VM must contain only disks with format 'qcow2'; otherwise an exception will be raised. Parameters: @@ -54,10 +54,6 @@ class VMSnapshotsModel(object): Return: A Task running the operation. """ - vir_dom = VMModel.get_vm(vm_name, self.conn) - 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) diff --git a/tests/test_model.py b/tests/test_model.py index b53822a..3740143 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -144,8 +144,13 @@ class ModelTests(unittest.TestCase): info = inst.vm_lookup('kimchi-vm') self.assertEquals('running', info['state']) - self.assertRaises(InvalidOperation, inst.vmsnapshots_create, - u'kimchi-vm') + # make sure creating a snapshot of a running VM works + task = inst.vmsnapshots_create(u'kimchi-vm') + inst.task_wait(task['id']) + task = inst.task_lookup(task['id']) + self.assertEquals('finished', task['status']) + inst.vmsnapshot_delete(u'kimchi-vm', + task['target_uri'].split('/')[-1]) inst.vm_poweroff(u'kimchi-vm') vm = inst.vm_lookup(u'kimchi-vm') -- 2.1.0
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 20/04/2015 15:58, Crístian Deives wrote:
Currently, Kimchi only allows snapshots to be taken of shutoff VMs. Now that the infrastructure for taking snapshots of running VMs is done, we can allow that feature as well.
Do not restrict the snapshot creation based on the VM's state, and allow it to be created when the VM is running as well.
Signed-off-by: Crístian Deives <cristiandeives@gmail.com> --- src/kimchi/i18n.py | 1 - src/kimchi/model/vmsnapshots.py | 8 ++------ tests/test_model.py | 9 +++++++-- 3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 18e84bc..91e543e 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -320,7 +320,6 @@ messages = { "KCHREPOS0029E": _("Repository metalink must be an http://, ftp:// or file:// URL."), "KCHREPOS0030E": _("Cannot specify mirrorlist and metalink at the same time."),
- "KCHSNAP0001E": _("Virtual machine '%(vm)s' must be stopped before creating a snapshot of it."), "KCHSNAP0002E": _("Unable to create snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"), "KCHSNAP0003E": _("Snapshot '%(name)s' does not exist on virtual machine '%(vm)s'."), "KCHSNAP0004E": _("Unable to retrieve snapshot '%(name)s' on virtual machine '%(vm)s'. Details: %(err)s"), diff --git a/src/kimchi/model/vmsnapshots.py b/src/kimchi/model/vmsnapshots.py index 331f532..85ecb24 100644 --- a/src/kimchi/model/vmsnapshots.py +++ b/src/kimchi/model/vmsnapshots.py @@ -26,7 +26,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.vms import VMModel from kimchi.model.vmstorages import VMStorageModel, VMStoragesModel from kimchi.utils import add_task
@@ -42,7 +42,7 @@ class VMSnapshotsModel(object): def create(self, vm_name, params={}): """Create a snapshot with the current domain state.
- The VM must be stopped and contain only disks with format 'qcow2'; + The VM must contain only disks with format 'qcow2'; otherwise an exception will be raised.
Parameters: @@ -54,10 +54,6 @@ class VMSnapshotsModel(object): Return: A Task running the operation. """ - vir_dom = VMModel.get_vm(vm_name, self.conn) - 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) diff --git a/tests/test_model.py b/tests/test_model.py index b53822a..3740143 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -144,8 +144,13 @@ class ModelTests(unittest.TestCase): info = inst.vm_lookup('kimchi-vm') self.assertEquals('running', info['state'])
- self.assertRaises(InvalidOperation, inst.vmsnapshots_create, - u'kimchi-vm') + # make sure creating a snapshot of a running VM works + task = inst.vmsnapshots_create(u'kimchi-vm') + inst.task_wait(task['id']) + task = inst.task_lookup(task['id']) + self.assertEquals('finished', task['status']) + inst.vmsnapshot_delete(u'kimchi-vm', + task['target_uri'].split('/')[-1])
inst.vm_poweroff(u'kimchi-vm') vm = inst.vm_lookup(u'kimchi-vm')
participants (3)
-
Aline Manera -
Crístian Deives -
Crístian Viana