[Kimchi-devel] [PATCH 1/3] Add disks to LVM pool: control and model changes

[Aline Manera]

On 02/10/2014 12:17 AM, Sheldon wrote:
> a minor comment below
>
> On 02/07/2014 08:53 PM, Daniel Barboza wrote:
>> From: Daniel Henrique Barboza<danielhb at linux.vnet.ibm.com>
>>
>> Added a new update parameter to storagepool resource. Changed the
>> logic in the model to accept any update parameter ('autostart' or
>> 'disks'). 'disks' parameter will add disks/partitions to the
>> target LVM pool.
> need to update API.json?

Good point.

I've just checked the API.json and there is no entry for 
storagepool_update so it
will need to add a new entry for that

>> Signed-off-by: Daniel Henrique Barboza<danielhb at linux.vnet.ibm.com>
>> ---
>>   src/kimchi/control/storagepools.py |  2 +-
>>   src/kimchi/model/storagepools.py   | 35 
>> +++++++++++++++++++++++++++--------
>>   2 files changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/kimchi/control/storagepools.py 
>> b/src/kimchi/control/storagepools.py
>> index ae5185c..af10acd 100644
>> --- a/src/kimchi/control/storagepools.py
>> +++ b/src/kimchi/control/storagepools.py
>> @@ -80,7 +80,7 @@ class StoragePools(Collection):
>>   class StoragePool(Resource):
>>       def __init__(self, model, ident):
>>           super(StoragePool, self).__init__(model, ident)
>> -        self.update_params = ["autostart"]
>> +        self.update_params = ["autostart", "disks"]
>>           self.uri_fmt = "/storagepools/%s"
>>           self.activate = self.generate_action_handler('activate')
>>           self.deactivate = self.generate_action_handler('deactivate')
>> diff --git a/src/kimchi/model/storagepools.py 
>> b/src/kimchi/model/storagepools.py
>> index 233a8a7..89cb61f 100644
>> --- a/src/kimchi/model/storagepools.py
>> +++ b/src/kimchi/model/storagepools.py
>> @@ -28,7 +28,7 @@ from kimchi.exception import InvalidOperation, 
>> MissingParameter
>>   from kimchi.exception import NotFoundError, OperationFailed
>>   from kimchi.model.libvirtstoragepool import StoragePoolDef
>>   from kimchi.utils import add_task, kimchi_log
>> -
>> +from kimchi.utils import run_command
>>
>>   ISO_POOL_NAME = u'kimchi_isos'
>>   POOL_STATE_MAP = {0: 'inactive',
>> @@ -200,15 +200,34 @@ class StoragePoolModel(object):
>>                   pass
>>           return res
>>
>> +    def _update_lvm_disks(self, pool_name, disks):
>> +        vgextend_cmd = ["vgextend", pool_name]
>> +        vgextend_cmd += disks
>> +        output, error, returncode = run_command(vgextend_cmd)
>> +        if returncode != 0:
>> +            kimchi_log.error('Could not add disks to pool %s, '
>> +                             'error: %s', pool_name, error)
>> +            raise OperationFailed('Error while adding disks to pool 
>> %s.',
>> +                                  pool_name)
>> +        pool_refresh_cmd = ["virsh", "pool-refresh", pool_name]
> src/kimchi/model/storagepools.py:
> pool = self.get_storagepool(pool_name , self.conn)
> pool.refresh(0)
>> +        output, error, returncode = run_command(pool_refresh_cmd)
>> +        if returncode != 0:
>> +            kimchi_log.error('Could not refresh pool %s using 
>> libvirt, '
>> +                             'error: %s', pool_name, error)
>> +            raise OperationFailed('Error while refreshing pool %s '
>> +                                  'using libvirt.', pool_name)
>> +
>>       def update(self, name, params):
>> -        autostart = params['autostart']
>> -        if autostart not in [True, False]:
>> -            raise InvalidOperation("Autostart flag must be true or 
>> false")
>>           pool = self.get_storagepool(name, self.conn)
>> -        if autostart:
>> -            pool.setAutostart(1)
>> -        else:
>> -            pool.setAutostart(0)
>> +        if 'autostart' in params:

>> +            if params['autostart'] not in [True, False]:

After updating the API.json you can remove the above verification as 
jsonschema will care about it

>> +                raise InvalidOperation("Autostart flag must be true 
>> or false")
>> +            if params['autostart']:
>> +                pool.setAutostart(1)
>> +            else:
>> +                pool.setAutostart(0)
> do you need to check this pool is a logical pool here as you docs says.
>

Good point. =)

> * disks: Adds one or more disks to the pool (for 'logical' pool only)
>
>
>> +        if 'disks' in params:
>> +            self._update_lvm_disks(name, params['disks'])
>>           ident = pool.name()
>>           return ident
>>
>
>