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. > > >> + >> +-- | 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 -> ... ? > > >> + case fromJVal (dcReportData dcr') :: Result CPUavgload of >> + Ok cav -> Just $ CPUavgloadReport cav >> + Bad _ -> Nothing >> + | otherwise -> Nothing >> + >> +-- | Query a MonD for a single Data Collector. >> +queryAMonD :: DataCollector -> Node.Node -> IO (Maybe Node.Node) >> +queryAMonD dc node = do >> + dcReport <- fromCurl dc node >> + case mkReport dc dcReport of >> + Nothing -> return Nothing >> + Just report -> >> + case report of >> + CPUavgloadReport cav -> >> + let ct = cavCpuTotal cav >> + du = Node.utilLoad node >> + du' = du {cpuWeight = ct} >> + in return $ Just node {Node.utilLoad = du'} >> + >> +-- | Update utilization data. >> +updateUtilData :: Instance.List -> Node.Node -> Instance.List >> +updateUtilData il node = >> + let ct = cpuWeight (Node.utilLoad node) >> + n_uCpu = Node.uCpu node >> + upd inst = >> + if Node.idx node == Instance.pNode inst >> + then >> + let i_vcpus = Instance.vcpus inst >> + i_util = ct / fromIntegral n_uCpu * fromIntegral i_vcpus >> + i_du = Instance.util inst >> + i_du' = i_du {cpuWeight = i_util} >> + in inst {Instance.util = i_du'} >> + else inst >> > > Beware the indentation. We use "then" and "else" one level more indented > than "if": > > if .... > then > ... > else > ... > > > ack > > > > >> + in Container.map upd il >> + >> +-- | Prepare url to query a single collector. >> +prepareUrl :: DataCollector -> Node.Node -> URLString >> +prepareUrl dc node = >> + Node.name node ++ ":" ++ show C.defaultMondPort ++ "/" >> + ++ "1" ++ "/report/" ++ >> + getDCCName (dCategory dc) ++ "/" ++ dName dc >> > > Don't use "1" as a "magic number". It's defined as "latestAPIVersion" in > src/Ganeti/Monitoring/Server.hs. It's better to take it from there. > If importing that file is unconvenient, just move it to the constants file > and import it from there in both places. > > > I'll move it to lib/constants.py. > > > >> + >> +-- | Get Category Name. >> +getDCCName :: Maybe DCCategory -> String >> +getDCCName dcc = >> + case dcc of >> + Nothing -> "default" >> + Just c -> map toLower . drop 2 . show $ c >> > > Duplicating the function for extracting the name of a category here is not > elegant. It's already part of the DCCategory definition, in > src/Ganeti/DataCollector/Types.hs. Currently, it's part of the JSON > instance. You can either obtain the name asking for the JSON representation > of the category (which isn't really elegant but works) or, much better, > extract the code from there as an independent function, and then use it in > the definition of showJSON and here. > > > CPUload data collector doesn't have a category. Can I still use showJSON > in order to construct the url? We need "default" as an answer. > Yes, use showJSON (or a function extracted from it) for the "Just" case, and "default" for the "Nothing" case. The important thing, in my opinion, is not to replicate the "map toLower . drop 2 . show", which, theoretically, might change, and chasing multiple versions of it around the source code would be really not nice. > > >> diff --git a/src/Ganeti/HTools/Program/Hail.hs >> b/src/Ganeti/HTools/Program/Hail.hs >> index 50009a3..507192c 100644 >> --- a/src/Ganeti/HTools/Program/Hail.hs >> +++ b/src/Ganeti/HTools/Program/Hail.hs >> @@ -39,7 +39,8 @@ import Ganeti.Common >> import Ganeti.HTools.CLI >> import Ganeti.HTools.Backend.IAlloc >> import Ganeti.HTools.Loader (Request(..), ClusterData(..)) >> -import Ganeti.HTools.ExtLoader (maybeSaveData, loadExternalData) >> +import Ganeti.HTools.ExtLoader (maybeSaveData, loadExternalData >> + , queryAllMonDDCs) >> import Ganeti.Utils >> >> -- | Options list and functions. >> @@ -51,6 +52,7 @@ options = >> , oDataFile >> , oNodeSim >> , oVerbose >> + , oIgnoreDyn >> ] >> >> -- | The list of arguments supported by the program. >> @@ -69,8 +71,11 @@ wrapReadRequest opts args = do >> cdata <- loadExternalData opts >> let Request rqt _ = r1 >> return $ Request rqt cdata >> - else return r1 >> - >> + else do >> + let Request rqt cdata = r1 >> + cdata' <- >> + if optIgnoreDynu opts then return cdata else queryAllMonDDCs >> cdata >> + return $ Request rqt cdata' >> >> -- | Main function. >> main :: Options -> [String] -> IO () >> -- >> 1.7.10.4 >> >> > Thanks, > Michele > > > Thanks, > Spyros > > > -- > 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 > > > 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
