On 15/12/2015 10:11, pvital@linux.vnet.ibm.com wrote:

From: Paulo Vital <pvital@linux.vnet.ibm.com>

Libvirt of the Fedora 23 is not able to delete the storage volume in the testcase test_image_based_template when testing ModelTests. This happens because Libvirt is not able to unlink/rmdir returning an error (-1) and setting errno to ENOENT, causing the error:

"cannot unlink file '/var/lib/libvirt/images/base-vol.img': Success"

This patch fixes the testcase by manually remove the volume file from the storage pool directory if the wok.plugins.kimchi.model.storagevolume_delete() was not capable to delete the volume.

Signed-off-by: Paulo Vital <pvital@linux.vnet.ibm.com> --- tests/test_model.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/test_model.py b/tests/test_model.py index 762f0f8..4aa19bd 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -96,6 +96,16 @@ class ModelTests(unittest.TestCase):

os.unlink(self.tmp_store)

+ def temporary_storagevolume_delete(inst, pool, vol): + # Quick fix to passby the Libvirt error: + # "error: cannot unlink file '%(vol)': Success" + # in Fedora <= 23. The Libvirt fix will be present only on Fedora 24 + try: + inst.storagevolume_delete(pool, vol) + except OperationFailed as e: + path = inst.storagevolume_lookup(pool, vol)['path'] + os.remove(path) + def test_vm_info(self): inst = model.Model('test:///default', self.tmp_store) vms = inst.vms_get_list() @@ -250,19 +260,22 @@ class ModelTests(unittest.TestCase):

@unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_image_based_template(self): + inst = model.Model(objstore_loc=self.tmp_store)

with RollbackContext() as rollback: + pool = 'default' vol = 'base-vol.img' params = {'name': vol, 'capacity': 1073741824, # 1 GiB 'allocation': 1048576, # 1 MiB 'format': 'qcow2'}

- task_id = inst.storagevolumes_create('default', params)['id'] - rollback.prependDefer(inst.storagevolume_delete, 'default', vol)

Does that mean the user will not be able to delete a volume through Kimchi API? DELETE /plugins/kimchi/storagepools/default/<vol> If so, I suggest to move the manual removal to the API. So even though libvirt fails, Kimchi will be able to remove the volume.

+ task_id = inst.storagevolumes_create(pool, params)['id'] + rollback.prependDefer(temporary_storagevolume_delete, inst, + pool, vol) inst.task_wait(task_id) self.assertEquals('finished', inst.task_lookup(task_id)['status']) - vol_path = inst.storagevolume_lookup('default', vol)['path'] + vol_path = inst.storagevolume_lookup(pool, vol)['path']

# Create template based on IMG file tmpl_name = "img-tmpl"

On 15/12/2015 10:11, pvital@linux.vnet.ibm.com wrote:

From: Paulo Vital <pvital@linux.vnet.ibm.com>

The vmsnapshot_revert() updates the name in the VM XML to the previous VM name (kimchi-vm), but the domain name used by libvirt to lookup this VM/domain, is the updated name (kimchi-vm-new) as proved here: https://paste.fedoraproject.org/296617/49064913/. In addition, if libvirtd is restarted, the name displayed in the 'virsh list --all' command is the updated name.

This patch changed the name of the VM used to test the lifecycle.

Signed-off-by: Paulo Vital <pvital@linux.vnet.ibm.com> --- tests/test_model.py | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/tests/test_model.py b/tests/test_model.py index ff63182..aebd787 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -157,7 +157,7 @@ class ModelTests(unittest.TestCase): params = {'name': 'kimchi-vm', 'template': '/plugins/kimchi/templates/test'} task = inst.vms_create(params) - rollback.prependDefer(inst.vm_delete, 'kimchi-vm') + rollback.prependDefer(inst.vm_delete, 'kimchi-vm-new') inst.task_wait(task['id'], 10) task = inst.task_lookup(task['id']) self.assertEquals('finished', task['status']) @@ -204,7 +204,7 @@ class ModelTests(unittest.TestCase): task = inst.vmsnapshots_create(u'kimchi-vm') snap_name = task['target_uri'].split('/')[-1] rollback.prependDefer(inst.vmsnapshot_delete, - u'kimchi-vm', snap_name) + u'kimchi-vm-new', snap_name) inst.task_wait(task['id']) task = inst.task_lookup(task['id']) self.assertEquals('finished', task['status']) @@ -227,34 +227,42 @@ class ModelTests(unittest.TestCase):

result = inst.vmsnapshot_revert(u'kimchi-vm-new', params['name']) self.assertEquals(result, [u'kimchi-vm', snap['name']])

Usually we don't know when reverting a snapshot will lead on a VM name update. For that, the revert() function returns a list which contains the new VM name: def revert(self, vm_name, name): try: vir_dom = VMModel.get_vm(vm_name, self.conn) vir_snap = self.get_vmsnapshot(vm_name, name) 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: raise OperationFailed('KCHSNAP0009E', {'name': name, 'vm': vm_name, 'err': e.message}) I strongly recommend to get the 'result' value and use the expected name in the operations below. That way we make sure the flow is also working when doing it via UI.

- vm = inst.vm_lookup(u'kimchi-vm') + # The vmsnapshot_revert() updates the name in the VM XML to the + # previous VM name (kimchi-vm), but the domain name used by libvirt + # to lookup this VM/domain, is the updated name (kimchi-vm-new) as + # proved here: https://paste.fedoraproject.org/296617/49064913/.

What do we need to do that to have libvirt working in the real VM name after reverting the snapshot?

+ # In addition, if libvirtd is restarted, the name displayed in the + # 'virsh list --all' cmd is the updated name.

So the conclusion is "Reverting a snapshot WILL NEVER update the vm name". Is that correct? What is the actual Kimchi behavior while performing those action via UI?

+ vm = inst.vm_lookup(u'kimchi-vm-new') self.assertEquals(vm['state'], snap['state'])

- current_snap = inst.currentvmsnapshot_lookup(u'kimchi-vm') + current_snap = inst.currentvmsnapshot_lookup(u'kimchi-vm-new') self.assertEquals(params['name'], current_snap['name'])

self.assertRaises(NotFoundError, inst.vmsnapshot_delete, - u'kimchi-vm', u'foobar') + u'kimchi-vm-new', u'foobar')

# suspend and resume the VM - info = inst.vm_lookup(u'kimchi-vm') + info = inst.vm_lookup(u'kimchi-vm-new') self.assertEquals(info['state'], 'shutoff') - self.assertRaises(InvalidOperation, inst.vm_suspend, u'kimchi-vm') - inst.vm_start(u'kimchi-vm') - info = inst.vm_lookup(u'kimchi-vm') + self.assertRaises(InvalidOperation, inst.vm_suspend, + u'kimchi-vm-new') + inst.vm_start(u'kimchi-vm-new') + info = inst.vm_lookup(u'kimchi-vm-new') self.assertEquals(info['state'], 'running') - inst.vm_suspend(u'kimchi-vm') - info = inst.vm_lookup(u'kimchi-vm') + inst.vm_suspend(u'kimchi-vm-new') + info = inst.vm_lookup(u'kimchi-vm-new') self.assertEquals(info['state'], 'paused') - self.assertRaises(InvalidParameter, inst.vm_update, u'kimchi-vm', - {'name': 'foo'}) - inst.vm_resume(u'kimchi-vm') - info = inst.vm_lookup(u'kimchi-vm') + self.assertRaises(InvalidParameter, inst.vm_update, + u'kimchi-vm-new', {'name': 'foo'}) + inst.vm_resume(u'kimchi-vm-new') + info = inst.vm_lookup(u'kimchi-vm-new') self.assertEquals(info['state'], 'running') - self.assertRaises(InvalidOperation, inst.vm_resume, u'kimchi-vm') + self.assertRaises(InvalidOperation, inst.vm_resume, + u'kimchi-vm-new') # leave the VM suspended to make sure a paused VM can be # deleted correctly - inst.vm_suspend(u'kimchi-vm') + inst.vm_suspend(u'kimchi-vm-new')

vms = inst.vms_get_list() self.assertFalse('kimchi-vm' in vms)