Thanks for the review!

On Wed, Aug 20, 2014 at 04:59:11PM +0200, Michele Tartara wrote:
> Il 19/ago/2014 10:37 "'Aaron Karper' via ganeti-devel" <
> [email protected]> ha scritto:
> >
> > In order to modify the data collectors displayed, there are the options
> > --activate-data-collector=[inst-status-xen ..] and
> > --deactivate-data-collector=[inst-status-xen ..]
> >
> > Includes bootstrap.py and cfgupgrade.py setting data_collectors config
> > values.
> >
> > Implements 870
> 
> Actually, it's the other patch fixing the issue with the xen collector.
> This one is just an evolution of the fix. I'd remove this line.

- Implements 870

> 
> >
> > Signed-off-by: Aaron Karper <[email protected]>
> > ---
> >  lib/bootstrap.py               |  4 ++++
> >  lib/client/gnt_cluster.py      | 28 ++++++++++++++++++++++++++--
> >  lib/cmdlib/cluster.py          |  6 ++++++
> >  lib/objects.py                 |  2 +-
> >  src/Ganeti/OpCodes.hs          |  2 ++
> >  src/Ganeti/OpParams.hs         | 14 ++++++++++++++
> >  test/py/cfgupgrade_unittest.py |  7 +++++++
> >  tools/cfgupgrade               |  2 --
> >  8 files changed, 60 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/bootstrap.py b/lib/bootstrap.py
> > index 5f79dd1..3da5f78 100644
> > --- a/lib/bootstrap.py
> > +++ b/lib/bootstrap.py
> > @@ -799,6 +799,9 @@ def InitCluster(cluster_name, mac_prefix, # pylint:
> disable=R0913, R0914
> >    if compression_tools is not None:
> >      cluster.CheckCompressionTools(compression_tools)
> >
> > +  data_collectors = dict((name, dict(active=True)) for name in
> > +          constants.DATA_COLLECTOR_NAMES)
> > +
> >    # init of cluster config file
> >    cluster_config = objects.Cluster(
> >      serial_no=1,
> > @@ -828,6 +831,7 @@ def InitCluster(cluster_name, mac_prefix, # pylint:
> disable=R0913, R0914
> >      ctime=now,
> >      mtime=now,
> >      maintain_node_health=maintain_node_health,
> > +    data_collectors=data_collectors,
> >      drbd_usermode_helper=drbd_helper,
> >      default_iallocator=default_iallocator,
> >      default_iallocator_params=default_iallocator_params,
> > diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
> > index 655cb43..67cab0a 100644
> > --- a/lib/client/gnt_cluster.py
> > +++ b/lib/client/gnt_cluster.py
> > @@ -75,6 +75,16 @@ TO_OPT = cli_option("--to", default=None,
> type="string",
> >  RESUME_OPT = cli_option("--resume", default=False, action="store_true",
> >                          help="Resume any pending Ganeti upgrades")
> >
> > +ACTIVATE_DATA_COLLECTOR = cli_option(
> > +    "--activate-data-collector",
> > +    action="append", choices=list(constants.DATA_COLLECTOR_NAMES),
> > +    help="Reactivate a data collector for reporting.")
> > +
> > +DEACTIVATE_DATA_COLLECTOR = cli_option(
> > +    "--deactivate-data-collector",
> > +    action="append", choices=list(constants.DATA_COLLECTOR_NAMES),
> > +    help="Deactivate a data collector for reporting.")
> 
> In similar situations we tend to use enable/disable instead of
> activate/deactivate. Could you please rename them, for consistency?

Done, the changes are in the patch attached.

> > +
> >  _EPO_PING_INTERVAL = 30 # 30 seconds between pings
> >  _EPO_PING_TIMEOUT = 1 # 1 second
> >  _EPO_REACHABLE_TIMEOUT = 15 * 60 # 15 minutes
> > @@ -1173,7 +1183,9 @@ def SetClusterParams(opts, args):
> >            opts.shared_file_storage_dir is not None or
> >            opts.compression_tools is not None or
> >            opts.shared_file_storage_dir is not None or
> > -          opts.enabled_user_shutdown is not None):
> > +          opts.enabled_user_shutdown is not None or
> > +          opts.activate_data_collector is not None or
> > +          opts.deactivate_data_collector is not None):
> >      ToStderr("Please give at least one of the parameters.")
> >      return 1
> >
> > @@ -1256,6 +1268,14 @@ def SetClusterParams(opts, args):
> >
> >    compression_tools = _GetCompressionTools(opts)
> >
> > +  activate_dcs = set(opts.activate_data_collector or [])
> > +  deactivate_dcs = set(opts.deactivate_data_collector or [])
> > +  activate_and_deactivate = activate_dcs & deactivate_dcs
> > +  if activate_and_deactivate:
> > +    ToStderr("Can't activate and deactivate %s at the same time." %
> > +             sorted(activate_and_deactivate))
> > +    return 1
> > +
> >    op = opcodes.OpClusterSetParams(
> >      vg_name=vg_name,
> >      drbd_helper=drbd_helper,
> > @@ -1294,6 +1314,8 @@ def SetClusterParams(opts, args):
> >      shared_file_storage_dir=opts.shared_file_storage_dir,
> >      compression_tools=compression_tools,
> >      enabled_user_shutdown=opts.enabled_user_shutdown,
> > +    activate_data_collectors=list(activate_dcs),
> > +    deactivate_data_collectors=list(deactivate_dcs),
> >      )
> >    return base.GetResult(None, opts, SubmitOrSend(op, opts))
> >
> > @@ -2279,7 +2301,9 @@ commands = {
> >        ENABLED_USER_SHUTDOWN_OPT] +
> >       INSTANCE_POLICY_OPTS +
> >       [GLOBAL_FILEDIR_OPT, GLOBAL_SHARED_FILEDIR_OPT, ZEROING_IMAGE_OPT,
> > -      COMPRESSION_TOOLS_OPT],
> > +      COMPRESSION_TOOLS_OPT] +
> > +     [ACTIVATE_DATA_COLLECTOR,
> > +      DEACTIVATE_DATA_COLLECTOR],
> >      "[opts...]",
> >      "Alters the parameters of the cluster"),
> >    "renew-crypto": (
> > diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
> > index 10c9d8c..2ddd81d 100644
> > --- a/lib/cmdlib/cluster.py
> > +++ b/lib/cmdlib/cluster.py
> > @@ -1559,6 +1559,12 @@ class LUClusterSetParams(LogicalUnit):
> >
> >      ensure_kvmd = False
> >
> > +    for name in self.op.activate_data_collectors:
> > +      self.cluster.data_collectors[name]["active"] = True
> 
> "Active" here is like a magic number. Please add a constant for it and
> substitute all of its uses.

Done.

> 
> > +
> > +    for name in self.op.deactivate_data_collectors:
> > +      self.cluster.data_collectors[name]["active"] = False
> > +
> >      if self.op.hvparams:
> >        self.cluster.hvparams = self.new_hvparams
> >      if self.op.os_hvp:
> > diff --git a/lib/objects.py b/lib/objects.py
> > index 2fa72a0..02ea411 100644
> > --- a/lib/objects.py
> > +++ b/lib/objects.py
> > @@ -407,7 +407,6 @@ class ConfigData(ConfigObject):
> >      "networks",
> >      "disks",
> >      "serial_no",
> > -    "datacollectors",
> >      ] + _TIMESTAMPS
> >
> >    def ToDict(self, _with_private=False):
> > @@ -1618,6 +1617,7 @@ class Cluster(TaggableObject):
> >      "zeroing_image",
> >      "compression_tools",
> >      "enabled_user_shutdown",
> > +    "data_collectors",
> >      ] + _TIMESTAMPS + _UUID
> >
> >    def UpgradeConfig(self):
> > diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs
> > index 635cd1b..858d629 100644
> > --- a/src/Ganeti/OpCodes.hs
> > +++ b/src/Ganeti/OpCodes.hs
> > @@ -246,6 +246,8 @@ $(genOpCode "OpCode"
> >       , pZeroingImage
> >       , pCompressionTools
> >       , pEnabledUserShutdown
> > +     , pActivateDataCollectors
> > +     , pDeactivateDataCollectors
> >       ],
> >       [])
> >    , ("OpClusterRedistConf",
> > diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs
> > index 413fae9..83255d4 100644
> > --- a/src/Ganeti/OpParams.hs
> > +++ b/src/Ganeti/OpParams.hs
> > @@ -282,6 +282,8 @@ module Ganeti.OpParams
> >    , pEnabledDiskTemplates
> >    , pEnabledUserShutdown
> >    , pAdminStateSource
> > +  , pActivateDataCollectors
> > +  , pDeactivateDataCollectors
> >    ) where
> >
> >  import Control.Monad (liftM, mplus)
> > @@ -1829,3 +1831,15 @@ pNetworkVlan :: Field
> >  pNetworkVlan =
> >    withDoc "Network vlan when connecting to a group" .
> >    defaultField [| "" |] $ stringField "network_vlan"
> > +
> > +pActivateDataCollectors :: Field
> > +pActivateDataCollectors =
> > +  withDoc "Reactivate the data collectors" .
> > +  defaultField [| emptyListSet |] $
> > +  simpleField "activate_data_collectors" [t| ListSet String |]
> > +
> > +pDeactivateDataCollectors :: Field
> > +pDeactivateDataCollectors =
> > +  withDoc "Deactivate the data collectors" .
> > +  defaultField [| emptyListSet |] $
> > +  simpleField "deactivate_data_collectors" [t| ListSet String |]
> > diff --git a/test/py/cfgupgrade_unittest.py
> b/test/py/cfgupgrade_unittest.py
> > index 3f8c3cf..b61967e 100755
> > --- a/test/py/cfgupgrade_unittest.py
> > +++ b/test/py/cfgupgrade_unittest.py
> > @@ -54,6 +54,13 @@ def GetMinimalConfig():
> >        "zeroing_image": "",
> >        "compression_tools": constants.IEC_DEFAULT_TOOLS,
> >        "enabled_user_shutdown": False,
> > +      "data_collectors": {
> > +        "diskstats": { "active": True },
> > +        "drbd": { "active": True },
> > +        "lv": { "active": True },
> > +        "inst-status-xen": { "active": True },
> > +        "cpu-avg-load": { "active": True },
> > +      },
> >      },
> >      "instances": {},
> >      "disks": {},
> > diff --git a/tools/cfgupgrade b/tools/cfgupgrade
> > index b97ebd4..78e26d9 100755
> > --- a/tools/cfgupgrade
> > +++ b/tools/cfgupgrade
> > @@ -437,8 +437,6 @@ def UpgradeTopLevelDisks(config_data):
> >      iobj["disks"] = disk_uuids
> >
> >
> > -
> > -
> >  def UpgradeAll(config_data):
> >    config_data["version"] = version.BuildVersion(TARGET_MAJOR,
> TARGET_MINOR, 0)
> >    UpgradeRapiUsers()
> > --
> > 2.1.0.rc2.206.gedb03e5
> >
> 
> Thanks,
> Michele
>From 4040b2563154a0c15d1fd9889c45f7368b4b1900 Mon Sep 17 00:00:00 2001
Message-Id: <4040b2563154a0c15d1fd9889c45f7368b4b1900.1408615091.git.akar...@google.com>
In-Reply-To: <cage_vqvagwu0yq5ue89oirkmxeadwbr_m-qpddyaqn4dw6k...@mail.gmail.com>
References: <cage_vqvagwu0yq5ue89oirkmxeadwbr_m-qpddyaqn4dw6k...@mail.gmail.com>
From: Aaron Karper <[email protected]>
Date: Tue, 12 Aug 2014 15:04:19 +0200
Subject: [PATCH master] Add `gnt-cluster modify --[dis|en]able-data-collector`

In order to modify the data collectors displayed, there are the options
--enable-data-collector=[inst-status-xen ..] and
--disable-data-collector=[inst-status-xen ..]

Includes bootstrap.py and cfgupgrade.py setting data_collectors config
values.

Signed-off-by: Aaron Karper <[email protected]>
---
 lib/bootstrap.py               |  4 ++++
 lib/client/gnt_cluster.py      | 28 ++++++++++++++++++++++++++--
 lib/cmdlib/cluster.py          |  7 +++++++
 lib/objects.py                 |  2 +-
 src/Ganeti/Constants.hs        |  3 +++
 src/Ganeti/OpCodes.hs          |  2 ++
 src/Ganeti/OpParams.hs         | 14 ++++++++++++++
 test/py/cfgupgrade_unittest.py |  7 +++++++
 tools/cfgupgrade               |  2 --
 9 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/lib/bootstrap.py b/lib/bootstrap.py
index 5f79dd1..3da5f78 100644
--- a/lib/bootstrap.py
+++ b/lib/bootstrap.py
@@ -799,6 +799,9 @@ def InitCluster(cluster_name, mac_prefix, # pylint: disable=R0913, R0914
   if compression_tools is not None:
     cluster.CheckCompressionTools(compression_tools)
 
+  data_collectors = dict((name, dict(active=True)) for name in
+          constants.DATA_COLLECTOR_NAMES)
+
   # init of cluster config file
   cluster_config = objects.Cluster(
     serial_no=1,
@@ -828,6 +831,7 @@ def InitCluster(cluster_name, mac_prefix, # pylint: disable=R0913, R0914
     ctime=now,
     mtime=now,
     maintain_node_health=maintain_node_health,
+    data_collectors=data_collectors,
     drbd_usermode_helper=drbd_helper,
     default_iallocator=default_iallocator,
     default_iallocator_params=default_iallocator_params,
diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py
index 655cb43..69a3fdf 100644
--- a/lib/client/gnt_cluster.py
+++ b/lib/client/gnt_cluster.py
@@ -75,6 +75,16 @@ TO_OPT = cli_option("--to", default=None, type="string",
 RESUME_OPT = cli_option("--resume", default=False, action="store_true",
                         help="Resume any pending Ganeti upgrades")
 
+ENABLE_DATA_COLLECTOR = cli_option(
+    "--enable-data-collector",
+    action="append", choices=list(constants.DATA_COLLECTOR_NAMES),
+    help="Reactivate a data collector for reporting.")
+
+DISABLE_DATA_COLLECTOR = cli_option(
+    "--disable-data-collector",
+    action="append", choices=list(constants.DATA_COLLECTOR_NAMES),
+    help="Deactivate a data collector for reporting.")
+
 _EPO_PING_INTERVAL = 30 # 30 seconds between pings
 _EPO_PING_TIMEOUT = 1 # 1 second
 _EPO_REACHABLE_TIMEOUT = 15 * 60 # 15 minutes
@@ -1173,7 +1183,9 @@ def SetClusterParams(opts, args):
           opts.shared_file_storage_dir is not None or
           opts.compression_tools is not None or
           opts.shared_file_storage_dir is not None or
-          opts.enabled_user_shutdown is not None):
+          opts.enabled_user_shutdown is not None or
+          opts.enable_data_collector is not None or
+          opts.disable_data_collector is not None):
     ToStderr("Please give at least one of the parameters.")
     return 1
 
@@ -1256,6 +1268,14 @@ def SetClusterParams(opts, args):
 
   compression_tools = _GetCompressionTools(opts)
 
+  enable_dcs = set(opts.enable_data_collector or [])
+  disable_dcs = set(opts.disable_data_collector or [])
+  enable_and_disable = enable_dcs & disable_dcs
+  if enable_and_disable:
+    ToStderr("Can't enable and disable %s at the same time." %
+             sorted(enable_and_disable))
+    return 1
+
   op = opcodes.OpClusterSetParams(
     vg_name=vg_name,
     drbd_helper=drbd_helper,
@@ -1294,6 +1314,8 @@ def SetClusterParams(opts, args):
     shared_file_storage_dir=opts.shared_file_storage_dir,
     compression_tools=compression_tools,
     enabled_user_shutdown=opts.enabled_user_shutdown,
+    enable_data_collectors=list(enable_dcs),
+    disable_data_collectors=list(disable_dcs),
     )
   return base.GetResult(None, opts, SubmitOrSend(op, opts))
 
@@ -2279,7 +2301,9 @@ commands = {
       ENABLED_USER_SHUTDOWN_OPT] +
      INSTANCE_POLICY_OPTS +
      [GLOBAL_FILEDIR_OPT, GLOBAL_SHARED_FILEDIR_OPT, ZEROING_IMAGE_OPT,
-      COMPRESSION_TOOLS_OPT],
+      COMPRESSION_TOOLS_OPT] +
+     [ENABLE_DATA_COLLECTOR,
+      DISABLE_DATA_COLLECTOR],
     "[opts...]",
     "Alters the parameters of the cluster"),
   "renew-crypto": (
diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py
index 10c9d8c..915c418 100644
--- a/lib/cmdlib/cluster.py
+++ b/lib/cmdlib/cluster.py
@@ -1559,6 +1559,13 @@ class LUClusterSetParams(LogicalUnit):
 
     ensure_kvmd = False
 
+    active = constants.DATA_COLLECTOR_STATE_ACTIVE
+    for name in self.op.enable_data_collectors:
+      self.cluster.data_collectors[name][active] = True
+
+    for name in self.op.disable_data_collectors:
+      self.cluster.data_collectors[name][active] = False
+
     if self.op.hvparams:
       self.cluster.hvparams = self.new_hvparams
     if self.op.os_hvp:
diff --git a/lib/objects.py b/lib/objects.py
index 2fa72a0..02ea411 100644
--- a/lib/objects.py
+++ b/lib/objects.py
@@ -407,7 +407,6 @@ class ConfigData(ConfigObject):
     "networks",
     "disks",
     "serial_no",
-    "datacollectors",
     ] + _TIMESTAMPS
 
   def ToDict(self, _with_private=False):
@@ -1618,6 +1617,7 @@ class Cluster(TaggableObject):
     "zeroing_image",
     "compression_tools",
     "enabled_user_shutdown",
+    "data_collectors",
     ] + _TIMESTAMPS + _UUID
 
   def UpgradeConfig(self):
diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs
index eb6f4f1..474bcc1 100644
--- a/src/Ganeti/Constants.hs
+++ b/src/Ganeti/Constants.hs
@@ -5119,3 +5119,6 @@ dataCollectorNames =
                       , "inst-status-xen"
                       , "cpu-avg-load"
                       ]
+
+dataCollectorStateActive :: String
+dataCollectorStateActive = "active"
diff --git a/src/Ganeti/OpCodes.hs b/src/Ganeti/OpCodes.hs
index 635cd1b..b5b4991 100644
--- a/src/Ganeti/OpCodes.hs
+++ b/src/Ganeti/OpCodes.hs
@@ -246,6 +246,8 @@ $(genOpCode "OpCode"
      , pZeroingImage
      , pCompressionTools
      , pEnabledUserShutdown
+     , pEnableDataCollectors
+     , pDisableDataCollectors
      ],
      [])
   , ("OpClusterRedistConf",
diff --git a/src/Ganeti/OpParams.hs b/src/Ganeti/OpParams.hs
index 413fae9..2b02280 100644
--- a/src/Ganeti/OpParams.hs
+++ b/src/Ganeti/OpParams.hs
@@ -282,6 +282,8 @@ module Ganeti.OpParams
   , pEnabledDiskTemplates
   , pEnabledUserShutdown
   , pAdminStateSource
+  , pEnableDataCollectors
+  , pDisableDataCollectors
   ) where
 
 import Control.Monad (liftM, mplus)
@@ -1829,3 +1831,15 @@ pNetworkVlan :: Field
 pNetworkVlan =
   withDoc "Network vlan when connecting to a group" .
   defaultField [| "" |] $ stringField "network_vlan"
+
+pEnableDataCollectors :: Field
+pEnableDataCollectors =
+  withDoc "Reactivate the data collectors" .
+  defaultField [| emptyListSet |] $
+  simpleField "enable_data_collectors" [t| ListSet String |]
+
+pDisableDataCollectors :: Field
+pDisableDataCollectors =
+  withDoc "Deactivate the data collectors" .
+  defaultField [| emptyListSet |] $
+  simpleField "disable_data_collectors" [t| ListSet String |]
diff --git a/test/py/cfgupgrade_unittest.py b/test/py/cfgupgrade_unittest.py
index 3f8c3cf..b61967e 100755
--- a/test/py/cfgupgrade_unittest.py
+++ b/test/py/cfgupgrade_unittest.py
@@ -54,6 +54,13 @@ def GetMinimalConfig():
       "zeroing_image": "",
       "compression_tools": constants.IEC_DEFAULT_TOOLS,
       "enabled_user_shutdown": False,
+      "data_collectors": {
+        "diskstats": { "active": True },
+        "drbd": { "active": True },
+        "lv": { "active": True },
+        "inst-status-xen": { "active": True },
+        "cpu-avg-load": { "active": True },
+      },
     },
     "instances": {},
     "disks": {},
diff --git a/tools/cfgupgrade b/tools/cfgupgrade
index b97ebd4..78e26d9 100755
--- a/tools/cfgupgrade
+++ b/tools/cfgupgrade
@@ -437,8 +437,6 @@ def UpgradeTopLevelDisks(config_data):
     iobj["disks"] = disk_uuids
 
 
-
-
 def UpgradeAll(config_data):
   config_data["version"] = version.BuildVersion(TARGET_MAJOR, TARGET_MINOR, 0)
   UpgradeRapiUsers()
-- 
2.1.0.rc2.206.gedb03e5

Reply via email to