[Kimchi-devel] [PATCH] Fix disk format lock and checking during template creation

[Aline Manera]

On 27/04/2015 17:51, Rodrigo Trujillo wrote:
>
>
> On 04/27/2015 05:00 PM, Aline Manera wrote:
>>
>>
>> On 27/04/2015 16:23, Rodrigo Trujillo wrote:
>>> Users are able to pass the disk format (qcow, raw, etc) in disk 
>>> Template
>>> information. However, Kimchi was ignoring this information and always
>>> creating QCOW2 disk images (if the storagepool is not 'LOGICAL') 
>>> when it
>>> creates a VM based on a given Template.
>>> This patch fixes this problem and also moves the checking and disk
>>> format auto assignment to Template creation phase, in VMTemplate class
>>> init.
>>>
>>> Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo at linux.vnet.ibm.com>
>>> ---
>>>   src/kimchi/i18n.py       |  1 +
>>>   src/kimchi/vmtemplate.py | 15 +++++++++++----
>>>   2 files changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
>>> index 18e84bc..0651032 100644
>>> --- a/src/kimchi/i18n.py
>>> +++ b/src/kimchi/i18n.py
>>> @@ -154,6 +154,7 @@ messages = {
>>>       "KCHTMPL0025E": _("When specifying CPU topology, VCPUs must be 
>>> a product of sockets, cores, and threads."),
>>>       "KCHTMPL0026E": _("When specifying CPU topology, each element 
>>> must be an integer greater than zero."),
>>>       "KCHTMPL0027E": _("Invalid disk image format. Valid formats: 
>>> bochs, cloop, cow, dmg, qcow, qcow2, qed, raw, vmdk, vpc."),
>>> +    "KCHTMPL0028E": _("Invalid disk image format. Logical, SCSI and 
>>> iSCSI storage pools must use disk image format 'raw'."),
>>>
>>>       "KCHPOOL0001E": _("Storage pool %(name)s already exists"),
>>>       "KCHPOOL0002E": _("Storage pool %(name)s does not exist"),
>>> diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
>>> index ef97d83..84e877b 100644
>>> --- a/src/kimchi/vmtemplate.py
>>> +++ b/src/kimchi/vmtemplate.py
>>> @@ -75,8 +75,16 @@ class VMTemplate(object):
>>>               args['graphics'] = graphics
>>>           self.info.update(args)
>>>
>>> -        # Assign right disk format to logical and [i]scsi storagepools
>>> +        # Check disk image format passed and pool type
>>>           if self._get_storage_type() in ['logical', 'iscsi', 'scsi']:
>>> +            paramdisks = args.get('disks')
>>> +            if paramdisks is not None:
>>
>> You should use 'self.info' instead of 'args'.
>> As we plan to support change the Template defaults from a file the 
>> default information needs to be validated as well and it will be in 
>> self.info.
>>
>>
> For default disk format values, the second 'for', below, is changing 
> all disk formats to 'RAW'.

> What about write in the configuration file a comment saying that for 
> [i]scsi and logical, the default format is going to be always  'raw' ?
> Better that let user change and raise errors.

And trust user will read the comment and do the right thing...?
I don't think it is a good idea.

> Once this is finished I am going to code the support to multiple disks 
> , so this checkings are going to change anyway in a near future.
>

I don't care about future changes!
I am concerned only about the patch I am reviewing.

>>> +                for disk in paramdisks:
>>> +                    fmt = disk.get('format')
>>> +                    if fmt is not None and fmt != 'raw':
>>> +                        raise InvalidParameter('KCHTMPL0028E')
>>> +            # Finally add disk image format when not passed or
>>> +            # using default
>>>               for i, disk in enumerate(self.info['disks']):
>>>                   self.info['disks'][i]['format'] = 'raw'
>>>
>>> @@ -198,7 +206,6 @@ class VMTemplate(object):
>>>
>>>       def to_volume_list(self, vm_uuid):
>>>           storage_path = self._get_storage_path()
>>> -        fmt = 'raw' if self._get_storage_type() in ['logical'] else 
>>> 'qcow2'
>>>           ret = []
>>>           for i, d in enumerate(self.info['disks']):
>>>               index = d.get('index', i)
>>> @@ -206,11 +213,11 @@ class VMTemplate(object):
>>>
>>>               info = {'name': volume,
>>>                       'capacity': d['size'],
>>> -                    'format': fmt,
>>> +                    'format': d['format'],
>>>                       'path': '%s/%s' % (storage_path, volume)}
>>>
>>>               if 'logical' == self._get_storage_type() or \
>>> -               fmt not in ['qcow2', 'raw']:
>>> +               info['format'] not in ['qcow2', 'raw']:
>>>                   info['allocation'] = info['capacity']
>>>               else:
>>>                   info['allocation'] = 0
>>
>> _______________________________________________
>> Kimchi-devel mailing list
>> Kimchi-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>>
>
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>