Il 20/ago/2014 13:14 "Aaron Karper" <[email protected]> ha scritto: > > Hi Michele, thanks for the review! I'll respond in-line with the diffs, > but I'll attach the full .patch. > > On Tue, Aug 19, 2014 at 05:40:58PM +0200, Michele Tartara wrote: > > Il 19/ago/2014 09:58 "'Aaron Karper' via ganeti-devel" < > > [email protected]> ha scritto: > > > > > > Generally a data collector can be deactivated in the config and each > > > data collector defines own conditions to show/hide it. > > > > > > A hidden data collector is not shown in /1/list/collectors, > > > /1/report/all. The relevant config is requested from the RConfD. > > > > > > * Added new constant dataCollectorNames: The consistency is checked > > > at compile time thanks to template haskell. > > > * cfgupgrade: add datacollectors section in cluster. The section > > > currently has a single entry: 'active' > > > * Added Arbitrary instances for the relevant types > > > * Move Ganeti.Monitoring.{Server,Types}.DataCollector > > > * Move {Monitoring.Server,DataCollectors.Types}.DataCollector > > > > This patch is really big. The fact that you had to describe it through a > > list gives you a hint in this direction. In this case, we prefer to send > > out a patch set. Each of the patches in a set needs to leave the codebase > > on a stable state. Partial implementation of functionality and introduction > > of temporarily unused code is OK, as long as this is plugged in before the > > end of the psych set. > > > > Having small patches, each implementing a specific piece of code, makes it > > much easier and quicker to review the code. > > > > No need to do it for this patch, but please break things down as much as > > possible in the future. > > > > > > > > Implements 870 > > > > Please, specify that it is Issue 870, otherwise it's a bit obscure what > > you're referring to. > > > > - Implements 870 > + Implements 870: inst-status-xen should not be shown for KVM cluster.
Probably I was not clear enough: I just meant "Implements Issue 870". No need to describe it :-) > > > > > > > Signed-off-by: Aaron Karper <[email protected]> > > > --- > > > Makefile.am | 1 + > > > lib/objects.py | 1 + > > > src/Ganeti/Confd/Server.hs | 9 +++ > > > src/Ganeti/Confd/Types.hs | 1 + > > > src/Ganeti/Constants.hs | 14 ++++ > > > src/Ganeti/DataCollectors.hs | 74 +++++++++++++++++++++ > > > src/Ganeti/DataCollectors/CPUload.hs | 2 +- > > > src/Ganeti/DataCollectors/Diskstats.hs | 2 +- > > > src/Ganeti/DataCollectors/Drbd.hs | 2 +- > > > src/Ganeti/DataCollectors/InstStatus.hs | 2 +- > > > src/Ganeti/DataCollectors/Lv.hs | 2 +- > > > src/Ganeti/DataCollectors/Types.hs | 21 ++++++ > > > src/Ganeti/HTools/ExtLoader.hs | 2 +- > > > src/Ganeti/JSON.hs | 4 ++ > > > src/Ganeti/Monitoring/Server.hs | 91 ++++++++++++-------------- > > > src/Ganeti/Objects.hs | 110 > > ++++++++++++++++++-------------- > > > src/Ganeti/Query/Server.hs | 2 + > > > src/ganeti-mond.hs | 22 +++++-- > > > test/hs/Test/Ganeti/Objects.hs | 9 +++ > > > test/hs/Test/Ganeti/OpCodes.hs | 9 +++ > > > tools/cfgupgrade | 11 ++++ > > > 21 files changed, 282 insertions(+), 109 deletions(-) > > > create mode 100644 src/Ganeti/DataCollectors.hs > > > > > > diff --git a/Makefile.am b/Makefile.am > > > index 34d4156..f50eea1 100644 > > > --- a/Makefile.am > > > +++ b/Makefile.am > > > @@ -790,6 +790,7 @@ HS_LIB_SRCS = \ > > > src/Ganeti/Curl/Multi.hs \ > > > src/Ganeti/Daemon.hs \ > > > src/Ganeti/Daemon/Utils.hs \ > > > + src/Ganeti/DataCollectors.hs \ > > > src/Ganeti/DataCollectors/CLI.hs \ > > > src/Ganeti/DataCollectors/CPUload.hs \ > > > src/Ganeti/DataCollectors/Diskstats.hs \ > > > diff --git a/lib/objects.py b/lib/objects.py > > > index 553c5a8..2fa72a0 100644 > > > --- a/lib/objects.py > > > +++ b/lib/objects.py > > > @@ -407,6 +407,7 @@ class ConfigData(ConfigObject): > > > "networks", > > > "disks", > > > "serial_no", > > > + "datacollectors", > > > ] + _TIMESTAMPS > > > > > > def ToDict(self, _with_private=False): > > > diff --git a/src/Ganeti/Confd/Server.hs b/src/Ganeti/Confd/Server.hs > > > index 3fbdc26..892a7ed 100644 > > > --- a/src/Ganeti/Confd/Server.hs > > > +++ b/src/Ganeti/Confd/Server.hs > > > @@ -55,6 +55,8 @@ import Ganeti.Logging > > > import qualified Ganeti.Constants as C > > > import qualified Ganeti.Query.Cluster as QCluster > > > import Ganeti.Utils > > > +import Ganeti.DataCollectors.Types (DataCollector(..)) > > > +import Ganeti.DataCollectors (collectors) > > > > > > -- * Types and constants definitions > > > > > > @@ -245,6 +247,13 @@ buildResponse cdata req@(ConfdRequest { confdRqType > > = ReqConfigQuery > > > J.Ok jsvalue -> return (ReplyStatusOk, jsvalue) > > > J.Error _ -> return queryArgumentError > > > > > > +-- | Return activation state of data collectors > > The comment describes what the function does, so it should be third person > > singular: "Returns...". > > Also, missing full stop at the end of the sentence. > > > > --- | Return activation state of data collectors > +-- | Returns activation state of data collectors. > > > > +buildResponse (cdata,_) (ConfdRequest { confdRqType = ReqDataCollectors > > }) = do > > > + let mkConfig col = > > > + (dcName col, DataCollectorConfig $ dcActive col col cdata) > > > + datacollectors = containerFromList $ map mkConfig collectors > > > + return (ReplyStatusOk, J.showJSON datacollectors) > > > + > > > -- | Creates a ConfdReply from a given answer. > > > serializeResponse :: Result StatusAnswer -> ConfdReply > > > serializeResponse r = > > > diff --git a/src/Ganeti/Confd/Types.hs b/src/Ganeti/Confd/Types.hs > > > index 8eed390..33eab6b 100644 > > > --- a/src/Ganeti/Confd/Types.hs > > > +++ b/src/Ganeti/Confd/Types.hs > > > @@ -65,6 +65,7 @@ $(declareILADT "ConfdRequestType" > > > , ("ReqNodeInstances", 8) > > > , ("ReqInstanceDisks", 9) > > > , ("ReqConfigQuery", 10) > > > + , ("ReqDataCollectors", 11) > > > ]) > > > $(makeJSONInstance ''ConfdRequestType) > > > > > > diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs > > > index eb33ba8..eb6f4f1 100644 > > > --- a/src/Ganeti/Constants.hs > > > +++ b/src/Ganeti/Constants.hs > > > @@ -4220,6 +4220,9 @@ confdReqInstanceDisks = Types.confdRequestTypeToRaw > > ReqInstanceDisks > > > confdReqConfigQuery :: Int > > > confdReqConfigQuery = Types.confdRequestTypeToRaw ReqConfigQuery > > > > > > +confdReqDataCollectors :: Int > > > +confdReqDataCollectors = Types.confdRequestTypeToRaw ReqDataCollectors > > > + > > > confdReqs :: FrozenSet Int > > > confdReqs = > > > ConstantUtils.mkSet . > > > @@ -5105,3 +5108,14 @@ ipv4NetworkMinSize = 30 > > > -- for performance tuning was successful. > > > ipv4NetworkMaxSize :: Int > > > ipv4NetworkMaxSize = 30 > > > + > > > +-- * Data Collectors > > > + > > > +dataCollectorNames :: FrozenSet String > > > +dataCollectorNames = > > > + ConstantUtils.mkSet [ "diskstats" > > > + , "drbd" > > > + , "lv" > > > + , "inst-status-xen" > > > + , "cpu-avg-load" > > > + ] > > > diff --git a/src/Ganeti/DataCollectors.hs b/src/Ganeti/DataCollectors.hs > > > new file mode 100644 > > > index 0000000..062a36b > > > --- /dev/null > > > +++ b/src/Ganeti/DataCollectors.hs > > > @@ -0,0 +1,74 @@ > > > +{-| Definition of the data collectors used by MonD. > > > + > > > +-} > > > + > > > +{- > > > + > > > +Copyright (C) 2014 Google Inc. > > > + > > > +This program is free software; you can redistribute it and/or modify > > > +it under the terms of the GNU General Public License as published by > > > +the Free Software Foundation; either version 2 of the License, or > > > +(at your option) any later version. > > > + > > > +This program is distributed in the hope that it will be useful, but > > > +WITHOUT ANY WARRANTY; without even the implied warranty of > > > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > +General Public License for more details. > > > + > > > +You should have received a copy of the GNU General Public License > > > +along with this program; if not, write to the Free Software > > > +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > > +02110-1301, USA. > > > + > > > +-} > > > + > > > +module Ganeti.DataCollectors( collectors ) where > > > + > > > +import Data.Map (findWithDefault) > > > +import Data.Monoid (mempty) > > > + > > > +import qualified Ganeti.DataCollectors.CPUload as CPUload > > > +import qualified Ganeti.DataCollectors.Diskstats as Diskstats > > > +import qualified Ganeti.DataCollectors.Drbd as Drbd > > > +import qualified Ganeti.DataCollectors.InstStatus as InstStatus > > > +import qualifiedGaneti.DataCollectors.Lvas Lv > > > +import Ganeti.DataCollectors.Types (DataCollector(..),ReportBuilder(..)) > > > +import Ganeti.JSON (GenericContainer(..)) > > > +import Ganeti.Objects > > > +import Ganeti.Types > > > + > > > +-- | The list of available builtin data collectors. > > > +collectors :: [DataCollector] > > > +collectors = > > > + [ diskStatsCollector > > > + , drdbCollector > > > + , instStatusCollector > > > + , lvCollector > > > + , cpuLoadCollector > > > + ] > > > > Given that you are refactoring this piece of code anyway, could you please > > list the collectors in alphabetical order? > > - [ diskStatsCollector > + [ cpuLoadCollector > + , diskStatsCollector > , drdbCollector > , instStatusCollector > , lvCollector > - , cpuLoadCollector > ] > > > > > > + where > > > + f .&&. g = \x y -> f x y && g x y > > > + xenHypervisor = flip elem [XenPvm, XenHvm] > > > + xenCluster _ cfg = > > > + any xenHypervisor $ clusterEnabledHypervisors $ configCluster cfg > > > + activeConfig description cfg = > > > + let config = fromContainer $ clusterDataCollectors $ configCluster > > cfg > > > + collectorConfig = findWithDefault mempty (dcName description) > > config > > > + in dataCollectorActive collectorConfig > > > + diskStatsCollector = > > > + DataCollector Diskstats.dcName Diskstats.dcCategory > > > + Diskstats.dcKind (StatelessR Diskstats.dcReport) Nothing > > activeConfig > > > + drdbCollector = > > > + DataCollector Drbd.dcName Drbd.dcCategory Drbd.dcKind > > > + (StatelessR Drbd.dcReport) Nothing activeConfig > > > + instStatusCollector = > > > + DataCollector InstStatus.dcName InstStatus.dcCategory > > > + InstStatus.dcKind (StatelessR InstStatus.dcReport) Nothing > > > + $ xenCluster .&&. activeConfig > > > + lvCollector = > > > + DataCollector Lv.dcName Lv.dcCategory Lv.dcKind > > > + (StatelessR Lv.dcReport) Nothing activeConfig > > > + cpuLoadCollector = > > > + DataCollector CPUload.dcName CPUload.dcCategory CPUload.dcKind > > > + (StatefulR CPUload.dcReport) (Just CPUload.dcUpdate) activeConfig > > > diff --git a/src/Ganeti/DataCollectors/CPUload.hs > > b/src/Ganeti/DataCollectors/CPUload.hs > > > index edcd40e..1367065 100644 > > > --- a/src/Ganeti/DataCollectors/CPUload.hs > > > +++ b/src/Ganeti/DataCollectors/CPUload.hs > > > @@ -43,7 +43,7 @@ import System.Posix.Unistd (getSysVar, > > SysVar(ClockTick)) > > > import qualified Ganeti.BasicTypes as BT > > > import qualified Ganeti.Constants as C > > > import Ganeti.Cpu.LoadParser(cpustatParser) > > > -import Ganeti.DataCollectors.Types > > > +import Ganeti.DataCollectors.Types hiding (DataCollector(..)) > > > import Ganeti.Utils > > > import Ganeti.Cpu.Types > > > > > > diff --git a/src/Ganeti/DataCollectors/Diskstats.hs > > b/src/Ganeti/DataCollectors/Diskstats.hs > > > index 92d7ac3..d9d5515 100644 > > > --- a/src/Ganeti/DataCollectors/Diskstats.hs > > > +++ b/src/Ganeti/DataCollectors/Diskstats.hs > > > @@ -48,7 +48,7 @@ import qualified Ganeti.Constants as C > > > import Ganeti.Storage.Diskstats.Parser(diskstatsParser) > > > import Ganeti.Common > > > import Ganeti.DataCollectors.CLI > > > -import Ganeti.DataCollectors.Types > > > +import Ganeti.DataCollectors.Types hiding (DataCollector(..)) > > > import Ganeti.Utils > > > > > > > > > diff --git a/src/Ganeti/DataCollectors/Drbd.hs > > b/src/Ganeti/DataCollectors/Drbd.hs > > > index 4be9ddb..5f6a8f0 100644 > > > --- a/src/Ganeti/DataCollectors/Drbd.hs > > > +++ b/src/Ganeti/DataCollectors/Drbd.hs > > > @@ -53,7 +53,7 @@ import Ganeti.Common > > > import Ganeti.Confd.Client > > > import Ganeti.Confd.Types > > > import Ganeti.DataCollectors.CLI > > > -import Ganeti.DataCollectors.Types > > > +import Ganeti.DataCollectors.Types hiding (DataCollector(..)) > > > import Ganeti.Utils > > > > > > > > > diff --git a/src/Ganeti/DataCollectors/InstStatus.hs > > b/src/Ganeti/DataCollectors/InstStatus.hs > > > index 936c5b3..ec9d9e3 100644 > > > --- a/src/Ganeti/DataCollectors/InstStatus.hs > > > +++ b/src/Ganeti/DataCollectors/InstStatus.hs > > > @@ -48,7 +48,7 @@ import Ganeti.Confd.ClientFunctions > > > import Ganeti.Common > > > import Ganeti.DataCollectors.CLI > > > import Ganeti.DataCollectors.InstStatusTypes > > > -import Ganeti.DataCollectors.Types > > > +import Ganeti.DataCollectors.Types hiding (DataCollector(..)) > > > import Ganeti.Hypervisor.Xen > > > import Ganeti.Hypervisor.Xen.Types > > > import Ganeti.Logging > > > diff --git a/src/Ganeti/DataCollectors/Lv.hs > > b/src/Ganeti/DataCollectors/Lv.hs > > > index 02232f0..9cb5513 100644 > > > --- a/src/Ganeti/DataCollectors/Lv.hs > > > +++ b/src/Ganeti/DataCollectors/Lv.hs > > > @@ -49,7 +49,7 @@ import qualified Ganeti.BasicTypes as BT > > > import Ganeti.Common > > > import Ganeti.Confd.ClientFunctions > > > import Ganeti.DataCollectors.CLI > > > -import Ganeti.DataCollectors.Types > > > +import Ganeti.DataCollectors.Types hiding (DataCollector(..)) > > > import Ganeti.JSON > > > import Ganeti.Objects > > > import Ganeti.Storage.Lvm.LVParser > > > diff --git a/src/Ganeti/DataCollectors/Types.hs > > b/src/Ganeti/DataCollectors/Types.hs > > > index 5081e39..535013b 100644 > > > --- a/src/Ganeti/DataCollectors/Types.hs > > > +++ b/src/Ganeti/DataCollectors/Types.hs > > > @@ -38,6 +38,8 @@ module Ganeti.DataCollectors.Types > > > , buildReport > > > , mergeStatuses > > > , getCategoryName > > > + , ReportBuilder(..) > > > + , DataCollector(..) > > > ) where > > > > > > import Data.Char > > > @@ -47,6 +49,7 @@ import qualified Data.Sequence as Seq > > > import Text.JSON > > > > > > import Ganeti.Constants as C > > > +import Ganeti.Objects (ConfigData) > > > import Ganeti.THH > > > import Ganeti.Utils (getCurrentTime) > > > > > > @@ -177,3 +180,21 @@ buildReport name version format_version category > > kind jsonData = do > > > return $ > > > DCReport name version format_version timestamp category kind > > > jsonData > > > + > > > +-- | A report of a data collector might be stateful or stateless. > > > +data ReportBuilder = StatelessR (IO DCReport) > > > + | StatefulR (Maybe CollectorData -> IO DCReport) > > > + > > > +-- | Type describing a data collector basic information > > > +data DataCollector = DataCollector > > > + { dcName :: String -- ^ Name of the data collector > > Why changing the names of these fields from dName etc. to dcName etc.? If I > > recall correctly the naming was due to avoiding some naming conflicts. > > Yes, this change was due to naming conflicts too. I will change it back > and fix the resulting naming conflicts in HTool. > > > > > > + , dcCategory :: Maybe DCCategory -- ^ Category (storage, instance, ecc) > > > + -- of the collector > > > + , dcKind :: DCKind -- ^ Kind (performance or status > > reporting) of > > > + -- the data collector > > > + , dcReport :: ReportBuilder -- ^ Report produced by the collector > > > + , dcUpdate :: Maybe (Maybe CollectorData -> IO CollectorData) > > > + -- ^ Update operation for stateful > > collectors. > > > + , dcActive :: DataCollector -> ConfigData -> Bool > > > > It looks like you are only using the name of the collector inside dcActive. > > Why passing the entire DataCollector, then? > > True, I'll add a descriptive type name and modify the call side > accordingly (not seen in the picture below). > > +type Name = String > > -- | Type describing a data collector basic information > data DataCollector = DataCollector > - { dcName :: String -- ^ Name of the data collector > + { dcName :: Name -- ^ Name of the data collector > , dcCategory :: Maybe DCCategory -- ^ Category (storage, instance, ecc) > -- of the collector > , dcKind :: DCKind -- ^ Kind (performance or status reporting) of > @@ -195,6 +197,6 @@ data DataCollector = DataCollector > , dcReport :: ReportBuilder -- ^ Report produced by the collector > , dcUpdate :: Maybe (Maybe CollectorData -> IO CollectorData) > -- ^ Update operation for stateful collectors. > - , dcActive :: DataCollector -> ConfigData -> Bool > - -- ^ Check if the collector applies for the cluster > + , dcActive :: Name -> ConfigData -> Bool > + -- ^ Checks if the collector applies for the cluster. > > > > > > + -- ^ Check if the collector applies for the cluster > > > + } > > > diff --git a/src/Ganeti/HTools/ExtLoader.hs > > b/src/Ganeti/HTools/ExtLoader.hs > > > index 8860a04..1a6188a 100644 > > > --- a/src/Ganeti/HTools/ExtLoader.hs > > > +++ b/src/Ganeti/HTools/ExtLoader.hs > > > @@ -65,7 +65,7 @@ import Ganeti.HTools.Loader (mergeData, checkData, > > ClusterData(..) > > > > > > import Ganeti.BasicTypes > > > import Ganeti.Cpu.Types > > > -import Ganeti.DataCollectors.Types > > > +import Ganeti.DataCollectors.Types hiding (DataCollector(..)) > > Why are all the definitions of DataCollector from Types being hidden, here > > and elsewhere? If there are so many conflicts wouldn't it be better to just > > give it a different name? > > I'll see how bad the 'd' prefix is. 'd' is taken 'dc' is taken and > 'dataCollector' is also taken unfortunately. > > > > > > import Ganeti.HTools.Types > > > import Ganeti.HTools.CLI > > > import Ganeti.JSON > > > diff --git a/src/Ganeti/JSON.hs b/src/Ganeti/JSON.hs > > > index 3df2063..dd68223 100644 > > > --- a/src/Ganeti/JSON.hs > > > +++ b/src/Ganeti/JSON.hs > > > @@ -48,6 +48,7 @@ module Ganeti.JSON > > > , toArray > > > , optionalJSField > > > , optFieldsToObj > > > + , containerFromList > > > , lookupContainer > > > , alterContainerL > > > , readContainer > > > @@ -321,6 +322,9 @@ instance F.Traversable (GenericContainer a) where > > > -- | Type alias for string keys. > > > type Container = GenericContainer String > > > > > > +containerFromList :: Ord a => [(a,b)] -> GenericContainer a b > > > +containerFromList = GenericContainer . Map.fromList > > > > The description of the function is missing. > > +-- | Creates a GenericContainer from a list of key-value pairs. > containerFromList :: Ord a => [(a,b)] -> GenericContainer a b > > > > > > -- | Looks up a value in a container with a default value. > > > -- If a key has no value, a given monadic default is returned. > > > -- This allows simple error handling, as the default can be > > > diff --git a/src/Ganeti/Monitoring/Server.hs > > b/src/Ganeti/Monitoring/Server.hs > > > index bc860bd..277d089 100644 > > > --- a/src/Ganeti/Monitoring/Server.hs > > > +++ b/src/Ganeti/Monitoring/Server.hs > > > @@ -29,12 +29,14 @@ module Ganeti.Monitoring.Server > > > ( main > > > , checkMain > > > , prepMain > > > + , DataCollector(..) > > > > If I'm not mistaken this is just a re-export of the DataCollector defined > > in Types. Do we really need this? > > No, that is just an unfortunate artefact of the refactoring effort > > , prepMain > - , DataCollector(..) > > > > > > ) where > > > > > > import Control.Applicative > > > import Control.Monad > > > import Control.Monad.IO.Class > > > import Data.ByteString.Char8 hiding (map, filter, find) > > > +import Data.Maybe (fromMaybe) > > > import Data.List > > > import qualified Data.Map as Map > > > import Snap.Core > > > @@ -43,13 +45,13 @@ import qualified Text.JSON as J > > > import Control.Concurrent > > > > > > import qualified Ganeti.BasicTypes as BT > > > +import Ganeti.Confd.Client > > > +import qualified Ganeti.Confd.Types as CT > > > import Ganeti.Daemon > > > -import qualified Ganeti.DataCollectors.CPUload as CPUload > > > -import qualified Ganeti.DataCollectors.Diskstats as Diskstats > > > -import qualified Ganeti.DataCollectors.Drbd as Drbd > > > -import qualified Ganeti.DataCollectors.InstStatus as InstStatus > > > -import qualifiedGaneti.DataCollectors.Lvas Lv > > > +import qualified Ganeti.DataCollectors as DC > > > import Ganeti.DataCollectors.Types > > > +import qualified Ganeti.JSON as GJ > > > +import Ganeti.Objects (DataCollectorConfig(..)) > > > import qualified Ganeti.Constants as C > > > import Ganeti.Runtime > > > > > > @@ -65,38 +67,6 @@ type PrepResult = Config Snap () > > > latestAPIVersion :: Int > > > latestAPIVersion = C.mondLatestApiVersion > > > > > > --- | A report of a data collector might be stateful or stateless. > > > -data Report = StatelessR (IO DCReport) > > > - | StatefulR (Maybe CollectorData -> IO DCReport) > > > - > > > --- | Type describing a data collector basic information > > > -data DataCollector = DataCollector > > > - { dName :: String -- ^ Name of the data collector > > > - , dCategory :: Maybe DCCategory -- ^ Category (storage, instance, ecc) > > > - -- of the collector > > > - , dKind :: DCKind -- ^ Kind (performance or status > > reporting) of > > > - -- the data collector > > > - , dReport :: Report -- ^ Report produced by the collector > > > - , dUpdate :: Maybe (Maybe CollectorData -> IO CollectorData) > > > - -- ^ Update operation for stateful > > collectors. > > > - } > > > - > > > - > > > --- | The list of available builtin data collectors. > > > -collectors :: [DataCollector] > > > -collectors = > > > - [ DataCollector Diskstats.dcName Diskstats.dcCategory Diskstats.dcKind > > > - (StatelessR Diskstats.dcReport) Nothing > > > - , DataCollector Drbd.dcName Drbd.dcCategory Drbd.dcKind > > > - (StatelessR Drbd.dcReport) Nothing > > > - , DataCollector InstStatus.dcName InstStatus.dcCategory > > InstStatus.dcKind > > > - (StatelessR InstStatus.dcReport) Nothing > > > - , DataCollector Lv.dcName Lv.dcCategory Lv.dcKind > > > - (StatelessR Lv.dcReport) Nothing > > > - , DataCollector CPUload.dcName CPUload.dcCategory CPUload.dcKind > > > - (StatefulR CPUload.dcReport) (Just CPUload.dcUpdate) > > > - ] > > > - > > > -- * Configuration handling > > > > > > -- | The default configuration for the HTTP server. > > > @@ -141,22 +111,41 @@ version1Api mvar = > > > , ("report", reportHandler mvar) > > > ] > > > > > > +activeCollectors :: IO [DataCollector] > > > +activeCollectors = do > > > + confdClient <- getConfdClient Nothing Nothing > > > + response <- query confdClient CT.ReqDataCollectors CT.EmptyQuery > > > + let isActive = fromMaybe True . parseActive response > > > + return $ filter isActive DC.collectors > > > + where > > > + parseActive :: Maybe CT.ConfdReply -> DataCollector -> Maybe Bool > > > + parseActive response dc = do > > > + confdReply <- response > > > + let answer = CT.confdReplyAnswer confdReply > > > + case J.readJSON answer :: J.Result (GJ.Container > > DataCollectorConfig) of > > > + J.Error _ -> Nothing > > > + J.Ok container -> do > > > + config <- GJ.lookupContainer Nothing (dcName dc) container > > > + return $ dataCollectorActive config > > > + > > > -- | Get the JSON representation of a data collector to be used in the > > collector > > > -- list. > > > dcListItem :: DataCollector -> J.JSValue > > > dcListItem dc = > > > J.JSArray > > > - [ J.showJSON $ dName dc > > > - , maybe defaultCategory J.showJSON $ dCategory dc > > > - , J.showJSON $ dKind dc > > > + [ J.showJSON $ dcName dc > > > + , maybe defaultCategory J.showJSON $ dcCategory dc > > > + , J.showJSON $ dcKind dc > > > ] > > > where > > > defaultCategory = J.showJSON C.mondDefaultCategory > > > > > > -- | Handler for returning lists. > > > listHandler :: Snap () > > > -listHandler = > > > - dir "collectors" . writeBS . pack . J.encode $ map dcListItem > > collectors > > > +listHandler = dir "collectors" $ do > > > + collectors' <- liftIO activeCollectors > > > + writeBS . pack . J.encode $ map dcListItem collectors' > > > + > > > > > > -- | Handler for returning data collector reports. > > > reportHandler :: MVar CollectorMap -> Snap () > > > @@ -170,17 +159,18 @@ reportHandler mvar = > > > -- | Return the report of all the available collectors. > > > allReports :: MVar CollectorMap -> Snap () > > > allReports mvar = do > > > - reports <- mapM (liftIO . getReport mvar) collectors > > > + collectors' <- liftIO activeCollectors > > > + reports <- mapM (liftIO . getReport mvar) collectors' > > > writeBS . pack . J.encode $ reports > > > > > > -- | Takes the CollectorMap and a DataCollector and returns the report > > for this > > > -- collector. > > > getReport :: MVar CollectorMap -> DataCollector -> IO DCReport > > > getReport mvar collector = > > > - case dReport collector of > > > + case dcReport collector of > > > StatelessR r -> r > > > StatefulR r -> do > > > - colData <- getColData (dName collector) mvar > > > + colData <- getColData (dcName collector) mvar > > > r colData > > > > > > -- | Returns the data for the corresponding collector. > > > @@ -213,6 +203,7 @@ error404 = do > > > -- | Return the report of one collector. > > > oneReport :: MVar CollectorMap -> Snap () > > > oneReport mvar = do > > > + collectors' <- liftIO activeCollectors > > > categoryName <- maybe mzero unpack <$> getParam "category" > > > collectorName <- maybe mzero unpack <$> getParam "collector" > > > category <- > > > @@ -221,8 +212,8 @@ oneReport mvar = do > > > BT.Bad msg -> fail msg > > > collector <- > > > case > > > - find (\col -> collectorName == dName col) $ > > > - filter (\c -> category == dCategory c) collectors of > > > + find (\col -> collectorName == dcName col) $ > > > + filter (\c -> category == dcCategory c) collectors' of > > > Just col -> return col > > > Nothing -> fail "Unable to find the requested collector" > > > dcr <- liftIO $ getReport mvar collector > > > @@ -239,17 +230,17 @@ monitoringApi mvar = > > > -- function. > > > collect :: CollectorMap -> DataCollector -> IO CollectorMap > > > collect m collector = > > > - case dUpdate collector of > > > + case dcUpdate collector of > > > Nothing -> return m > > > Just update -> do > > > - let name = dName collector > > > + let name = dcName collector > > > existing = Map.lookup name m > > > new_data <- update existing > > > return $ Map.insert name new_data m > > > > > > -- | Invokes collect for each data collector. > > > collection :: CollectorMap -> IO CollectorMap > > > -collection m = foldM collect m collectors > > > +collection m = liftIO activeCollectors >>= foldM collect m > > > > > > -- | The thread responsible for the periodical collection of data for > > each data > > > -- data collector. > > > diff --git a/src/Ganeti/Objects.hs b/src/Ganeti/Objects.hs > > > index fbbfc83..1f51357 100644 > > > --- a/src/Ganeti/Objects.hs > > > +++ b/src/Ganeti/Objects.hs > > > @@ -39,6 +39,7 @@ module Ganeti.Objects > > > , PartialNic(..) > > > , FileDriver(..) > > > , DRBDSecret > > > + , DataCollectorConfig(..) > > > , LogicalVolume(..) > > > , DiskLogicalId(..) > > > , Disk(..) > > > @@ -110,6 +111,7 @@ import Data.Char > > > import Data.List (foldl', isPrefixOf, isInfixOf, intercalate) > > > import Data.Maybe > > > import qualified Data.Map as Map > > > +import Data.Monoid > > > import qualified Data.Set as Set > > > import Data.Tuple (swap) > > > import Data.Word > > > @@ -312,6 +314,15 @@ $(buildObject "PartialNic" "nic" $ > > > instance UuidObject PartialNic where > > > uuidOf = nicUuid > > > > > > +-- * Datacollector definitions > > > +$(buildObject "DataCollectorConfig" "dataCollector" [ > > > + simpleField "active" [t| Bool|] > > > + ]) > > > + > > > > The comment defines a new "section" but does not describe this type. Please > > add a description for it, and for the next one too. > > > -- * Datacollector definitions > +-- | The configuration regarding a single data collector. > $(buildObject "DataCollectorConfig" "dataCollector" [ > simpleField "active" [t| Bool|] > ]) > > +-- | Central default values of the data collector config. > instance Monoid DataCollectorConfig where > mempty = DataCollectorConfig { dataCollectorActive = True } > > > > > > +instance Monoid DataCollectorConfig where > > > + mempty = DataCollectorConfig { dataCollectorActive = True } > > > + mappend _ a = a > > > + > > > -- * Disk definitions > > > > > > -- | Constant for the dev_type key entry in the disk config. > > > @@ -801,58 +812,59 @@ type HypervisorState = Container JSValue > > > > > > -- * Cluster definitions > > > $(buildObject "Cluster" "cluster" $ > > > - [ simpleField "rsahostkeypub" [t| String > > |] > > > + [ simpleField "rsahostkeypub" [t| String > > |] > > > , optionalField $ > > > - simpleField "dsahostkeypub" [t| String > > |] > > > - , simpleField "highest_used_port" [t| Int > > |] > > > - , simpleField "tcpudp_port_pool" [t| [Int] > > |] > > > - , simpleField "mac_prefix" [t| String > > |] > > > + simpleField "dsahostkeypub" [t| String > > |] > > > + , simpleField "highest_used_port" [t| Int > > |] > > > + , simpleField "tcpudp_port_pool" [t| [Int] > > |] > > > + , simpleField "mac_prefix" [t| String > > |] > > > , optionalField $ > > > - simpleField "volume_group_name" [t| String > > |] > > > - , simpleField "reserved_lvs" [t| [String] > > |] > > > + simpleField "volume_group_name" [t| String > > |] > > > + , simpleField "reserved_lvs" [t| [String] > > |] > > > , optionalField $ > > > - simpleField "drbd_usermode_helper" [t| String > > |] > > > - , simpleField "master_node" [t| String > > |] > > > - , simpleField "master_ip" [t| String > > |] > > > - , simpleField "master_netdev" [t| String > > |] > > > - , simpleField "master_netmask" [t| Int > > |] > > > - , simpleField "use_external_mip_script" [t| Bool > > |] > > > - , simpleField "cluster_name" [t| String > > |] > > > - , simpleField "file_storage_dir" [t| String > > |] > > > - , simpleField "shared_file_storage_dir" [t| String > > |] > > > - , simpleField "gluster_storage_dir" [t| String > > |] > > > - , simpleField "enabled_hypervisors" [t| [Hypervisor] > > |] > > > - , simpleField "hvparams" [t| ClusterHvParams > > |] > > > - , simpleField "os_hvp" [t| OsHvParams > > |] > > > - , simpleField "beparams" [t| ClusterBeParams > > |] > > > - , simpleField "osparams" [t| ClusterOsParams > > |] > > > - , simpleField "osparams_private_cluster" [t| > > ClusterOsParamsPrivate |] > > > - , simpleField "nicparams" [t| ClusterNicParams > > |] > > > - , simpleField "ndparams" [t| FilledNDParams > > |] > > > - , simpleField "diskparams" [t| GroupDiskParams > > |] > > > - , simpleField "candidate_pool_size" [t| Int > > |] > > > - , simpleField "modify_etc_hosts" [t| Bool > > |] > > > - , simpleField "modify_ssh_setup" [t| Bool > > |] > > > - , simpleField "maintain_node_health" [t| Bool > > |] > > > - , simpleField "uid_pool" [t| UidPool > > |] > > > - , simpleField "default_iallocator" [t| String > > |] > > > - , simpleField "default_iallocator_params" [t| IAllocatorParams > > |] > > > - , simpleField "hidden_os" [t| [String] > > |] > > > - , simpleField "blacklisted_os" [t| [String] > > |] > > > - , simpleField "primary_ip_family" [t| IpFamily > > |] > > > - , simpleField "prealloc_wipe_disks" [t| Bool > > |] > > > - , simpleField "ipolicy" [t| FilledIPolicy > > |] > > > - , simpleField "hv_state_static" [t| HypervisorState > > |] > > > - , simpleField "disk_state_static" [t| DiskState > > |] > > > - , simpleField "enabled_disk_templates" [t| [DiskTemplate] > > |] > > > - , simpleField "candidate_certs" [t| > > CandidateCertificates |] > > > - , simpleField "max_running_jobs" [t| Int > > |] > > > - , simpleField "max_tracked_jobs" [t| Int > > |] > > > - , simpleField "install_image" [t| String > > |] > > > - , simpleField "instance_communication_network" [t| String > > |] > > > - , simpleField "zeroing_image" [t| String > > |] > > > - , simpleField "compression_tools" [t| [String] > > |] > > > - , simpleField "enabled_user_shutdown" [t| Bool > > |] > > > + simpleField "drbd_usermode_helper" [t| String > > |] > > > + , simpleField "master_node" [t| String > > |] > > > + , simpleField "master_ip" [t| String > > |] > > > + , simpleField "master_netdev" [t| String > > |] > > > + , simpleField "master_netmask" [t| Int > > |] > > > + , simpleField "use_external_mip_script" [t| Bool > > |] > > > + , simpleField "cluster_name" [t| String > > |] > > > + , simpleField "file_storage_dir" [t| String > > |] > > > + , simpleField "shared_file_storage_dir" [t| String > > |] > > > + , simpleField "gluster_storage_dir" [t| String > > |] > > > + , simpleField "enabled_hypervisors" [t| [Hypervisor] > > |] > > > + , simpleField "hvparams" [t| ClusterHvParams > > |] > > > + , simpleField "os_hvp" [t| OsHvParams > > |] > > > + , simpleField "beparams" [t| ClusterBeParams > > |] > > > + , simpleField "osparams" [t| ClusterOsParams > > |] > > > + , simpleField "osparams_private_cluster" [t| > > ClusterOsParamsPrivate |] > > > + , simpleField "nicparams" [t| ClusterNicParams > > |] > > > + , simpleField "ndparams" [t| FilledNDParams > > |] > > > + , simpleField "diskparams" [t| GroupDiskParams > > |] > > > + , simpleField "candidate_pool_size" [t| Int > > |] > > > + , simpleField "modify_etc_hosts" [t| Bool > > |] > > > + , simpleField "modify_ssh_setup" [t| Bool > > |] > > > + , simpleField "maintain_node_health" [t| Bool > > |] > > > + , simpleField "uid_pool" [t| UidPool > > |] > > > + , simpleField "default_iallocator" [t| String > > |] > > > + , simpleField "default_iallocator_params" [t| IAllocatorParams > > |] > > > + , simpleField "hidden_os" [t| [String] > > |] > > > + , simpleField "blacklisted_os" [t| [String] > > |] > > > + , simpleField "primary_ip_family" [t| IpFamily > > |] > > > + , simpleField "prealloc_wipe_disks" [t| Bool > > |] > > > + , simpleField "ipolicy" [t| FilledIPolicy > > |] > > > + , simpleField "hv_state_static" [t| HypervisorState > > |] > > > + , simpleField "disk_state_static" [t| DiskState > > |] > > > + , simpleField "enabled_disk_templates" [t| [DiskTemplate] > > |] > > > + , simpleField "candidate_certs" [t| > > CandidateCertificates |] > > > + , simpleField "max_running_jobs" [t| Int > > |] > > > + , simpleField "max_tracked_jobs" [t| Int > > |] > > > + , simpleField "install_image" [t| String > > |] > > > + , simpleField "instance_communication_network" [t| String > > |] > > > + , simpleField "zeroing_image" [t| String > > |] > > > + , simpleField "compression_tools" [t| [String] > > |] > > > + , simpleField "enabled_user_shutdown" [t| Bool > > |] > > > + , simpleField "data_collectors" [t| Container > > DataCollectorConfig |] > > > ] > > > ++ timeStampFields > > > ++ uuidFields > > > diff --git a/src/Ganeti/Query/Server.hs b/src/Ganeti/Query/Server.hs > > > index d6c942d..2930aa5 100644 > > > --- a/src/Ganeti/Query/Server.hs > > > +++ b/src/Ganeti/Query/Server.hs > > > @@ -183,6 +183,8 @@ handleCall _ _ cdata QueryClusterInfo = > > > showJSON $ clusterCompressionTools cluster) > > > , ("enabled_user_shutdown", > > > showJSON $ clusterEnabledUserShutdown cluster) > > > + , ("data_collectors", > > > + showJSON $ clusterDataCollectors cluster) > > > ] > > > > > > in case master of > > > diff --git a/src/ganeti-mond.hs b/src/ganeti-mond.hs > > > index 33897df..a138841 100644 > > > --- a/src/ganeti-mond.hs > > > +++ b/src/ganeti-mond.hs > > > @@ -1,3 +1,4 @@ > > > +{-# LANGUAGE TemplateHaskell #-} > > > {-| Ganeti monitoring agent daemon > > > > > > -} > > > @@ -25,10 +26,23 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, > > Boston, MA > > > > > > module Main (main) where > > > > > > -import qualified Ganeti.Monitoring.Server > > > +import Data.List ((\\)) > > > + > > > import Ganeti.Daemon > > > +import Ganeti.DataCollectors (collectors) > > > +import Ganeti.DataCollectors.Types (dcName) > > > import Ganeti.Runtime > > > +import qualified Ganeti.Monitoring.Server as S > > > import qualified Ganeti.Constants as C > > > +import qualified Ganeti.ConstantUtils as CU > > > + > > > +$(let names = map dcName collectors > > > + missing = names \\ CU.toList C.dataCollectorNames > > > + in if missing == [] > > > + then return [] > > > + else fail $ "Please add " ++ show missing > > > + ++ " to the Ganeti.Constants.dataCollectorNames.") > > Please add a comment on top of this code describing what it does. You wrote > > it in the commit message but it should be in the code too. > > > +-- Check constistency of defined data collectors and their names used for the > +-- Python constant generation: > $(let names = map dcName collectors > > > > > > + > > > > > > -- | Options list and functions. > > > options :: [OptType] > > > @@ -44,6 +58,6 @@ options = > > > main :: IO () > > > main = > > > genericMain GanetiMond options > > > - Ganeti.Monitoring.Server.checkMain > > > - Ganeti.Monitoring.Server.prepMain > > > - Ganeti.Monitoring.Server.main > > > + S.checkMain > > > + S.prepMain > > > + S.main > > > diff --git a/test/hs/Test/Ganeti/Objects.hs > > b/test/hs/Test/Ganeti/Objects.hs > > > index 4c5da26..bd442a3 100644 > > > --- a/test/hs/Test/Ganeti/Objects.hs > > > +++ b/test/hs/Test/Ganeti/Objects.hs > > > @@ -60,6 +60,7 @@ import Test.Ganeti.TestCommon > > > import Test.Ganeti.Types () > > > > > > import qualified Ganeti.Constants as C > > > +import qualified Ganeti.ConstantUtils as CU > > > import Ganeti.Network > > > import Ganeti.Objects as Objects > > > import qualified Ganeti.Objects.BitArray as BA > > > @@ -68,6 +69,14 @@ import Ganeti.Types > > > > > > -- * Arbitrary instances > > > > > > +instance Arbitrary (Container DataCollectorConfig) where > > > + arbitrary = do > > > + let names = CU.toList C.dataCollectorNames > > > > Do we really need a manually defined ( although checked ) list of names? > > Cannot it be obtained by simply mapping dcName on the list of collectors > > that already exists? > > I thought so too, but there would be a circular dependency: > > Module imports form a cycle: > module `Ganeti.DataCollectors.InstStatusTypes' > imports `Ganeti.DataCollectors.Types' > which imports `Ganeti.Constants' > which imports `Ganeti.DataCollectors' > which imports `Ganeti.DataCollectors.InstStatus' > which imports `Ganeti.DataCollectors.InstStatusTypes' > > and also the smaller cycle Constants -> DataCollectors -> Constants. > What if you define the arbitrary instance directly in the same file of the test where you need go use it, instead of Objects.hs? Would that cause a cycle too? > > > > > + activations <- vector $ length names > > > + let configs = map DataCollectorConfig activations > > > + return GenericContainer { > > > + fromContainer = Map.fromList $ zip names configs } > > > + > > > $(genArbitrary ''PartialNDParams) > > > > > > instance Arbitrary Node where > > > diff --git a/test/hs/Test/Ganeti/OpCodes.hs > > b/test/hs/Test/Ganeti/OpCodes.hs > > > index 57e96cf..f881731 100644 > > > --- a/test/hs/Test/Ganeti/OpCodes.hs > > > +++ b/test/hs/Test/Ganeti/OpCodes.hs > > > @@ -39,6 +39,7 @@ import Control.Monad > > > import Data.Char > > > import Data.List > > > import qualified Data.Map as Map > > > +import qualified Data.Set as Set > > > import qualified Text.JSON as J > > > import Text.Printf (printf) > > > > > > @@ -50,6 +51,7 @@ import Test.Ganeti.Types () > > > > > > import Ganeti.BasicTypes > > > import qualified Ganeti.Constants as C > > > +import qualified Ganeti.ConstantUtils as CU > > > import qualified Ganeti.OpCodes as OpCodes > > > import Ganeti.Types > > > import Ganeti.OpParams > > > @@ -116,6 +118,11 @@ instance Arbitrary ExportTarget where > > > , ExportTargetRemote <$> pure [] > > > ] > > > > > > +arbitraryDataCollector :: Gen (ListSet String) > > > +arbitraryDataCollector = do > > > + els <- listOf . elements $ CU.toList C.dataCollectorNames > > > + return . ListSet $ Set.fromList els > > > + > > > instance Arbitrary OpCodes.OpCode where > > > arbitrary = do > > > op_id <- elements OpCodes.allOpIDs > > > @@ -224,6 +231,8 @@ instance Arbitrary OpCodes.OpCode where > > > <*> genMaybe (listOf genPrintableAsciiStringNE) > > > -- compression_tools > > > <*> arbitrary -- enabled_user_shutdown > > > + <*> arbitraryDataCollector -- activate_data_collectors > > > + <*> arbitraryDataCollector -- deactivate_data_collectors > > > "OP_CLUSTER_REDIST_CONF" -> pure OpCodes.OpClusterRedistConf > > > "OP_CLUSTER_ACTIVATE_MASTER_IP" -> > > > pure OpCodes.OpClusterActivateMasterIp > > > diff --git a/tools/cfgupgrade b/tools/cfgupgrade > > > index fd87a30..b97ebd4 100755 > > > --- a/tools/cfgupgrade > > > +++ b/tools/cfgupgrade > > > @@ -162,6 +162,8 @@ def UpgradeCluster(config_data): > > > cluster.get("compression_tools", constants.IEC_DEFAULT_TOOLS) > > > if "enabled_user_shutdown" not in cluster: > > > cluster["enabled_user_shutdown"] = False > > > + cluster["data_collectors"] = dict( > > > + (name, dict(active=True)) for name in constants.DATA_COLLECTOR_NAMES) > > > > > > > > > def UpgradeGroups(config_data): > > > @@ -435,6 +437,8 @@ def UpgradeTopLevelDisks(config_data): > > > iobj["disks"] = disk_uuids > > > > > > > > > + > > > + > > Please remove these two extra newlines. > > - > - > > > > > > def UpgradeAll(config_data): > > > config_data["version"] = version.BuildVersion(TARGET_MAJOR, > > TARGET_MINOR, 0) > > > UpgradeRapiUsers() > > > @@ -466,12 +470,19 @@ def DowngradeExtAccess(config_data): > > > del group_extparams["access"] > > > > > > > > > +def DowngradeDataCollectors(config_data): > > > + cluster = config_data["cluster"] > > > + if "data_collectors" in cluster: > > > + del cluster["data_collectors"] > > > + > > > + > > > def DowngradeAll(config_data): > > > # Any code specific to a particular version should be labeled that > > way, so > > > # it can be removed when updating to the next version. > > > config_data["version"] = version.BuildVersion(DOWNGRADE_MAJOR, > > > DOWNGRADE_MINOR, 0) > > > DowngradeExtAccess(config_data) > > > + DowngradeDataCollectors(config_data) > > > > > > > > > def _ParseOptions(): > > > -- > > > 2.1.0.rc2.206.gedb03e5 > > > > > > > Thanks, > > Michele Thanks, Michele
