On Tue, Oct 15, 2013 at 9:59 AM, Jose A. Lopes <[email protected]> wrote:
> On Mon, Oct 14, 2013 at 10:25:30AM +0200, Hrvoje Ribicic wrote:
> > The final console field has been added, using the RPC calls of the
> > previous commit.
> > As this requires another RPC call, the live data retrieval functions
> > of the instance queries have been modified and refactored slightly.
>
> Prefer active voice over passive voice, for example,
>
> This patch adds the final console field...
>
> Present tense:
> s/have been/are
>
>
Add the console field
This patch adds the final console field, using the RPC calls of the
previous commit.
As this requires another RPC call, the live data retrieval functions
of the instance queries are modified and refactored slightly.
>
> > --- | The InstanceInfo structure does not provide enough information. We
> also
> > --- | need to know whether the instance information was found on the
> right node.
> > -type LiveInfo = (InstanceInfo, Bool)
> > +-- | The LiveInfo consists of two entries whose presence is independent.
>
> Empty haddock line, like so
>
> --
>
> > +-- The InstanceInfo is the live instance information, accompanied by a
> bool
> > +-- signifying if it was found on its designated primary node or not.
> > +-- The InstanceConsoleInfo describes how to connect to an instance.
>
> Quotes on types and perhaps a word on how/why/when these can be 'Nothing'.
>
>
Some of the changes have been done as a result of previous merges, so they
might not show in the interdiff.
Thus, reproducing:
-- | The LiveInfo consists of two entries whose presence is independent.
-- The 'InstanceInfo' is the live instance information, accompanied by a
bool
-- signifying if it was found on its designated primary node or not.
-- The 'InstanceConsoleInfo' describes how to connect to an instance.
-- Any combination of these may or may not be present, depending on node and
-- instance availability.
type LiveInfo = (Maybe (InstanceInfo, Bool), Maybe InstanceConsoleInfo)
>
> > +-- | Groups instance information calls heading out to the same nodes.
> > +consoleParamsToCalls :: [InstanceConsoleInfoParams]
> > + -> [(Node, RpcCallInstanceConsoleInfo)]
> > +consoleParamsToCalls params =
> > + let sortedParams = sortBy
> > + (comparing (instPrimaryNode . instConsInfoParamsInstance))
> params
> > + groupedParams = groupBy compareParamsByNode sortedParams
> > + in map (\x -> case x of
> > + [] -> error "Programmer error: group must have one or more
> members"
>
> This function 'consoleParamsToCalls' is being called from within the
> IO monad. I'm sure we can handle this error better. Perhaps
> 'exitIfBad' in the IO monad?
>
>
This is truly a case of a programmer error - the groupBy function produces
groups containing at least one element.
Haskell cannot infer this, and so it complains if the empty list case is
not handled.
The head function would perhaps be ideal, but hlint complains about it.
> > -- | Collect live data from RPC query if enabled.
> > collectLiveData :: Bool -> ConfigData -> [Instance] -> IO [(Instance,
> Runtime)]
> > @@ -754,9 +823,15 @@ collectLiveData False _ instances =
> > return $ zip instances
> > (repeat (Left $ RpcResultError "Live data disabled"))
> > collectLiveData True cfg instances = do
> > - let hvSpec = getDefaultHypervisorSpec cfg
> > - instance_nodes = nub . justOk $
> > - map (getNode cfg . instPrimaryNode) instances
> > - good_nodes = nodesWithValidConfig cfg instance_nodes
> > - rpcres <- executeRpcCall good_nodes (RpcCallAllInstancesInfo [hvSpec])
> > - return . zip instances $ map (extractLiveInfo rpcres) instances
> > + let hvSpecs = getHypervisorSpecs cfg instances
> > + instanceNodes = nub . justOk $
> > + map (getNode cfg . instPrimaryNode) instances
> > + goodNodes = nodesWithValidConfig cfg instanceNodes
> > + instInfoRes <- executeRpcCall goodNodes
> > + (RpcCallAllInstancesInfo hvSpecs)
>
> This fits in one line.
>
> Indeed it does.
Thanks for the suggestions, interdiff is:
diff --git a/src/Ganeti/Query/Instance.hs b/src/Ganeti/Query/Instance.hs
index c07345a..11f26db 100644
--- a/src/Ganeti/Query/Instance.hs
+++ b/src/Ganeti/Query/Instance.hs
@@ -58,9 +58,11 @@ import Ganeti.Types
import Ganeti.Utils (formatOrdinal)
-- | The LiveInfo consists of two entries whose presence is independent.
--- The InstanceInfo is the live instance information, accompanied by a bool
+-- The 'InstanceInfo' is the live instance information, accompanied by a
bool
-- signifying if it was found on its designated primary node or not.
--- The InstanceConsoleInfo describes how to connect to an instance.
+-- The 'InstanceConsoleInfo' describes how to connect to an instance.
+-- Any combination of these may or may not be present, depending on node
and
+-- instance availability.
type LiveInfo = (Maybe (InstanceInfo, Bool), Maybe InstanceConsoleInfo)
-- | Runtime containing the 'LiveInfo'. See the genericQuery function in
@@ -776,8 +778,7 @@ collectLiveData liveDataEnabled cfg instances
instanceNodes = nub . justOk $
map (getNode cfg . instPrimaryNode) instances
goodNodes = nodesWithValidConfig cfg instanceNodes
- instInfoRes <- executeRpcCall goodNodes
- (RpcCallAllInstancesInfo hvSpecs)
+ instInfoRes <- executeRpcCall goodNodes (RpcCallAllInstancesInfo
hvSpecs)
consInfoRes <- case getAllConsoleParams cfg instances of
Bad _ -> return . zip goodNodes . repeat . Left $ RpcResultError
"Cannot construct parameters for console info call"
Hrvoje Ribicic
Ganeti Engineering
Google Germany GmbH
Dienerstr. 12, 80331, München
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores
Steuernummer: 48/725/00206
Umsatzsteueridentifikationsnummer: DE813741370
