Re: [PATCH master 06/23] Fixup hypervisor queries in node query

[Iustin Pop]

On Wed, Oct 10, 2012 at 11:54:48AM +0200, Michael Hanselmann wrote:
> 2012/10/5 Iustin Pop <ius...@google.com>:
> > --- a/htools/Ganeti/Query/Query.hs
> > +++ b/htools/Ganeti/Query/Query.hs
> > @@ -103,7 +103,9 @@ maybeCollectLiveData False _ nodes =
> >
> >  maybeCollectLiveData True cfg nodes = do
> >    let vgs = [clusterVolumeGroupName $ configCluster cfg]
> > -      hvs = clusterEnabledHypervisors $ configCluster cfg
> > +      hvs = case clusterEnabledHypervisors $ configCluster cfg of
> > +              []  -> [XenPvm] -- this case shouldn't happen, but we handle 
> > it
> 
> Why do you hardcode XenPvm here? Shouldn't you rather raise an
> exception or have this in a global place?

Eee, I was thinking exactly about moving this into a
getDefaultHypervisor function in Config.hs, but then though "if I send
an interdiff, I will delay review even more".

Thanks for catching this; interdiff:

diff --git a/htools/Ganeti/Config.hs b/htools/Ganeti/Config.hs
index f88ba8a..55cb492 100644
--- a/htools/Ganeti/Config.hs
+++ b/htools/Ganeti/Config.hs
@@ -31,6 +31,7 @@ module Ganeti.Config
     , getNodeRole
     , getNodeNdParams
     , getDefaultNicLink
+    , getDefaultHypervisor
     , getInstancesIpByLink
     , getNode
     , getInstance
@@ -126,6 +127,16 @@ getDefaultNicLink =
   nicpLink . (M.! C.ppDefault) . fromContainer .
   clusterNicparams . configCluster
 
+-- | Returns the default cluster hypervisor.
+getDefaultHypervisor :: ConfigData -> Hypervisor
+getDefaultHypervisor cfg =
+  case clusterEnabledHypervisors $ configCluster cfg of
+    -- FIXME: this case shouldn't happen (configuration broken), but
+    -- for now we handle it here because we're not authoritative for
+    -- the config
+    []  -> XenPvm
+    x:_ -> x
+
 -- | Returns instances of a given link.
 getInstancesIpByLink :: LinkIpMap -> String -> [String]
 getInstancesIpByLink linkipmap link =
diff --git a/htools/Ganeti/Query/Query.hs b/htools/Ganeti/Query/Query.hs
index ff7d33d..7a172ff 100644
--- a/htools/Ganeti/Query/Query.hs
+++ b/htools/Ganeti/Query/Query.hs
@@ -56,6 +56,7 @@ import Data.Maybe (fromMaybe)
 import qualified Data.Map as Map
 
 import Ganeti.BasicTypes
+import Ganeti.Config
 import Ganeti.JSON
 import Ganeti.Rpc
 import Ganeti.Query.Language
@@ -103,9 +104,7 @@ maybeCollectLiveData False _ nodes =
 
 maybeCollectLiveData True cfg nodes = do
   let vgs = [clusterVolumeGroupName $ configCluster cfg]
-      hvs = case clusterEnabledHypervisors $ configCluster cfg of
-              []  -> [XenPvm] -- this case shouldn't happen, but we handle it
-              x:_ -> [x]
+      hvs = [getDefaultHypervisor cfg]
   executeRpcCall nodes (RpcCallNodeInfo vgs hvs)
 
 -- | Check whether list of queried fields contains live fields.

-- 
iustin