[Kimchi-devel] [PATCH 2/3] model test for updating template

[Zhou Zheng Sheng]

on 2014/10/24 21:12, Christy Perez wrote:
> 
> 
> On 10/24/2014 01:43 AM, Zhou Zheng Sheng wrote:
>> Hi Christy,
>>
>> I only found some indentation errors when running pep8 on this file.
>> Maybe you can run a pep8 check manually and fix those problems. For the
>> code itself, I think it's good.
>>
> Thanks Zhou. I'll take care of those. One question for you:
> Did you happen to run the all the tests with this patch (sudo
> ./run_tests.sh)? If I run that, I get a strange failure and a hang. If I
> run just ./run_tests.sh test_model, the tests pass fine.
> 
> I haven't been able to figure out what's going on. I noticed that if I
> add a new test routine of any sort (so a stubbed out method) that I get
> the error. As soon as I delete that new method signature, it goes away.
> 
> It's always just after a debug report test failure, and then I just got
> an abrt notification about the sos package. So, I'm wondering if this
> has something to do with what I have installed (sos-3.1-1.fc20.noarch on
> Fedora 20).
> 
> None of the other test buckets in isolation will hit it, though.
> 
> Regards,
> 
> - Christy

I just located the problem. It's a very nasty bug. The root cause is
that libvirt connection is not shared between model instance.

As you can see, each test method creates a new model instance, and each
new model instance creates a new LibvirtConnection instance. The
original purpose of LibvirtConnection is to share connections among the
same instance. However in test_model, it results a fresh
LibvirtConnection instance for each test method. It seems even after the
model and LibvirtConnection instances go out of scope, the underlying
libvirt connection is not closed. Maybe Python's garbage collection
algorithm does not decide to clean those objects yet.

There is also a known problem of circular reference counting in Python.
Usually Python deal with circular reference very well. However if one of
the object in the reference circle has customized __delete__() method,
Python can not possibly know the correct release order, so it just
refuse to garbage collect them. Unfortunately each class in libvirt
Python binding has customized __delete__() method. I think this might be
the cause why the underlying connection doesn't get freed.

This problem is not related to your patch. Everyone who adds a new test
case will hit this problem. I already drafted a fix for
LibvirtConnection, I'm sending it after I address other problems in
test_model.

>> on 2014/10/24 04:57, Christy Perez wrote:
>>> Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
>>> ---
>>>  tests/test_model.py | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/tests/test_model.py b/tests/test_model.py
>>> index e407fe5..b759667 100644
>>> --- a/tests/test_model.py
>>> +++ b/tests/test_model.py
>>> @@ -800,6 +800,31 @@ def test_template_update(self):
>>>              self.assertRaises(InvalidParameter, inst.template_update,
>>>                                'new-test', params)
>>>
>>> +    @unittest.skipUnless(utils.running_as_root(), 'Must be run as root')
>>> +    def test_template_update_cpu_info(self):
>>> +        inst = model.Model(None,
>>> +                           objstore_loc=self.tmp_store)
>>> +        with RollbackContext() as rollback:
>>> +
>>> +            params = {'name': 'test', 'memory': 1024, 'cpus': 1,
>>> +                           'cdrom': self.kimchi_iso}
>>> +            inst.templates_create(params)
>>> +            rollback.prependDefer(inst.template_delete, 'test')
>>> +
>>> +            # topology and cpus aren't compatible:
>>> +            params['cpu_info'] = {'topology':
>>> +                            {'sockets': 1, 'cores': 1, 'threads': 2}}
>>> +            self.assertRaises(InvalidParameter, inst.template_update, 'test',\
>>> +                             params)
>>> +
>>> +            # Now the update should work:
>>> +            params['cpus'] = 2
>>> +            inst.template_update('test', params)
>>> +
>>> +            info = inst.template_lookup('test')
>>> +            for key in params.keys():
>>> +                self.assertEquals(params[key], info[key])
>>> +
>>>      def test_vm_edit(self):
>>>          inst = model.Model(None,
>>>                             objstore_loc=self.tmp_store)
>>>
>>
>>


-- 
Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397