On Tue, Sep 24, 2013 at 4:04 PM, Spyros Trigazis <[email protected]> wrote:
> > Στις 24 Σεπ 2013, 2:22 μ.μ., ο/η Michele Tartara <[email protected]> > έγραψε: > > On Tue, Sep 24, 2013 at 1:07 PM, Spyros Trigazis <[email protected]>wrote: > >> >> Στις 24 Σεπ 2013, 12:24 μ.μ., ο/η Michele Tartara <[email protected]> >> έγραψε: >> >> On Fri, Sep 20, 2013 at 3:14 PM, Spyros Trigazis <[email protected]>wrote: >> >>> Contact all MonDs from HTools to fetch data from its Data >>> Collectors (only CPUload Data Collector is queried at the >>> moment). This information is available to all HTools and can be >>> ignored if the --ignore-dynu option is enabled. This >>> functionality is implemented in ExtLoader.hs. >>> >> >> The semantics should be changed according to Klaus' suggestion in the >> other email thread. >> >> >> ack >> >> >> >>> >>> Signed-off-by: Spyros Trigazis <[email protected]> >>> --- >>> src/Ganeti/HTools/Cluster.hs | 15 ++++- >>> src/Ganeti/HTools/ExtLoader.hs | 129 >>> ++++++++++++++++++++++++++++++++++++- >>> src/Ganeti/HTools/Program/Hail.hs | 11 +++- >>> 3 files changed, 147 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/Ganeti/HTools/Cluster.hs b/src/Ganeti/HTools/Cluster.hs >>> index 88891a4..cf1705e 100644 >>> --- a/src/Ganeti/HTools/Cluster.hs >>> +++ b/src/Ganeti/HTools/Cluster.hs >>> @@ -496,6 +496,17 @@ applyMove nl inst (FailoverAndReplace new_sdx) = >>> new_inst, old_sdx, new_sdx) >>> in new_nl >>> >>> +-- | Update the instance utilization according to the utilization of the >>> +-- given node and the requested number of cpus. >>> +uInstUtil :: Node.Node -> Instance.Instance -> Instance.Instance >>> +uInstUtil node inst = >>> + let cpus = Node.uCpu node + Instance.vcpus inst >>> + cpuLoad = cpuWeight . Node.utilLoad $ node >>> + perCpu = cpuLoad / fromIntegral cpus >>> + load = fromIntegral (Instance.vcpus inst) * perCpu >>> + oldUtil = Instance.util inst >>> + in inst { Instance.util = oldUtil { cpuWeight = load }} >>> + >>> -- | Tries to allocate an instance on one given node. >>> allocateOnSingle :: Node.List -> Instance.Instance -> Ndx >>> -> OpResult Node.AllocElement >>> @@ -504,7 +515,7 @@ allocateOnSingle nl inst new_pdx = >>> new_inst = Instance.setBoth inst new_pdx Node.noSecondary >>> in do >>> Instance.instMatchesPolicy inst (Node.iPolicy p) (Node.exclStorage >>> p) >>> - new_p <- Node.addPri p inst >>> + new_p <- Node.addPri p (uInstUtil p inst) >>> let new_nl = Container.add new_pdx new_p nl >>> new_score = compCV new_nl >>> return (new_nl, new_inst, [new_p], new_score) >>> @@ -518,7 +529,7 @@ allocateOnPair nl inst new_pdx new_sdx = >>> in do >>> Instance.instMatchesPolicy inst (Node.iPolicy tgt_p) >>> (Node.exclStorage tgt_p) >>> - new_p <- Node.addPri tgt_p inst >>> + new_p <- Node.addPri tgt_p (uInstUtil tgt_p inst) >>> new_s <- Node.addSec tgt_s inst new_pdx >>> >> >> Why isn't the utilization information added to the secondary node as well? >> >> >> ack >> >> >> >>> let new_inst = Instance.setBoth inst new_pdx new_sdx >>> new_nl = Container.addTwo new_pdx new_p new_sdx new_s nl >>> diff --git a/src/Ganeti/HTools/ExtLoader.hs >>> b/src/Ganeti/HTools/ExtLoader.hs >>> index 488345b..d42c833 100644 >>> --- a/src/Ganeti/HTools/ExtLoader.hs >>> +++ b/src/Ganeti/HTools/ExtLoader.hs >>> @@ -27,31 +27,47 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, >>> Boston, MA >>> >>> -} >>> >>> +{-# LANGUAGE BangPatterns #-} >>> + >>> module Ganeti.HTools.ExtLoader >>> ( loadExternalData >>> , commonSuffix >>> , maybeSaveData >>> + , queryAllMonDDCs >>> ) where >>> >>> import Control.Monad >>> import Control.Exception >>> -import Data.Maybe (isJust, fromJust) >>> +import Data.Char >>> +import Data.Maybe (isJust, fromJust, catMaybes) >>> +import Network.Curl >>> import System.FilePath >>> import System.IO >>> import System.Time (getClockTime) >>> import Text.Printf (hPrintf) >>> >>> +import qualified Text.JSON as J >>> + >>> +import qualified Ganeti.Constants as C >>> +import qualified Ganeti.DataCollectors.CPUload as CPUload >>> +import qualified Ganeti.HTools.Container as Container >>> import qualified Ganeti.HTools.Backend.Luxi as Luxi >>> import qualified Ganeti.HTools.Backend.Rapi as Rapi >>> import qualified Ganeti.HTools.Backend.Simu as Simu >>> import qualified Ganeti.HTools.Backend.Text as Text >>> import qualified Ganeti.HTools.Backend.IAlloc as IAlloc >>> +import qualified Ganeti.HTools.Node as Node >>> +import qualified Ganeti.HTools.Instance as Instance >>> import Ganeti.HTools.Loader (mergeData, checkData, ClusterData(..) >>> , commonSuffix, clearDynU) >>> >>> import Ganeti.BasicTypes >>> +import Ganeti.Cpu.Types >>> +import Ganeti.DataCollectors.Types >>> import Ganeti.HTools.Types >>> import Ganeti.HTools.CLI >>> +import Ganeti.JSON >>> +import Ganeti.Logging (logWarning) >>> import Ganeti.Utils (sepSplit, tryRead, exitIfBad, exitWhen) >>> >>> -- | Error beautifier. >>> @@ -115,11 +131,12 @@ loadExternalData opts = do >>> ldresult = input_data >>= (if ignoreDynU then clearDynU else >>> return) >>> >>= mergeData eff_u exTags selInsts exInsts >>> now >>> cdata <- exitIfBad "failed to load data, aborting" ldresult >>> - let (fix_msgs, nl) = checkData (cdNodes cdata) (cdInstances cdata) >>> + cdata' <- if ignoreDynU then return cdata else queryAllMonDDCs cdata >>> >> >> I guess the change of semantic (from using MonD as the default, to using >> it only when an option is provided, will have to be performed here. >> >> >> ack >> >> >> >>> + let (fix_msgs, nl) = checkData (cdNodes cdata') (cdInstances cdata') >>> >>> unless (optVerbose opts == 0) $ maybeShowWarnings fix_msgs >>> >>> - return cdata {cdNodes = nl} >>> + return cdata' {cdNodes = nl} >>> >>> -- | Function to save the cluster data to a file. >>> maybeSaveData :: Maybe FilePath -- ^ The file prefix to save to >>> @@ -134,3 +151,109 @@ maybeSaveData (Just path) ext msg cdata = do >>> writeFile out_path adata >>> hPrintf stderr "The cluster state %s has been written to file '%s'\n" >>> msg out_path >>> + >>> +-- | Type describing a data collector basic information >>> +data DataCollector = DataCollector >>> + { dName :: String -- ^ Name of the data collector >>> + , dCategory :: Maybe DCCategory -- ^ The name of the category >>> + } >>> + >>> +-- | The actual data types for MonD's Data Collectors. >>> +data Report = CPUavgloadReport CPUavgload >>> + >>> +-- | The list of Data Collectors used by hail and hbal. >>> +collectors :: [DataCollector] >>> +collectors = >>> + [ DataCollector CPUload.dcName CPUload.dcCategory ] >>> + >>> +-- | Query all MonDs for all Data Collector. >>> +queryAllMonDDCs :: ClusterData -> IO ClusterData >>> +queryAllMonDDCs cdata = do >>> + let (ClusterData _ nl il _ _) = cdata >>> + (nl', il') <- foldM queryAllMonDs (nl, il) collectors >>> + return $ cdata {cdNodes = nl', cdInstances = il'} >>> + >>> +-- | Query all MonDs for a single Data Collector. >>> +queryAllMonDs :: (Node.List, Instance.List) -> DataCollector >>> + -> IO (Node.List, Instance.List) >>> +queryAllMonDs (nl, il) dc = do >>> + elems <- mapM (queryAMonD dc) (Container.elems nl) >>> + let elems' = catMaybes elems >>> + if length elems == length elems' >>> + then do >>> + let il' = foldl updateUtilData il elems' >>> + nl' = zip (Container.keys nl) elems' >>> + return (Container.fromList nl', il') >>> + else return (nl,il) >>> >> >> If I'm not mistaken, here you are using the data from MonD only if you >> got all of them. Which is good, but the user should also get some message >> saying that the data are not being used because they are incomplete. >> I see, in the following part of the code, that there is a message when >> there was a failure contacting a node, but that doesn't say that ALL the >> data are being ignored. >> >> >> I can add to this message that the data provided by the specified data >> collector are not going to be used at all. >> > > That would be enough. But it means that it would be there once for every > missing MonD reply. Whereas, if you add it where you check the length of > elems, it will only appear once, which seems more elegant to me. > > > ack > > > >> >> >>> + >>> +-- | Query a specified MonD for a Data Collector. >>> +fromCurl :: DataCollector -> Node.Node -> IO (Maybe DCReport) >>> +fromCurl dc node = do >>> + (code, !body) <- curlGetString (prepareUrl dc node) [] >>> + case code of >>> + CurlOK -> do >>> + case J.decodeStrict body :: J.Result DCReport of >>> + J.Ok r -> return $ Just r >>> + J.Error _ -> return Nothing >>> + _ -> do >>> + logWarning $ "Failed to contact node's " ++ Node.name node >>> + ++ " MonD for DC " ++ dName dc >>> + return Nothing >>> + >>> +-- | Return the data from correct combination of a Data Collector >>> +-- and a DCReport. >>> +mkReport :: DataCollector -> Maybe DCReport -> Maybe Report >>> +mkReport dc dcr = >>> + case dcr of >>> + Nothing -> Nothing >>> + Just dcr' -> >>> + case () of >>> + _ | CPUload.dcName == dName dc -> >>> >> >> This is a really unusual syntax. Can't you just use an "if" instead of >> "case () of _"? >> >> >> I could do that, but in this way it is easier to add more collectors: >> >> _ | FirstCollector.dcName == dName dc -> ... >> _ | SecondCollector.dcName == dName dc -> ... >> >> What do you think? >> > > Now I see your point. I agree the "if" is not the best choice. But then, > why not: > case (dName dc) of > FirstCollector.dcName -> ... > SecondCollector.dcName -> ... > > ? > > > (dName dc) is a string that needs to be matched with another, it is not > a type binding. I don't think that I can use this kind of syntax here. I > borrowed this syntax from HTools/Backend/IAlloc.hs. Should I keep it? > Oh, right! Sorry for not noticing this. Yes, keep the "case () of _" syntax. Thanks, Michele -- Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
