On Mon, Mar 12, 2012 at 12:34 PM, Iustin Pop <[email protected]> wrote: > 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. >
Ok, at this point I'd say the series is ready for submission, or at least I see nothing major missing yet. Thanks, Guido
