[PATCH V2 0/6] Fix VM error messages handling
V2: Fix tests V1: The Guest tab was not showing backend error messages properly. So user was not able to know what happened if a VM failed to start, stop, reset or delete. In order to fix this, both backend and frontend had to be changed. Rodrigo Trujillo (6): Fix vm start UI error return message Fix VM stop error messages handling (backend/UI) Fix VM reset (UI) error messages hnadling Fix VM delete error message handling (UI/Backend) Delete unsed _vm_exists funtion Fix Kimchi vm tests according to new messages sent from backend src/kimchi/i18n.py | 2 ++ src/kimchi/model/vms.py | 51 ++++++++++++++++++++---------------------- tests/test_model.py | 29 +++++++++++++++++++----- ui/js/src/kimchi.guest_main.js | 26 ++++++++++----------- ui/pages/i18n.html.tmpl | 1 + 5 files changed, 63 insertions(+), 46 deletions(-) -- 1.8.5.3
This patch fixed the UI error return message and also fix the button name used in the JS function. Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_main.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/js/src/kimchi.guest_main.js b/ui/js/src/kimchi.guest_main.js index e350373..ab09976 100644 --- a/ui/js/src/kimchi.guest_main.js +++ b/ui/js/src/kimchi.guest_main.js @@ -29,9 +29,9 @@ kimchi.vmstart = function(event) { kimchi.startVM(vm_id, function(result) { button.removeClass('loading'); kimchi.listVmsAuto(); - }, function() { - startButton.removeClass('loading'); - kimchi.message.error(i18n['msg.fail.start']); + }, function(err) { + button.removeClass('loading'); + kimchi.message.error(err.responseJSON.reason); } ); } else { -- 1.8.5.3
The UI was not able to catch the error messages sent from backend when a user tried to stop a VM. This patch fixes the processes and also change the 'stop' backend function, then exceptions from "get_vm" are send to frontend. Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 7 +++++-- ui/js/src/kimchi.guest_main.js | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index ca72a80..f262c06 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -77,6 +77,7 @@ messages = { "KCHVM0017E": _("Volume list (LUNs names) not given."), "KCHVM0018E": _("Virtual machine volumes must be a list of strings with distinct LUNs names."), "KCHVM0019E": _("Unable to start virtual machine %(name)s. Details: %(err)s"), + "KCHVM0020E": _("Unable to stop virtual machine %(name)s. Details: %(err)s"), "KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index b6a42e6..7f29f1a 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -383,9 +383,12 @@ class VMModel(object): {'name': name, 'err': e.get_error_message()}) def stop(self, name): - if self._vm_exists(name): - dom = self.get_vm(name, self.conn) + dom = self.get_vm(name, self.conn) + try: dom.destroy() + except libvirt.libvirtError as e: + raise OperationFailed("KCHVM0020E", + {'name': name, 'err': e.get_error_message()}) def _vm_get_graphics(self, name): dom = self.get_vm(name, self.conn) diff --git a/ui/js/src/kimchi.guest_main.js b/ui/js/src/kimchi.guest_main.js index ab09976..0aadc9b 100644 --- a/ui/js/src/kimchi.guest_main.js +++ b/ui/js/src/kimchi.guest_main.js @@ -50,8 +50,8 @@ kimchi.vmstop = function(event) { kimchi.stopVM(vm_id, function(result) { button.removeClass('loading'); kimchi.listVmsAuto(); - }, function() { - kimchi.message.error(i18n['msg.fail.stop']); + }, function(err) { + kimchi.message.error(err.responseJSON.reason); }); } else { event.preventDefault(); -- 1.8.5.3
This patch fixes the UI in order to show properly the messages sent by backend when usert tries to reset a running VM. Error messages are basically sent from vm 'stop' and 'start' functions. Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- ui/js/src/kimchi.guest_main.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/js/src/kimchi.guest_main.js b/ui/js/src/kimchi.guest_main.js index 0aadc9b..41bf18d 100644 --- a/ui/js/src/kimchi.guest_main.js +++ b/ui/js/src/kimchi.guest_main.js @@ -64,8 +64,8 @@ kimchi.vmreset = function(event){ var vm_id=vm.attr("id"); kimchi.resetVM(vm_id, function(result) { kimchi.listVmsAuto(); - }, function() { - kimchi.message.error(i18n['msg.fail.reset']); + }, function(err) { + kimchi.message.error(err.responseJSON.reason); } ); }; -- 1.8.5.3
This patch fixes delete confirmation window, changes backend in order to raise proper exception and fixes the ui to catch the right message from backend. Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 1 + src/kimchi/model/vms.py | 31 +++++++++++++++++-------------- ui/js/src/kimchi.guest_main.js | 12 ++++++------ ui/pages/i18n.html.tmpl | 1 + 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index f262c06..e955d3b 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -78,6 +78,7 @@ messages = { "KCHVM0018E": _("Virtual machine volumes must be a list of strings with distinct LUNs names."), "KCHVM0019E": _("Unable to start virtual machine %(name)s. Details: %(err)s"), "KCHVM0020E": _("Unable to stop virtual machine %(name)s. Details: %(err)s"), + "KCHVM0021E": _("Unable to delete virtual machine %(name)s. Details: %(err)s"), "KCHVMIF0001E": _("Interface %(iface)s does not exist in virtual machine %(name)s"), "KCHVMIF0002E": _("Network %(network)s specified for virtual machine %(name)s does not exist"), diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index 7f29f1a..f999ed0 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -345,26 +345,29 @@ class VMModel(object): raise def delete(self, name): - if self._vm_exists(name): - conn = self.conn.get() - dom = self.get_vm(name, self.conn) - self._vmscreenshot_delete(dom.UUIDString()) - paths = self._vm_get_disk_paths(dom) - info = self.lookup(name) + conn = self.conn.get() + dom = self.get_vm(name, self.conn) + self._vmscreenshot_delete(dom.UUIDString()) + paths = self._vm_get_disk_paths(dom) + info = self.lookup(name) - if info['state'] == 'running': - self.stop(name) + if info['state'] == 'running': + self.stop(name) + try: dom.undefine() + except libvirt.libvirtError as e: + raise OperationFailed("KCHVM0021E", + {'name': name, 'err': e.get_error_message()}) - for path in paths: - vol = conn.storageVolLookupByPath(path) - vol.delete(0) + for path in paths: + vol = conn.storageVolLookupByPath(path) + vol.delete(0) - with self.objstore as session: - session.delete('vm', dom.UUIDString(), ignore_missing=True) + with self.objstore as session: + session.delete('vm', dom.UUIDString(), ignore_missing=True) - vnc.remove_proxy_token(name) + vnc.remove_proxy_token(name) def start(self, name): # make sure the ISO file has read permission diff --git a/ui/js/src/kimchi.guest_main.js b/ui/js/src/kimchi.guest_main.js index 41bf18d..ead5f3b 100644 --- a/ui/js/src/kimchi.guest_main.js +++ b/ui/js/src/kimchi.guest_main.js @@ -74,16 +74,16 @@ kimchi.vmdelete = function(event) { var vm = $(this).closest('li[name=guest]'); var vm_id=vm.attr("id"); var settings = { - title : i18n['msg.confirm.delete.title'], - content : i18n['msg.vm.confirm.delete'], - confirm : i18n['msg.confirm.delete.confirm'], - cancel : i18n['msg.confirm.delete.cancel'] + title : i18n['KCHVM6002M'], + content : i18n['KCHVM6001M'], + confirm : i18n['KCHAPI6002M'], + cancel : i18n['KCHAPI6003M'] }; kimchi.confirm(settings, function() { kimchi.deleteVM(vm_id, function(result) { kimchi.listVmsAuto(); - }, function() { - kimchi.message.error(i18n['msg.fail.delete']); + }, function(err) { + kimchi.message.error(err.responseJSON.reason); }); }, function() { }); diff --git a/ui/pages/i18n.html.tmpl b/ui/pages/i18n.html.tmpl index f8af68e..b3eeab8 100644 --- a/ui/pages/i18n.html.tmpl +++ b/ui/pages/i18n.html.tmpl @@ -95,6 +95,7 @@ var i18n = { 'KCHDR6011M': "$_("Report name should contain only letters, digits and/or hyphen ('-').")", 'KCHVM6001M': "$_("This will delete the virtual machine and its virtual disks. This operation cannot be undone. Would you like to continue?")", + 'KCHVM6002M': "$_("Delete Confirmation")", 'KCHNET6001E': "$_("The VLAN id must be between 1 and 4094.")", -- 1.8.5.3
Due to the new error messaging handling the function _vm_exists is not necessary anymore and moved OperationFailed exception to get_vm function, where makes more sence. Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- src/kimchi/model/vms.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py index f999ed0..14867e1 100644 --- a/src/kimchi/model/vms.py +++ b/src/kimchi/model/vms.py @@ -322,16 +322,6 @@ class VMModel(object): xpath = "/domain/devices/disk[@device='disk']/source/@file" return xmlutils.xpath_get_text(xml, xpath) - def _vm_exists(self, name): - try: - self.get_vm(name, self.conn) - return True - except NotFoundError: - return False - except Exception, e: - raise OperationFailed("KCHVM0009E", {'name': name, - 'err': e.message}) - @staticmethod def get_vm(name, conn): conn = conn.get() @@ -342,7 +332,8 @@ class VMModel(object): if e.get_error_code() == libvirt.VIR_ERR_NO_DOMAIN: raise NotFoundError("KCHVM0002E", {'name': name}) else: - raise + raise OperationFailed("KCHVM0009E", {'name': name, + 'err': e.message}) def delete(self, name): conn = self.conn.get() -- 1.8.5.3
With new backend messaging system, when a VM is not found the backend raises an exception which was breaking Rollback functions. This patch fixes this problem. Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- tests/test_model.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index 74e2424..d48377d 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -547,14 +547,17 @@ class ModelTests(unittest.TestCase): params_2 = {'name': 'kimchi-vm2', 'template': '/templates/test'} inst.vms_create(params_1) inst.vms_create(params_2) - rollback.prependDefer(inst.vm_delete, 'kimchi-vm1') - rollback.prependDefer(inst.vm_delete, 'kimchi-vm2') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + 'kimchi-vm1') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + 'kimchi-vm2') vms = inst.vms_get_list() self.assertTrue('kimchi-vm1' in vms) inst.vm_start('kimchi-vm1') - rollback.prependDefer(inst.vm_stop, 'kimchi-vm1') + rollback.prependDefer(self._rollback_wrapper, inst.vm_stop, + 'kimchi-vm1') info = inst.vm_lookup('kimchi-vm1') self.assertEquals('running', info['state']) @@ -569,7 +572,8 @@ class ModelTests(unittest.TestCase): 'kimchi-vm1', {'name': 'kimchi-vm2'}) inst.vm_update('kimchi-vm1', params) self.assertEquals(info['uuid'], inst.vm_lookup(u'пeω-∨м')['uuid']) - rollback.prependDefer(inst.vm_delete, u'пeω-∨м') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + u'пeω-∨м') @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_network(self): @@ -758,6 +762,17 @@ class ModelTests(unittest.TestCase): inst.task_lookup(taskid)['message']) self.assertEquals('failed', inst.task_lookup(taskid)['status']) + # This wrapper function is needed due to the new backend messaging in + # vm model. vm_stop and vm_delete raise exception if vm is not found. + # These functions are called after vm has been deleted if test finishes + # correctly, then NofFoundError exception is raised and rollback breaks + def _rollback_wrapper(self, func, vmname): + try: + func(vmname) + except NotFoundError: + # VM has been deleted already + return + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_delete_running_vm(self): inst = model.Model(objstore_loc=self.tmp_store) @@ -769,10 +784,12 @@ class ModelTests(unittest.TestCase): params = {'name': u'kīмсhī-∨м', 'template': u'/templates/test'} inst.vms_create(params) - rollback.prependDefer(inst.vm_delete, u'kīмсhī-∨м') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + u'kīмсhī-∨м') inst.vm_start(u'kīмсhī-∨м') - rollback.prependDefer(inst.vm_stop, u'kīмсhī-∨м') + rollback.prependDefer(self._rollback_wrapper, inst.vm_stop, + u'kīмсhī-∨м') inst.vm_delete(u'kīмсhī-∨м') -- 1.8.5.3
AFAIU, here is what happens in the method "test_vm_update": the VMs "kimchi-vm1" and "kimchi-vm2" are created; the commands to *delete those VMS by their names* are deferred in the transaction; *the VM "kimchi-vm1" is renamed to "пeω-∨м"*; the command to *delete "пeω-∨м" by its name* is deferred in the transaction. Notice how the VM is deleted twice. Both "kimchi-vm1" and "пeω-∨м" are the same VMs, with different names in different moments. The command "vm_delete" fails for two reasons: 1) the VM "kimchi-vm1" does not exist anymore when "vm_delete" is executed (NotFoundError); 2) even if it did, you cannot delete a VM twice (also NotFoundError). So the correct solution would be to not delete the VM twice, and keep track of the names they have during the tests. If the VMs are renamed, the deferred commands will fail in the future when they execute. The same analysis can be done to work out a better solution for the "vm_stop" case. That function "_rollback_wrapper" looks very hacky and there should be a way not to need it. Am 26-02-2014 16:05, schrieb Rodrigo Trujillo:
With new backend messaging system, when a VM is not found the backend raises an exception which was breaking Rollback functions. This patch fixes this problem.
Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- tests/test_model.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index 74e2424..d48377d 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -547,14 +547,17 @@ class ModelTests(unittest.TestCase): params_2 = {'name': 'kimchi-vm2', 'template': '/templates/test'} inst.vms_create(params_1) inst.vms_create(params_2) - rollback.prependDefer(inst.vm_delete, 'kimchi-vm1') - rollback.prependDefer(inst.vm_delete, 'kimchi-vm2') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + 'kimchi-vm1') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + 'kimchi-vm2')
vms = inst.vms_get_list() self.assertTrue('kimchi-vm1' in vms)
inst.vm_start('kimchi-vm1') - rollback.prependDefer(inst.vm_stop, 'kimchi-vm1') + rollback.prependDefer(self._rollback_wrapper, inst.vm_stop, + 'kimchi-vm1')
info = inst.vm_lookup('kimchi-vm1') self.assertEquals('running', info['state']) @@ -569,7 +572,8 @@ class ModelTests(unittest.TestCase): 'kimchi-vm1', {'name': 'kimchi-vm2'}) inst.vm_update('kimchi-vm1', params) self.assertEquals(info['uuid'], inst.vm_lookup(u'пeω-∨м')['uuid']) - rollback.prependDefer(inst.vm_delete, u'пeω-∨м') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + u'пeω-∨м')
@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_network(self): @@ -758,6 +762,17 @@ class ModelTests(unittest.TestCase): inst.task_lookup(taskid)['message']) self.assertEquals('failed', inst.task_lookup(taskid)['status'])
+ # This wrapper function is needed due to the new backend messaging in + # vm model. vm_stop and vm_delete raise exception if vm is not found. + # These functions are called after vm has been deleted if test finishes + # correctly, then NofFoundError exception is raised and rollback breaks + def _rollback_wrapper(self, func, vmname): + try: + func(vmname) + except NotFoundError: + # VM has been deleted already + return + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_delete_running_vm(self): inst = model.Model(objstore_loc=self.tmp_store) @@ -769,10 +784,12 @@ class ModelTests(unittest.TestCase):
params = {'name': u'kīмсhī-∨м', 'template': u'/templates/test'} inst.vms_create(params) - rollback.prependDefer(inst.vm_delete, u'kīмсhī-∨м') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + u'kīмсhī-∨м')
inst.vm_start(u'kīмсhī-∨м') - rollback.prependDefer(inst.vm_stop, u'kīмсhī-∨м') + rollback.prependDefer(self._rollback_wrapper, inst.vm_stop, + u'kīмсhī-∨м')
inst.vm_delete(u'kīмсhī-∨м')
Hi Cristian, your analyses about the flow and root of the problem are correct. However I think you are only seeing the case where the flow just works, you did not took in account that rollbacks are used in case of errors/crashes/fails in the middle of test. For instance: 1- inst.vms_create(params_1) 2- inst.vms_create(params_2) 3- vms = inst.vms_get_list() 4- inst.vm_start('kimchi-vm1') 5- info = inst.vm_lookup('kimchi-vm1') ... - If the code fails/breaks in (3), then (1) and (2) must be 'rollbacked'. - If the code fails/breaks in (5) , then (1), (2) and (4) must be 'rollbacked' ... (not really sure if 4 is needed, but the code was already there) For these reasons, I cannot remove the rollback after create the vms, otherwise, they will remains in the system. If the code breaks in (4), while starting the vm (receive an exception), how would you remove the vms created above (1)(2) ? I asked myself this. The option I could think know is to add try/except in the code, but, the with/rollback was implemented exactly for this (or to avoid this). I also tried to split with/rollback, but this does not work. On 02/26/2014 04:52 PM, Crístian Viana wrote:
AFAIU, here is what happens in the method "test_vm_update": the VMs "kimchi-vm1" and "kimchi-vm2" are created; the commands to *delete those VMS by their names* are deferred in the transaction; *the VM "kimchi-vm1" is renamed to "?e?-??"*; the command to *delete "?e?-??" by its name* is deferred in the transaction.
Notice how the VM is deleted twice. Both "kimchi-vm1" and "?e?-??" are the same VMs, with different names in different moments. The command "vm_delete" fails for two reasons: 1) the VM "kimchi-vm1" does not exist anymore when "vm_delete" is executed (NotFoundError); 2) even if it did, you cannot delete a VM twice (also NotFoundError). So the correct solution would be to not delete the VM twice, and keep track of the names they have during the tests. If the VMs are renamed, the deferred commands will fail in the future when they execute.
The same analysis can be done to work out a better solution for the "vm_stop" case. That function "_rollback_wrapper" looks very hacky and there should be a way not to need it.
Am 26-02-2014 16:05, schrieb Rodrigo Trujillo:
With new backend messaging system, when a VM is not found the backend raises an exception which was breaking Rollback functions. This patch fixes this problem.
Signed-off-by: Rodrigo Trujillo<rodrigo.trujillo@linux.vnet.ibm.com> --- tests/test_model.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/tests/test_model.py b/tests/test_model.py index 74e2424..d48377d 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -547,14 +547,17 @@ class ModelTests(unittest.TestCase): params_2 = {'name': 'kimchi-vm2', 'template': '/templates/test'} inst.vms_create(params_1) inst.vms_create(params_2) - rollback.prependDefer(inst.vm_delete, 'kimchi-vm1') - rollback.prependDefer(inst.vm_delete, 'kimchi-vm2') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + 'kimchi-vm1') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + 'kimchi-vm2')
vms = inst.vms_get_list() self.assertTrue('kimchi-vm1' in vms)
inst.vm_start('kimchi-vm1') - rollback.prependDefer(inst.vm_stop, 'kimchi-vm1') + rollback.prependDefer(self._rollback_wrapper, inst.vm_stop, + 'kimchi-vm1')
info = inst.vm_lookup('kimchi-vm1') self.assertEquals('running', info['state']) @@ -569,7 +572,8 @@ class ModelTests(unittest.TestCase): 'kimchi-vm1', {'name': 'kimchi-vm2'}) inst.vm_update('kimchi-vm1', params) self.assertEquals(info['uuid'], inst.vm_lookup(u'?e?-??')['uuid']) - rollback.prependDefer(inst.vm_delete, u'?e?-??') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + u'?e?-??')
@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_network(self): @@ -758,6 +762,17 @@ class ModelTests(unittest.TestCase): inst.task_lookup(taskid)['message']) self.assertEquals('failed', inst.task_lookup(taskid)['status'])
+ # This wrapper function is needed due to the new backend messaging in + # vm model. vm_stop and vm_delete raise exception if vm is not found. + # These functions are called after vm has been deleted if test finishes + # correctly, then NofFoundError exception is raised and rollback breaks + def _rollback_wrapper(self, func, vmname): + try: + func(vmname) + except NotFoundError: + # VM has been deleted already + return + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_delete_running_vm(self): inst = model.Model(objstore_loc=self.tmp_store) @@ -769,10 +784,12 @@ class ModelTests(unittest.TestCase):
params = {'name': u'ki-??hi--??', 'template': u'/templates/test'} inst.vms_create(params) - rollback.prependDefer(inst.vm_delete, u'ki-??hi--??') + rollback.prependDefer(self._rollback_wrapper, inst.vm_delete, + u'ki-??hi--??')
inst.vm_start(u'ki-??hi--??') - rollback.prependDefer(inst.vm_stop, u'ki-??hi--??') + rollback.prependDefer(self._rollback_wrapper, inst.vm_stop, + u'ki-??hi--??')
inst.vm_delete(u'ki-??hi--??')
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
However I think you are only seeing the case where the flow just works, you did not took in account that rollbacks are used in case of errors/crashes/fails in the middle of test. You are right, I did not see the cases when the test fails. If the code breaks in (4), while starting the vm (receive an exception), how would you remove the vms created above (1)(2) ? I asked myself this. The option I could think know is to add try/except in the code, but, the with/rollback was implemented exactly for this (or to avoid this). I also tried to split with/rollback, but this does not work. One solution would be to remove the VMs by their UUIDs instead of by
Am 26-02-2014 17:55, schrieb Rodrigo Trujillo: their names. The UUID is a unique value and it never changes, so the deferred commands would always work, even if the VM name changes. However the Kimchi function "vm_delete" expects only a name, not a UUID (which, IMO, was not a good design, given that libvirt allows us to lookup VMs by names and UUIDs). So I do not see an easy way to solve this properly without having "vm_delete" to accept something like a domain object instead of its name. The function "_rollback_wrapper" still looks ugly though.
On 02/27/2014 12:32 PM, Crístian Viana wrote:
However I think you are only seeing the case where the flow just works, you did not took in account that rollbacks are used in case of errors/crashes/fails in the middle of test. You are right, I did not see the cases when the test fails. If the code breaks in (4), while starting the vm (receive an exception), how would you remove the vms created above (1)(2) ? I asked myself this. The option I could think know is to add try/except in the code, but, the with/rollback was implemented exactly for this (or to avoid this). I also tried to split with/rollback, but this does not work. One solution would be to remove the VMs by their UUIDs instead of by
Am 26-02-2014 17:55, schrieb Rodrigo Trujillo: their names. The UUID is a unique value and it never changes, so the deferred commands would always work, even if the VM name changes.
However the Kimchi function "vm_delete" expects only a name, not a UUID (which, IMO, was not a good design, given that libvirt allows us to lookup VMs by names and UUIDs). So I do not see an easy way to solve this properly without having "vm_delete" to accept something like a domain object instead of its name.
Even using UUID, the rollback problem will still exist. I don't see any other better solution to it
The function "_rollback_wrapper" still looks ugly though.
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
Am 27-02-2014 14:29, schrieb Aline Manera:
Even using UUID, the rollback problem will still exist.
Using UUID would be enough. Take a look at the code: with RollbackContext() as rollback: params = {'name': u'test', 'disks': []} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') params = {'name': u'ki-??hi--??', 'template': u'/templates/test'} inst.vms_create(params) * rollback.prependDefer(self._rollback_wrapper, inst.vm_delete,** ** u'ki-??hi--??')* inst.vm_start(u'ki-??hi--??') * rollback.prependDefer(self._rollback_wrapper, inst.vm_stop,** ** u'ki-??hi--??')* inst.vm_delete(u'ki-??hi--??') vms = inst.vms_get_list() self.assertFalse(u'ki-??hi--??' in vms) Suppose the two lines defer commands like "inst.vm_delete_by_uuid(uuid)". Even if something fails, if the VMs' names change, or if everything executes successfully, they would stop and delete the VM in the end of the transaction. By the way, the command "inst.vm_delete(u'ki-??hi--??')" will delete the VM and will obviously lead to an error when the deferred "vm_delete" is executed, because the VM will not exist anymore.
On 02/27/2014 05:54 PM, Crístian Viana wrote:
Am 27-02-2014 14:29, schrieb Aline Manera:
Even using UUID, the rollback problem will still exist.
Using UUID would be enough.
Take a look at the code:
with RollbackContext() as rollback: params = {'name': u'test', 'disks': []} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test')
params = {'name': u'ki-??hi--??', 'template': u'/templates/test'} inst.vms_create(params) * rollback.prependDefer(self._rollback_wrapper, inst.vm_delete,** ** u'ki-??hi--??')*
inst.vm_start(u'ki-??hi--??') * rollback.prependDefer(self._rollback_wrapper, inst.vm_stop,** ** u'ki-??hi--??')*
inst.vm_delete(u'ki-??hi--??')
vms = inst.vms_get_list() self.assertFalse(u'ki-??hi--??' in vms)
Suppose the two lines defer commands like "inst.vm_delete_by_uuid(uuid)". Even if something fails, if the VMs' names change, or if everything executes successfully, they would stop and delete the VM in the end of the transaction.
I agree with you that UUID works ... but I see it solving the problem when VM is renamed, only.
By the way, the command "inst.vm_delete(u'ki-??hi--??')" will delete the VM and will obviously lead to an error when the deferred "vm_delete" is executed, because the VM will not exist anymore.
Yeap, this was the root cause of the bug... and would happen using UUID or 'name'. I understand that rollback_wrapper function is not so elegant ... but, in fact, it is just "checking" if the vm exists in order to does not break rollback, which does not have its behaviour changed (maybe I named the function wrongly).
_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel
participants (3)
-
Aline Manera -
Crístian Viana -
Rodrigo Trujillo