8:05 p.m.
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
8:05 p.m.
New subject: [PATCH V2 1/6] Fix vm start UI error return message
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(a)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
8:05 p.m.
New subject: [PATCH V2 2/6] Fix VM stop error messages handling (backend/UI)
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(a)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
8:05 p.m.
New subject: [PATCH V2 3/6] Fix VM reset (UI) error messages hnadling
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(a)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
8:05 p.m.
New subject: [PATCH V2 4/6] Fix VM delete error message handling (UI/Backend)
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(a)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
8:05 p.m.
New subject: [PATCH V2 5/6] Delete unsed _vm_exists funtion
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(a)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
8:05 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
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(a)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
8:52 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
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(a)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ī-∨м')
9:55 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
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(a)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(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel
4:32 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
Am 26-02-2014 17:55, schrieb Rodrigo Trujillo:
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
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.
6:29 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
On 02/27/2014 12:32 PM, Crístian Viana wrote:
Am 26-02-2014 17:55, schrieb Rodrigo Trujillo:
> 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
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(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel
9:54 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
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.
10:52 p.m.
New subject: [PATCH V2 6/6] Fix Kimchi vm tests according to new messages sent from backend
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(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/kimchi-devel
3733
days inactive
3734
days old
13 comments
3 participants
participants (3)
-
Aline Manera -
Crístian Viana -
Rodrigo Trujillo