On Thu, Mar 08, 2012 at 08:29:46AM +0000, Guido Trotter wrote: > 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?
Let me try to explain in words what is happening, and you can then suggest a better name: - at first, we have old_pnode on which the instance lives, and new_pnode on which the instance will live - we "remove" the instance from its old_pnode, which gives us the new version of the old_pnode, which I called old_pnode' - we "add" the instance to its new_pnode, which gives us the new version of new_pnode, called new_pnode' I thought of calling them old_pnode, old_pnode_after_remove, new_pnode, new_pnode_after_add, but this seemed a bit "silly". I'm fine with any names, for the record, just don't know how to name them nicely. sorry! iustin > On Fri, Mar 2, 2012 at 10:40, Iustin Pop <[email protected]> wrote: > > As usual, I'm very late with the slides, so this is more of an FYI > > rather than RFC. > > > > The slides are very dense towards the end, I should probably cut some > > stuff… > On Fri, Mar 2, 2012 at 10:40, Iustin Pop <[email protected]> wrote: > > As usual, I'm very late with the slides, so this is more of an FYI > > rather than RFC. > > > > The slides are very dense towards the end, I should probably cut some > > stuff… sorry > > 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.
