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?
> 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...
> + identifiers to ask for disk space information. In
> + case of lvm-vg, the identifier is the vg name.
Please s/vg/VG/
> @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.
> +
> +
> +# 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.
> 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.
Rest LGTM, thanks.
Bernardo
