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@linux.vnet.ibm.com wrote:
From: Archana Singh <archus@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