Re: [Kimchi-devel] [PATCH] Fix get_storageservers API and UI retrieval of storage servers.

On 04/10/2014 01:14 PM, Christy Perez wrote: > This issue was discovered when it was realized that the list of NFS > servers currently on the system was not being displayed in a drop-down > for the server IP/name while adding a new NFS server. > > The UI was calling the getstorageservers API and passing a filter of netfs, but > the StorageServers class was referencing a non-existent object. This threw an > exception that was not passed up as an error, and an empty list was returned > every time. > > Additionally, a change to the STORAGE_SOURCES dictionary was causing errors > when the StorageServersModel::lookup() tried to access a non-existent key in > in the new scsi value. This is seen if a scsi pool exists on a system. > > To allow the API to work, and sucessfully retrieve only *server* objects, this > patch makes the following changes: > > 1. Add a STORAGE_SERVERS list to storageservers.py containing only valid > server types. > The API doc states that currently only netfs is supported. > 2. Since storagetargets correlates to an individual server (see the > API doc for storagetargets) use the new STORAGE_SERVERS list instead > of the sources ones. > 3. Move the STORAGE_SOURCES dictionary into storagepools.py, since it should > cover all pool types. In the future, it should be expanded to include > iscsi, netfs, dir, scsi fc_host, etc. > 4. Updates mockmodel to incorporate the new STORAGE_SERVERS list. > > To test, create different types of pools, one nfs, and use: > > $ curl -k -u user:pass -H 'Content-type: applicatiojson' -H 'Accept: application/json' https://localhost:8001/storageservers?_target_type=netfs > > and > > $ curl -k -u user:pass -H 'Content-type: applicatiojson' -H 'Accept: application/json' https://localhost:8001/storageservers > > Or create at least one NFS pool, then when creating a second NFS pool, the > names/IPs of the others will appear in the UI's server field called > "NFS Server IP" > > Signed-off-by: Christy Perez <christy(a)linux.vnet.ibm.com&gt; > --- > src/kimchi/mockmodel.py | 5 +++-- > src/kimchi/model/storagepools.py | 3 ++- > src/kimchi/model/storageservers.py | 16 +++++++++------- > src/kimchi/model/storagetargets.py | 4 ++-- > 4 files changed, 16 insertions(+), 12 deletions(-) > > diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py > index dbdd57e..fe02abe 100644 > --- a/src/kimchi/mockmodel.py > +++ b/src/kimchi/mockmodel.py > @@ -46,7 +46,8 @@ from kimchi.config import config as kconfig > from kimchi.distroloader import DistroLoader > from kimchi.exception import InvalidOperation, InvalidParameter > from kimchi.exception import MissingParameter, NotFoundError, OperationFailed > -from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES > +from kimchi.model.storagepools import ISO_POOL_NAME > +from kimchi.model.storageservers import STORAGE_SERVERS > from kimchi.model.utils import get_vm_name > from kimchi.model.vms import VM_STATIC_UPDATE_PARAMS > from kimchi.objectstore import ObjectStore > @@ -503,7 +504,7 @@ class MockModel(object): > > def storageservers_get_list(self, _target_type=None): > # FIXME: This needs to be updted when adding new storage server support > - target_type = STORAGE_SOURCES.keys() \ > + target_type = STORAGE_SERVERS \ > if not _target_type else [_target_type] > pools = self.storagepools_get_list() > server_list = [] > diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py > index 5af33b7..0f673bb 100644 > --- a/src/kimchi/model/storagepools.py > +++ b/src/kimchi/model/storagepools.py > @@ -36,6 +36,8 @@ POOL_STATE_MAP = {0: 'inactive', > 3: 'degraded', > 4: 'inaccessible'} > > +# Types of pools supported > +# FIXME: Add 'dir', 'logical' 'iscsi', 'fc_host' ? The STORAGE_SOURCES pools are those that need a source input: server IP, adapter name, ect The dir storage pool does not need those kind of info so it should not be listed there. In docs/API.md you can see which pools have source element Otherwise, patch looks good: Reviewed-by: Aline Manera <alinefm(a)linux.vnet.ibm.com&gt; If you agree I can update/remove the above comment before applying

> STORAGE_SOURCES = {'netfs': {'addr': '/pool/source/host/@name', > 'path': '/pool/source/dir/@path'}, > 'scsi': {'adapter_type': '/pool/source/adapter/@type', > @@ -207,7 +209,6 @@ class StoragePoolModel(object): > source[key] = "" > else: > source[key] = res > - > return source > > def _nfs_status_online(self, pool, poolArgs=None): > diff --git a/src/kimchi/model/storageservers.py b/src/kimchi/model/storageservers.py > index 167fedd..c9dfb25 100644 > --- a/src/kimchi/model/storageservers.py > +++ b/src/kimchi/model/storageservers.py > @@ -18,24 +18,26 @@ > # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > > from kimchi.exception import NotFoundError > -from kimchi.model.storagepools import StoragePoolModel, STORAGE_SOURCES > +from kimchi.model.storagepools import StoragePoolModel, StoragePoolsModel > + > +# Types of remote storage servers supported > +# FIXME: Add iscsi? > +STORAGE_SERVERS = ['netfs'] > > > class StorageServersModel(object): > def __init__(self, **kargs): > self.conn = kargs['conn'] > self.pool = StoragePoolModel(**kargs) > + self.pools = StoragePoolsModel(**kargs) > > def get_list(self, _target_type=None): > if not _target_type: > - target_type = STORAGE_SOURCES.keys() > + target_type = STORAGE_SERVERS > else: > target_type = [_target_type] > - pools = self.pools.get_list() > > - conn = self.conn.get() > - pools = conn.listStoragePools() > - pools += conn.listDefinedStoragePools() > + pools = self.pools.get_list() > > server_list = [] > for pool in pools: > @@ -64,7 +66,7 @@ class StorageServerModel(object): > for pool in pools: > try: > pool_info = self.pool.lookup(pool) > - if (pool_info['source'] and > + if (pool_info['type'] in STORAGE_SERVERS and > pool_info['source']['addr'] == server): > return dict(host=server) > except NotFoundError: > diff --git a/src/kimchi/model/storagetargets.py b/src/kimchi/model/storagetargets.py > index e090d84..caa8dbe 100644 > --- a/src/kimchi/model/storagetargets.py > +++ b/src/kimchi/model/storagetargets.py > @@ -23,7 +23,7 @@ from lxml import objectify > from lxml.builder import E > > from kimchi.model.config import CapabilitiesModel > -from kimchi.model.storagepools import STORAGE_SOURCES > +from kimchi.model.storageservers import STORAGE_SERVERS > from kimchi.utils import kimchi_log, patch_find_nfs_target > > > @@ -36,7 +36,7 @@ class StorageTargetsModel(object): > target_list = list() > > if not _target_type: > - target_types = STORAGE_SOURCES.keys() > + target_types = STORAGE_SERVERS > else: > target_types = [_target_type] >