[Kimchi-devel] [PATCH V2] [Kimchi] s390x specific changes to support storage path and storage pool as disk.
Aline Manera
alinefm at linux.vnet.ibm.com
Fri Nov 4 20:20:11 UTC 2016
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.
> + 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.
> [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.
> + 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.update(disk)
> keys = sorted(disk_info.keys())
> if ((keys != sorted(basic_path_disk)) and
More information about the Kimchi-devel
mailing list