On Mon, Jun 22, 2015 at 10:29:14AM +0300, Oleg Ponomarev wrote:
> Instance pinning is introduced in ganeti locations design document.
> It adds new instance tag of form htools:desiredlocation:x where x is
> a location tag of a desired primary node. This implemented by adding
> second component to the instance locationScore.
> The metric is extended with the following component:
> * The number of instances tagged htools:desiredlocation:x where their
> primary node is not tagged with x.
>
> Signed-off-by: Oleg Ponomarev <[email protected]>
> ---
> src/Ganeti/HTools/Cluster/Moves.hs | 10 ++++++++--
> src/Ganeti/HTools/Instance.hs | 4 ++++
> src/Ganeti/HTools/Loader.hs | 21 +++++++++++++++++----
> src/Ganeti/HTools/Node.hs | 2 +-
> src/Ganeti/HTools/Tags/Constants.hs | 7 ++++++-
> 5 files changed, 36 insertions(+), 8 deletions(-)
Generally, the patch looks good; there just a few minor points.
> setInstanceLocationScore t p s =
> - t { Instance.locationScore =
> - Set.size $ Node.locationTags p `Set.intersection` Node.locationTags
> s }
> + t { Instance.locationScore =
Please do not have trailing white-space in lines you change. (Also applies
to two other places.)
> + desiredLocationScore (Instance.dsrdLocTags t) (Node.locationTags p) +
> + Set.size (Node.locationTags p `Set.intersection` Node.locationTags
> s) }
> + where desiredLocationScore instTags nodeTags
> + | Set.null instTags = 0 -- there are no desired location tags
> + | Set.null ( instTags `Set.intersection` nodeTags ) = 1
> + | otherwise = 0 -- desired location is satisfied
Are you sure that this is a correct way of saying "0 if the location
requirements are
fullfilled, one otherwise"? To me it seems correct, if there is at most one
desired location for the instance, but what wrong if there are two (like
specifying
a desired geographic location and a desired machine type in an inhomogeneous
cluster). I would have expected something along the lines of
| Set.null ( instTags Set.\\ nodeTags) = 0
| otherwise = 1
The rest looks good, but please also extend your patch series by a patch adding
a shell-test demonstrating that desired-location tags do influence the behaviour
of htools.
Thanks,
Klaus
--
Klaus Aehlig
Google Germany GmbH, Dienerstr. 12, 80331 Muenchen
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschaeftsfuehrer: Graham Law, Christine Elizabeth Flores
