Il 20/ago/2014 16:52 "Aaron Karper" <[email protected]> ha scritto:
>
> On Wed, Aug 20, 2014 at 04:33:47PM +0200, Michele Tartara wrote:
> > 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 :-)
>
> Oh, indeed. Sorry!
>
> - Implements 870: inst-status-xen should not be shown for KVM cluster.
> + Implements Issue 870: inst-status-xen should not be shown for KVM
cluster.
>
> >
> > >
> > > > >
> > > > > 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?
>
> The problem wouldn't be the tests actually, we could use either here,
> but the Python generation of the constants would fail. Unfortunately
> that means that the names must be defined in src/Ganeti/Constants.hs and
> the dependencies mean that I can't use src/Ganeti/DataCollectors.hs to
> grab the names.
I've just seen the second patch and now I understand why you need the
python constant.
So, another idea: the aim is, if at all possible, not to define the names
twice. So what about just defining them once, in the list you introduced,
and use them from the collectors when defining the dcName field of each of
them?
>
> > > >
> > > > > + 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
Thanks,
Michele
