Re: [Kimchi-devel] [PATCH V2] [Kimchi] s390x specific changes to support storage path and storage pool as disk.

On 11/04/2016 04:56 AM, archus(a)linux.vnet.ibm.com wrote: > From: Archana Singh <archus(a)linux.vnet.ibm.com&gt; > > 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