Re: [Kimchi-devel] [PATCH V2 1/7] (WIP) Storagepool SCSI/FC: SCSI/Fibre Channel backend implementation

On 01/23/2014 10:29 PM, Rodrigo Trujillo wrote: > This patch creates basic functions that allow kimchi users to create > an libvirt SCSI storagepool using the rest API. > > Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo(a)linux.vnet.ibm.com&gt; > --- > docs/API.md | 6 ++++++ > src/kimchi/API.json | 19 +++++++++++++++++- > src/kimchi/model.py | 57 > ++++++++++++++++++++++++++++++++++++++++++++++++----- > 3 files changed, 76 insertions(+), 6 deletions(-) > > diff --git a/docs/API.md b/docs/API.md > index a86b884..d00cdd5 100644 > --- a/docs/API.md > +++ b/docs/API.md > @@ -242,6 +242,12 @@ Represents a snapshot of the Virtual Machine's > primary monitor. > Pool types: 'iscsi'. > * username: Login username of the iSCSI target. > * password: Login password of the iSCSI target. > + * adapter_type: Host adapter type: 'scsi_host' or 'fc_host'. > + * adapter_name: Scsi host name. > + Required if 'scsi_host' type is selected. > + * wwnn: Word Wide Node Name. > + * wwpn: Word Wide Port Name. > + From my view, only adapter_name is required to create a scsi pool. So all others should be marked as optional The user just need to provide the scsi host and kimchi will get all those info based on that

> ### Resource: Storage Pool > > diff --git a/src/kimchi/API.json b/src/kimchi/API.json > index e942824..b7eb27b 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -37,7 +37,8 @@ > "type": { > "description": "The type of the defined Storage > Pool", > "type": "string", > - "pattern": "^dir|netfs|logical|kimchi-iso$", > + "pattern": "^dir|netfs|logical|kimchi-iso|scsi$", > + "pattern": "^dir|netfs|logical|scsi$", Why are there 2 pattern entries there?

> "required": true > }, > "path": { > @@ -76,6 +77,22 @@ > "minimum": 1, > "maximum": 65535 > }, > + "adapter_type": { > + "description": "Host adapter type: > 'scsi_host' or 'fc_host'", > + "type": "string" > + }, If there are only 2 possible values you should use 'pattern' to only allow them (instead of string)

> + "adapter_name": { > + "description": "SCSI host name", > + "type": "string" > + }, > + "wwnn": { > + "description": "Fibre channel Word Wide > Node Name", > + "type": "string" > + }, > + "wwpn": { > + "description": "Fibre channel Word Wide > Port Name", > + "type": "string" > + }, > "auth": { > "description": "Storage back-end > authentication information", > "type": "object", > diff --git a/src/kimchi/model.py b/src/kimchi/model.py > index 51a4427..0eb8cc4 100644 > --- a/src/kimchi/model.py > +++ b/src/kimchi/model.py > @@ -75,8 +75,9 @@ from kimchi.objectstore import ObjectStore > from kimchi.rollbackcontext import RollbackContext > from kimchi.scan import Scanner > from kimchi.screenshot import VMScreenshot > -from kimchi.utils import get_enabled_plugins, is_digit, kimchi_log > -from kimchi.utils import run_command, parse_cmd_outpifferentut, > patch_find_nfs_target > +from kimchi.utils import get_enabled_plugins, is_digit > +from kimchi.utils import is_libvirt_version_lesser, kimchi_log, > parse_cmd_output > +from kimchi.utils import patch_find_nfs_target, run_command > from kimchi.vmtemplate import VMTemplate > > > @@ -86,7 +87,11 @@ HOST_STATS_INTERVAL = 1 > VM_STATIC_UPDATE_PARAMS = {'name': './name'} > VM_LIVE_UPDATE_PARAMS = {} > STORAGE_SOURCES = {'netfs': {'addr': '/pool/source/host/@name', > - 'path': '/pool/source/dir/@path'}} > + 'path': '/pool/source/dir/@path'}, > + 'scsi': {'adapter_type': > '/pool/source/adapter/@type', > + 'adapter_name': > '/pool/source/adapter/@name', > + 'wwnn': '/pool/source/adapter/@wwnn', > + 'wwpn': '/pool/source/adapter/@wwpn'}} > > > def _uri_to_name(collection, uri): > @@ -1095,9 +1100,10 @@ class Model(object): > return name > > pool = conn.storagePoolDefineXML(xml, 0) > - if params['type'] in ['logical', 'dir', 'netfs']: > + if params['type'] in ['logical', 'dir', 'netfs', 'scsi']: > pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW) > - # autostart dir and logical storage pool created > from kimchi > + # autostart dir, logical, netfs and scsi storage > pools created > + # from kimchi > pool.setAutostart(1) > else: > # disable autostart for others > @@ -1735,6 +1741,47 @@ class LogicalPoolDef(StoragePoolDef): > return xml > > > +class ScsiPoolDef(StoragePoolDef): > + poolType = 'scsi' > + > + def prepare(self, conn=None): > + # Adapters type are only available in libvirt >= 1.0.5 > + if is_libvirt_version_lesser('1.0.5'): As I commented in previous patch we should avoid comparing versions to decide between implementations Please, add a feature test for that.

> + self.poolArgs['source']['adapter_type'] = 'scsi_host' You are always overriding the 'adapter_type'. You should set values only if user doesn't pass it

> + tmp_name = self.poolArgs['source']['adapter_name'] > + self.poolArgs['source']['adapter_name'] = > tmp_name.replace('scsi_','') > + msg = "Libvirt version <= 1.0.5. Setting SCSI host name > as '%s'; "\ > + "setting SCSI adapter type as 'scsi_host'; "\ > + "ignoring wwnn and wwpn." %tmp_name > + kimchi_log.info(msg) > + And what about the code for the 'else' statement?

> + @property > + def xml(self): > + # Required parameters > + # name: > + # source[adapter_type]: > + # source[adapter_name]: > + # source[wwnn]: > + # source[wwpn]: > + # path: > + > + xml = """ > + <pool type='scsi'> > + <name>{name}</name> > + <source> > + <adapter type='{source[adapter_type]}'\ > + name='{source[adapter_name]}'\ > + wwnn='{source[wwnn]}'\ > + wwpn='{source[wwpn]}'/> > + </source> > + <target> > + <path>{path}</path> > + </target> > + </pool> > + """.format(**self.poolArgs) Does this xml work for all libvirt versions in supported distros? I thought the xml will be different according to libvirt support.

> + return xml > + > + > class IscsiPoolDef(StoragePoolDef): > poolType = 'iscsi' > _______________________________________________ Kimchi-devel mailing list Kimchi-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel