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. > > 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. > +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? > + 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. > + , 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? > + -- ^ 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? > 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. > -- | 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? > ) 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. > +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. > + > > -- | 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? > + 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
