[Kimchi-devel] [PATCH 3/5] Fix 'disk full' issue: Fix storage volume error handling

[Rodrigo Trujillo]

On 03/25/2014 03:40 PM, Aline Manera wrote:
> On 03/24/2014 05:53 PM, Rodrigo Trujillo wrote:
>> This patch fixes the error handling when storagevolume.py tries to
>> update the database with reference count information.
>>
>> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo at linux.vnet.ibm.com>
>> ---
>>   src/kimchi/i18n.py                 |  1 +
>>   src/kimchi/model/storagevolumes.py | 45 
>> +++++++++++++++++++++++---------------
>>   2 files changed, 28 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
>> index 231ea25..81a74c4 100644
>> --- a/src/kimchi/i18n.py
>> +++ b/src/kimchi/i18n.py
>> @@ -163,6 +163,7 @@ messages = {
>>       "KCHVOL0014E": _("Storage volume allocation must be an integer 
>> number"),
>>       "KCHVOL0015E": _("Storage volume format not supported"),
>>       "KCHVOL0016E": _("Storage volume requires a volume name"),
>> +    "KCHVOL0017E": _("Unable to update database with storage volume 
>> information due error: %(err)s"),
>>
>>       "KCHIFACE0001E": _("Interface %(name)s does not exist"),
>>
>> diff --git a/src/kimchi/model/storagevolumes.py 
>> b/src/kimchi/model/storagevolumes.py
>> index 1253823..d4fbb0e 100644
>> --- a/src/kimchi/model/storagevolumes.py
>> +++ b/src/kimchi/model/storagevolumes.py
>> @@ -79,8 +79,13 @@ class StorageVolumesModel(object):
>>                                     {'name': name, 'pool': pool,
>>                                      'err': e.get_error_message()})
>>
>> -        with self.objstore as session:
>> -            session.store('storagevolume', vol_id, {'ref_cnt': 0})
>> +        try:
>> +            with self.objstore as session:
>> +                session.store('storagevolume', vol_id, {'ref_cnt': 0})
>> +        except Exception as e:
>> +            # If the storage volume was created flawlessly, then 
>> lets hide this
>> +            # error to avoid more error in the VM creation process
>> +            pass
>
> Log the error to help on debug when needed.
I did not log here because the '__exit__'  from 'with' would do it. But 
I can include a message in storage volume context, makes sense. Thanks

>
>>
>>           return name
>>
>> @@ -117,22 +122,26 @@ class StorageVolumeModel(object):
>>
>>       def _get_ref_cnt(self, pool, name, path):
>>           vol_id = '%s:%s' % (pool, name)
>> -        with self.objstore as session:
>> -            try:
>> -                ref_cnt = session.get('storagevolume', 
>> vol_id)['ref_cnt']
>> -            except NotFoundError:
>> -                # Fix storage volume created outside kimchi scope
>> -                ref_cnt = 0
>> -                args = {'conn': self.conn, 'objstore': self.objstore}
>> -                # try to find this volume in exsisted vm
>> -                vms = VMsModel.get_vms(self.conn)
>> -                for vm in vms:
>> -                    storages = VMStoragesModel(**args).get_list(vm)
>> -                    for disk in storages:
>> -                        d_info = VMStorageModel(**args).lookup(vm, 
>> disk)
>> -                        if path == d_info['path']:
>> -                            ref_cnt = ref_cnt + 1
>> -                session.store('storagevolume', vol_id, {'ref_cnt': 
>> ref_cnt})
>
>> +        try:
>> +            with self.objstore as session:
>> +                try:
>> +                    ref_cnt = session.get('storagevolume', 
>> vol_id)['ref_cnt']
>> +                except NotFoundError:
>> +                    # Fix storage volume created outside kimchi scope
>> +                    ref_cnt = 0
>> +                    args = {'conn': self.conn, 'objstore': 
>> self.objstore}
>> +                    # try to find this volume in exsisted vm
>> +                    vms = VMsModel.get_vms(self.conn)
>> +                    for vm in vms:
>> +                        storages = VMStoragesModel(**args).get_list(vm)
>> +                        for disk in storages:
>> +                            d_info = 
>> VMStorageModel(**args).lookup(vm, disk)
>> +                            if path == d_info['path']:
>> +                                ref_cnt = ref_cnt + 1
>> +                    session.store('storagevolume', vol_id,
>> +                                 {'ref_cnt': ref_cnt})
>> +        except Exception as e:
>> +            raise OperationFailed('KCHVOL0017E',{'err': e.message})
>
> Use existent try statement inside the with block:
>
> with self.objstore as session:
>     try:
>         ...
>     except NotFoundError:
>         ...
>     except Exception:
>         raise OperationFailed()
>
I know it is ugly, but I had to do this way, I mean, include the 'with' 
inside a new 'try'.
In order to remove the outside 'try', I would have to do something like:

with:
     ...
     try:
         ...
     except:
         ....
         try:
             session.store('storagevolume', vol_id, {'ref_cnt': ref_cnt})
         except:
             <print/log db error>   -->  Because the __exit__ is going 
to receive the exception below and lose the sqlite3 error
             raise OperationFailed()

This is feasible. Did not implement this way to make the code more 
standard (I mean, just __exit__ would log the sqlite3 errors)
Do you prefer this new way ?  I can change.

Thanks

>>
>>           return ref_cnt
>>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>