[Kimchi-devel] [PATCH V2] [Kimchi] s390x specific changes to support storage path and storage pool as disk.
Archana Singh
archus at linux.vnet.ibm.com
Mon Nov 7 08:46:32 UTC 2016
Hi Aline,
Please find my comment inline.
Thanks,
Archana Singh
On 11/05/2016 01:50 AM, Aline Manera wrote:
>
>
> On 11/04/2016 04:56 AM, archus at linux.vnet.ibm.com wrote:
>> From: Archana Singh <archus at linux.vnet.ibm.com>
>>
>> 1) As on s390x, default is storage path, but if pool explicitly
>> specified in
>> template.conf it takes preference. So added code to ensure that
>> if storage pool explicitly specified in file conf then check for default
>> storage pool is performed.
>> 2) Code changes in tmpl default creation which ensure that on s390x,
>> default
>> disk is path but if conf file has explicitly pool specified then
>> defult disk
>> is the sepecified pool. And also ensures that disk is either pool or
>> path
>> and not both.
>> 3) Code changes in vmtemplate, to ensure that on s390x either a path
>> disk
>> or a pool disk can be added to template. And if disk to add does not
>> have
>> both path and pool then tmpl default storage is used.
>> 4) Added 'path' specific to s390x only as commented in template.conf.
>> ---
>> model/storagepools.py | 6 ++++--
>> osinfo.py | 40 +++++++++++++++++++++++++++++++++-------
>> template.conf | 3 +++
>> vmtemplate.py | 33 ++++++++++++++++++++++++---------
>> 4 files changed, 64 insertions(+), 18 deletions(-)
>>
>> diff --git a/model/storagepools.py b/model/storagepools.py
>> index 5942b31..cc4b906 100644
>> --- a/model/storagepools.py
>> +++ b/model/storagepools.py
>> @@ -73,7 +73,9 @@ class StoragePoolsModel(object):
>> def _check_default_pools(self):
>> pools = {}
>>
>> - if is_s390x():
>> + # Don't create default pool if it's not
>> + # explicitly specified in template.conf
>> + if is_s390x() and 'pool' not in tmpl_defaults['disks'][0]:
>> return
>>
>> default_pool = tmpl_defaults['disks'][0]['pool']['name']
>> @@ -91,7 +93,7 @@ class StoragePoolsModel(object):
>> error_msg = ("Storage pool %s does not exist or is not "
>> "active. Please, check the configuration in "
>> "%s/template.conf to ensure it lists only
>> valid "
>> - "networks." % (pool_name,
>> kimchiPaths.sysconf_dir))
>> + "storage." % (pool_name,
>> kimchiPaths.sysconf_dir))
>> try:
>> pool = conn.storagePoolLookupByName(pool_name)
>> except libvirt.libvirtError, e:
>> diff --git a/osinfo.py b/osinfo.py
>> index c51d6e0..3bc59d7 100644
>> --- a/osinfo.py
>> +++ b/osinfo.py
>> @@ -27,9 +27,9 @@ from configobj import ConfigObj
>> from distutils.version import LooseVersion
>>
>> from wok.config import PluginPaths
>> +from wok.exception import InvalidParameter
>> from wok.plugins.kimchi.config import kimchiPaths
>>
>> -
>> SUPPORTED_ARCHS = {'x86': ('i386', 'i686', 'x86_64'),
>> 'power': ('ppc', 'ppc64'),
>> 'ppc64le': ('ppc64le'),
>> @@ -176,14 +176,40 @@ def _get_tmpl_defaults():
>> default_config = ConfigObj(tmpl_defaults)
>>
>> # Load template configuration file
>> - if is_on_s390x:
>> - config_file = os.path.join(
>> - kimchiPaths.sysconf_dir,
>> - 'template_s390x.conf')
>> - else:
>> - config_file = os.path.join(kimchiPaths.sysconf_dir,
>> 'template.conf')
>> + config_file = os.path.join(kimchiPaths.sysconf_dir,
>> 'template.conf')
>> config = ConfigObj(config_file)
>>
>> + # File configuration takes preference.
>> + # In s390x, file configuration can have storage pool or path.
>> + # Default configuration for s390x is storage path.
>> + # In case file conf has storage pool then storage pool takes
>> preference.
>> + # When conf file has explicitly storage pool: "defaults" should
>> + # have storage pool and default configured path should be removed,
>> + # as either storage can be path or pool, cannot be both.
>> + # When conf file does not explicity storage pool or have explicitly
>> + # storage path: "default" should have storage path only and cannot
>> + # have default pool.
>> + #
>> + # Check file conf has storage configured.
>> + if is_on_s390x and config.get('storage').get('disk.0'):
>> + # remove storage from default_config as file configuration
>> takes
>> + # preference.
>> + default_config.pop('storage')
>> +
>> + # Get storage configuration present in conf file
>> + config_pool = config.get('storage').get('disk.0').get('pool')
>> + config_path = config.get('storage').get('disk.0').get('path')
>> +
>> + # If storage configured in conf file then it should have either
>> + # pool or path.
>> + if not config_pool and not config_path:
>> + raise InvalidParameter('KCHTMPL0040E')
>> +
>
>> + # On s390x if config file has both path and pool uncommented
>> + # then path should take preference.
>> + if config_pool and config_path:
>
> Please, add a warning message to logs letting the user knows the pool
> is being ignored and only path value will be used.
> It will help on debugging if needed.
>
I will add log for same in V3 patch
>> + config.get('storage').get('disk.0').pop('pool')
>> +
>> # Merge default configuration with file configuration
>> default_config.merge(config)
>>
>> diff --git a/template.conf b/template.conf
>> index c4598f1..55f3d70 100644
>> --- a/template.conf
>> +++ b/template.conf
>> @@ -28,6 +28,9 @@
>> # Storage pool used to handle the guest disk
>> #pool = default
>
>
>> +# Only Applicable for s390x. Storage path used to handle the guest disk
>> +#path = /var/lib/libvirt/images
>> +
>
> Please, add a comment to let user knows pool and path are mutually
> exclusive parameters and pool will be ignored in case of both specified.
Will take care in V3 patch
>
>> [graphics]
>> # Graphics type
>> # Valid options: vnc | spice
>> diff --git a/vmtemplate.py b/vmtemplate.py
>> index c3390fe..b249873 100644
>> --- a/vmtemplate.py
>> +++ b/vmtemplate.py
>> @@ -106,14 +106,25 @@ class VMTemplate(object):
>>
>> for index, disk in enumerate(disks):
>> disk_info = dict(default_disk)
>> - # on s390x/s390 either pool or path should be present in
>> - # default disk.
>> - if is_s390x() and 'pool' not in default_disk and \
>> - 'path' not in default_disk:
>> - raise InvalidParameter('KCHTMPL0040E')
>>
>> - # On s390x/s390 pool is optional attribute for disk.
>> - pool = disk.get('pool', default_disk.get('pool'))
>> + if is_s390x():
>> + # Default disk should have either pool or path.
>> + if 'pool' not in default_disk and 'path' not in
>> default_disk:
>> + raise InvalidParameter('KCHTMPL0040E')
>> +
>> + # Each disk should have either pool or path.
>> + # if not then use "default_disk" configuration.
>> + pool = disk.get('pool')
>> + path = disk.get('path')
>> + if not path and not pool:
>> + # If default disk is path then set disk with
>> default path
>> + if default_disk.get('path'):
>> + path = default_disk.get('path')
>
>> + # If default disk is pool then set disk with
>> default pool
>> + if default_disk.get('pool'):
>
> it should be a 'elif' as pool will only exist with no path is there.
take care of same in V3 patch
>
>> + pool = default_disk.get('pool')
>> + else:
>> + pool = disk.get('pool', default_disk.get('pool'))
>>
>> if pool:
>> pool_type = self._get_storage_type(pool['name'])
>> @@ -148,8 +159,12 @@ class VMTemplate(object):
>> disk_info['index'] = disk_info.get('index', index)
>> self.info['disks'][index] = disk_info
>> elif is_s390x():
>> - # For now support 'path' only on s390x
>> - path = disk.get('path', default_disk.get('path'))
>
>> + # This check is required where 'path' disk
>> + # has to be added and hence default pool
>> + # has to be removed during template update.
>> + if 'pool' in disk_info:
>> + del disk_info['pool']
>> +
>
> I am not sure I agree with that.
>
> Is disk_info the information provided by user? Why are you modifying it?
disk_info is dict(default_disk).
User provided disk is "disk"
Below if entire view of elif.
User provided disk is updated in disk_info.
However the user provided disk is path and not pool so still default
pool will be present which has to be removed.
elif is_s390x():
# This check is required where 'path' disk
# has to be added and hence default pool
# has to be removed during template update.
if 'pool' in disk_info:
del disk_info['pool']
disk_info.update(disk)
keys = sorted(disk_info.keys())
if ((keys != sorted(basic_path_disk)) and
(keys != sorted(base_path_disk))):
raise MissingParameter('KCHTMPL0042E')
disk_info['path'] = path
disk_info['index'] = disk_info.get('index', index)
self.info['disks'][index] = disk_info
>
>> disk_info.update(disk)
>> keys = sorted(disk_info.keys())
>> if ((keys != sorted(basic_path_disk)) and
>
More information about the Kimchi-devel
mailing list