Sounds better, thanks. There's a couple of things there: what's the difference for example between new_pnode and new_pnode' ? it's not quite clear at first sight and it seems just a way to reuse the same name even if we could not... any way to clarify that and similar things?
Thanks, Guido On Wed, Mar 7, 2012 at 10:04 PM, Iustin Pop <[email protected]> wrote: > On Wed, Mar 07, 2012 at 03:07:00PM +0000, Guido Trotter wrote: >> On Tue, Mar 6, 2012 at 11:59 PM, Iustin Pop <[email protected]> wrote: >> > This brings together all previous pieces and allows balancing to work >> > (with the known caveats related to disk space calculation) for shared >> > storage disk templates. >> > --- >> > htools/Ganeti/HTools/Cluster.hs | 20 +++++++++++++++++++- >> > htools/Ganeti/HTools/Types.hs | 2 ++ >> > 2 files changed, 21 insertions(+), 1 deletions(-) >> > >> > diff --git a/htools/Ganeti/HTools/Cluster.hs >> > b/htools/Ganeti/HTools/Cluster.hs >> > index e2ec8b8..7c225f7 100644 >> > --- a/htools/Ganeti/HTools/Cluster.hs >> > +++ b/htools/Ganeti/HTools/Cluster.hs >> > @@ -391,6 +391,17 @@ applyMove nl inst Failover = >> > new_inst, old_sdx, old_pdx) >> > in new_nl >> > >> > +-- Failover to any (fa) >> > +applyMove nl inst (FailoverToAny new_pdx) = >> > + let (old_pdx, old_sdx, old_p, _) = instanceNodes nl inst >> > + tgt_n = Container.find new_pdx nl >> > + !new_nl = do -- Maybe monad >> > + new_p <- Node.addPriEx (Node.offline old_p) tgt_n inst >> > + let new_inst = Instance.setPri inst new_pdx >> > + return (Container.addTwo old_pdx (Node.removePri old_p inst) >> > + new_pdx new_p nl, new_inst, new_pdx, old_sdx) >> > + in new_nl >> > + >> >> Can we have a bit more verbose variable names and explanations here? > > Sure. I initially copied the code from the ReplaceSecondary branch, > hence these names. > > The following interdiff attempts to improve the patch, while keeping the > same overall naming as the rest of the applyMove cases (and in general > with this file, e.g. nl = node list, pdx/sdx = pri/sec node index, > etc.). Let me know what you think: > > diff --git a/htools/Ganeti/HTools/Cluster.hs b/htools/Ganeti/HTools/Cluster.hs > index 835287b..1f868f5 100644 > --- a/htools/Ganeti/HTools/Cluster.hs > +++ b/htools/Ganeti/HTools/Cluster.hs > @@ -396,15 +396,15 @@ applyMove nl inst Failover = > in new_nl > > -- Failover to any (fa) > -applyMove nl inst (FailoverToAny new_pdx) = > - let (old_pdx, old_sdx, old_p, _) = instanceNodes nl inst > - tgt_n = Container.find new_pdx nl > - !new_nl = do -- Maybe monad > - new_p <- Node.addPriEx (Node.offline old_p) tgt_n inst > - let new_inst = Instance.setPri inst new_pdx > - return (Container.addTwo old_pdx (Node.removePri old_p inst) > - new_pdx new_p nl, new_inst, new_pdx, old_sdx) > - in new_nl > +applyMove nl inst (FailoverToAny new_pdx) = do > + let (old_pdx, old_sdx, old_pnode, _) = instanceNodes nl inst > + new_pnode = Container.find new_pdx nl > + force_failover = Node.offline old_pnode > + new_pnode' <- Node.addPriEx force_failover new_pnode inst > + let old_pnode' = Node.removePri old_pnode inst > + inst' = Instance.setPri inst new_pdx > + nl' = Container.addTwo old_pdx old_pnode' new_pdx new_pnode' nl > + return (nl', inst', new_pdx, old_sdx) > > -- Replace the primary (f:, r:np, f) > applyMove nl inst (ReplacePrimary new_pdx) = > > -- > iustin -- Guido Trotter Google - Corporate Computing Services SRE Google Ireland Ltd. : Registered in Ireland with company number 368047. Gordon House, Barrow Street, Dublin 4, Ireland.
