On Mon, Mar 12, 2012 at 11:28:42AM +0000, Guido Trotter wrote: > On Fri, Mar 9, 2012 at 6:52 PM, Iustin Pop <[email protected]> wrote: > > On Fri, Mar 09, 2012 at 03:00:18PM +0000, Guido Trotter wrote: > >> On Tue, Feb 21, 2012 at 12:21 PM, Iustin Pop <[email protected]> wrote: > >> > >> > +-- | Builds link -> ip -> instname map. > >> > +buildLinkIpInstnameMap :: ConfigData -> LinkIpMap > >> > +buildLinkIpInstnameMap cfg = > >> > + let cluster = configCluster cfg > >> > + instances = M.elems . configInstances $ cfg > >> > + defparams = (M.!) (clusterNicparams cluster) C.ppDefault > >> > + nics = concatMap (\i -> [(instName i, nic) | nic <- instNics i]) > >> > + instances > >> > + in foldl' (\accum (iname, nic) -> > >> > + let pparams = nicNicparams nic > >> > + fparams = fillNICParams defparams pparams > >> > + link = nicpLink fparams > >> > + in case nicIp nic of > >> > + Nothing -> accum > >> > + Just ip -> let oldipmap = M.findWithDefault > >> > (M.empty) > >> > + link accum > >> > + newipmap = M.insert ip iname oldipmap > >> > + in M.insert link newipmap accum > >> > + ) M.empty nics > >> > >> So, looking some more at this function: > >> > >> - could we have a way to fetch the instances from the config with > >> already filled params? This would definitely help simplifying this > >> function. > > > > Yes, and indeed! > > > >> - maybe we can abstract the [instances] to [(nic, instance_name)] part? > > > > Yep. > > > >> - maybe we can have a function from instance (or nic, instance_name) > >> to link -> (...) composed with a function from instance to ip -> > >> instance_name to simplify it a bit > > > > Possible. Not sure. > > > >> Does this make sense? Shall I give it a try? > > > > Yes, it makes sense. But don't you want to wait until we commit these? > > :) > > > > I'd prefer we commit what we have, and then iterate on it. Indeed, as > > you say, these are very hackish, but I don't think we should iterate > > out-of-tree until they are "very good"; rather, commit (but do not > > enable hconfd yet) and then improve the code until we feel it's good > > enough for release. > > > > Sounds good, but can we put at least a TODO on how we think we should > improve that function to make it more readable?
Good point! Interdiff: diff --git a/htools/Ganeti/Config.hs b/htools/Ganeti/Config.hs index 6031233..56e3dec 100644 --- a/htools/Ganeti/Config.hs +++ b/htools/Ganeti/Config.hs @@ -99,6 +99,14 @@ getInstPrimaryNode cfg name = getInstance cfg name >>= return . instPrimaryNode >>= getNode cfg -- | Builds link -> ip -> instname map. +-- +-- TODO: improve this by splitting it into multiple independent functions: +-- +-- * abstract the \"fetch instance with filled params\" functionality +-- +-- * abstsract the [instance] -> [(nic, instance_name)] part +-- +-- * etc. buildLinkIpInstnameMap :: ConfigData -> LinkIpMap buildLinkIpInstnameMap cfg = let cluster = configCluster cfg Updated patch series pushed to sandbox. thanks, iustin
