Hi!
On Thu, May 16, 2013 at 10:19 PM, Bernardo Dal Seno <[email protected]>wrote: > On 16 May 2013 13:18, Helga Velroyen <[email protected]> wrote: > > This replaces the field 'vg_names' in the RPC call of 'node info' by > > 'storage_units'. Storage units are a list of tuples <storage_type,key> > > I'm confused. Are they list of tuples, or just single tuples? > A storage unit is a tuple, the list of vg names is replaced by the list of storage units. I rephrased the commit message to make it more clear: RPC 'node_info': <storage_type,key> instead of vg_names This replaces the field 'vg_names' in the RPC call of 'node info' by 'storage_units'. A storage unit is a tuple <storage_type,key> and a generalization of a vg_name. The list of vg names is replaced by a list of storage units. The modified RPC call will be used to report storage space for more than just lvm volume groups. What the 'key' is depends on the storage type. For storage type lvm-vg, the key is the volume group name. To keep backward compatibility, all functions that use the old vg_names, convert them to a list where every volume group is mapped to a tuple [('lvm-vg',volume_group)] before making the call. > > > and an abstraction of the old vg_names. It will be used to report > > storage space for more than just lvm volume groups. What the 'key' is > > depends on the storage type. For storage type lvm-vg, the key is the > > volume group name. To keep backward compatibility, all functions that > > use the old vg_names, convert them to a list where every volume group > > is mapped to a tuple [('lvm-vg',volume_group)] before making the call. > > > > Signed-off-by: Helga Velroyen <[email protected]> > > --- > > This is a resend of the patch, because QA was failing. I also integrated > > the formerly unused code. > > lib/backend.py | 47 > ++++++++++++++++++++++++++++++++++++++++++----- > > lib/cmdlib.py | 15 ++++++++++++--- > > lib/masterd/iallocator.py | 2 +- > > lib/rpc_defs.py | 5 +++-- > > lib/server/noded.py | 4 ++-- > > src/Ganeti/Query/Node.hs | 9 +++++++-- > > src/Ganeti/Rpc.hs | 4 ++-- > > 7 files changed, 69 insertions(+), 17 deletions(-) > > > > diff --git a/lib/backend.py b/lib/backend.py > > index 215fdf5..7f94559 100644 > > --- a/lib/backend.py > > +++ b/lib/backend.py > > @@ -620,11 +620,13 @@ def _GetNamedNodeInfo(names, fn): > > return map(fn, names) > > > > > > -def GetNodeInfo(vg_names, hv_names, excl_stor): > > +def GetNodeInfo(storage_units, hv_names, excl_stor): > > """Gives back a hash with different information about the node. > > > > - @type vg_names: list of string > > - @param vg_names: Names of the volume groups to ask for disk space > information > > + @type storage_units: list of pairs (string, string) > > + @param storage_units: List of pairs of storage units and storage unit > > Shouldn't s/storage units/storage types/ perhaps? And I think it's > clearer if we write something like: > > List of pairs (storage_unit, identifier) to ask for disk space > information... > Okay. > > > + identifiers to ask for disk space information. > In > > + case of lvm-vg, the identifier is the vg name. > > Please s/vg/VG/ > Okay. > > > @type hv_names: list of string > > @param hv_names: Names of the hypervisors to ask for node information > > @type excl_stor: boolean > > @@ -635,10 +637,45 @@ def GetNodeInfo(vg_names, hv_names, excl_stor): > > > > """ > > bootid = utils.ReadFile(_BOOT_ID_PATH, size=128).rstrip("\n") > > - vg_info = _GetNamedNodeInfo(vg_names, (lambda vg: _GetVgInfo(vg, > excl_stor))) > > + storage_info = _GetNamedNodeInfo( > > + storage_units, > > + (lambda storage_unit: _GetStorageInfoFunction(storage_unit[0], > > + storage_unit[1], > > + excl_stor))) > > hv_info = _GetNamedNodeInfo(hv_names, _GetHvInfo) > > > > - return (bootid, vg_info, hv_info) > > + return (bootid, storage_info, hv_info) > > + > > + > > +def _GetStorageInfoNotImplemented(storage_key): > > + """Function to be called for storage units whose space reporting is > not > > I think you mean storage types, not storage units. > > > + implemented yet. > > + > > + """ > > + raise NotImplementedError("Storage reporting not implemented for > storage" > > + " unit '%s'." % storage_key) > > I'm afraid this error isn't very informative, as the interesting part > is storage type. I think we should use a closure whose storage type > parameter is initialized in _STORAGE_TYPE_INFO_FN, instead. > > I decided to get rid of GetStorageInfoNotImplemented and just let GetStorageInfoFunction handle the case when it is not implemented. It seems to be too much overhead for just displaying the not-implemented-message. > + > > + > > +# FIXME: implement storage reporting for all missing storage types. > > +_STORAGE_TYPE_INFO_FN = { > > + constants.ST_BLOCK: _GetStorageInfoNotImplemented, > > + constants.ST_DISKLESS: _GetStorageInfoNotImplemented, > > + constants.ST_EXT: _GetStorageInfoNotImplemented, > > + constants.ST_FILE: _GetStorageInfoNotImplemented, > > + constants.ST_LVM_VG: _GetVgInfo, > > + constants.ST_RADOS: _GetStorageInfoNotImplemented, > > +} > > + > > + > > +def _GetStorageInfoFunction(storage_type, *args): > > + """Looks up the correct function to calculate free and total storage > for the > > + given storage type. > > + > > + @type storage_type: string > > + @param storage_type: the storage type for which the storage shall be > reported. > > Please also document args and the return value. > > Sure. > > > diff --git a/src/Ganeti/Query/Node.hs b/src/Ganeti/Query/Node.hs > > index 293ceb7..be63587 100644 > > --- a/src/Ganeti/Query/Node.hs > > +++ b/src/Ganeti/Query/Node.hs > > @@ -232,7 +235,9 @@ collectLiveData True cfg nodes = do > > (nodeName n, ndpExclusiveStorage ndp) : em) > > Nothing -> (n : bn, gn, em) > > (bnodes, gnodes, emap) = foldr step ([], [], []) nodes > > - rpcres <- executeRpcCall gnodes (RpcCallNodeInfo vgs hvs > (Map.fromList emap)) > > - -- FIXME: The order of nodes in the result could be different from > the input > > + rpcres <- executeRpcCall gnodes (RpcCallNodeInfo storage_units hvs > > + (Map.fromList emap)) > > + -- FIXME (2.8): The order of nodes in the result could be different > from the > > + -- input > > The change to the comment should go to stable-2.8, not to master. > > Okay. > > Rest LGTM, thanks. > > Bernardo >
