Fixes #2294 - Make boolean command line flags use Scallop's 'toggle'

changelog.md

@@ -1,3 +1,9 @@
+## Changes from 0.11.0 to 0.12.0
+
+### Fixed Issues
+
+- #2294 - Make boolean command line flags use Scallop's 'toggle'
+
 ## Changes from 0.10.0 to 0.11.0
 
 ### Breaking Changes

docs/docs/command-line-flags.md

@@ -37,7 +37,8 @@ The core functionality flags can be also set by environment variable `MARATHON_O
     Comma separated list of allowed originating domains for HTTP requests.
     The origin(s) to allow in Marathon. Not set by default.
     Examples: `"*"`, or `"http://localhost:8888, http://domain.com"`.
-* `--checkpoint` (Optional. Default: true): Enable checkpointing of tasks.
+* <span class="label label-default">v0.12.0</span> `--[disable_]checkpoint` (Optional. Default: enabled):
+    Enable checkpointing of tasks.
     Requires checkpointing enabled on slaves. Allows tasks to continue running
     during mesos-slave restarts and Marathon scheduler failover.  See the
     description of `--failover_timeout`.
@@ -50,7 +51,8 @@ The core functionality flags can be also set by environment variable `MARATHON_O
     enabled.
 * `--framework_name` (Optional. Default: marathon-VERSION): The framework name
     to register with Mesos.
-* `--ha` (Optional. Default: true): Runs Marathon in HA mode with leader election.
+* <span class="label label-default">v0.12.0</span> `--[disable_]ha` (Optional. Default: enabled):
+    Run Marathon in HA mode with leader election.
     Allows starting an arbitrary number of other Marathons but all need to be
     started in HA mode. This mode requires a running ZooKeeper. See `--master`.
 * `--hostname` (Optional. Default: hostname of machine): The advertised hostname
@@ -266,22 +268,14 @@ The Web Site flags control the behavior of Marathon's web site, including the us
     concurrent http requests, that is allowed concurrently before requests get answered directly with a
     HTTP 503 Service Temporarily Unavailable.
 
-
-
-
 ### Debug Flags
 
-* <span class="label label-default">v0.8.2</span> `--logging_level` (Optional.):
+* <span class="label label-default">v0.8.2</span> `--logging_level` (Optional):
     Set the logging level of the application.
     Use one of `off`, `fatal`, `error`, `warn`, `info`, `debug`, `trace`, `all`.
-* <span class="label label-default">Deprecated</span> `--enable_metrics` (Optional.):
-    Enable metrics for all service method calls.
-    The execution time per method is available via the metrics endpoint.
-    This option is turned on by default since version 0.10.0.
-    To turn this feature off, you can use `--disable_metrics`.
-* <span class="label label-default">v0.10.0</span> `--disable_metrics` (Optional.):
-    Metrics are enabled by default, so that the execution time per method is available via the metrics endpoint.
-    This metrics measurement can be disabled with this option.
-* <span class="label label-default">v0.8.2</span> `--enable_tracing` (Optional.):
+* <span class="label label-default">v0.12.0</span> `--[disable_]metrics` (Optional. Default: enabled):
+    Expose the execution time per method via the metrics endpoint.
+    This metrics measurement can be disabled with --disable_metrics.
+* <span class="label label-default">v0.12.0</span> `--[disable_]tracing` (Optional. Default: disabled):
     Enable tracing for all service method calls.
-    Around the execution of every service method a trace log message is issued.
+    Log a trace message around the execution of every service method.

mesos-simulation/src/test/scala/mesosphere/mesos/scale/SingleAppScalingTest.scala

@@ -39,7 +39,8 @@ class SingleAppScalingTest
     val maxTasksPerOffer = Option(System.getenv("MARATHON_MAX_TASKS_PER_OFFER")).getOrElse("1")
 
     ProcessKeeper.startMarathon(cwd, env,
-      List("--http_port", port.toString, "--zk", config.zk, "--enable_metrics",
+      List("--http_port", port.toString,
+        "--zk", config.zk,
         "--max_tasks_per_offer", maxTasksPerOffer,
         "--task_launch_timeout", "20000",
         "--task_launch_confirm_timeout", "1000") ++ args.toList,

src/main/scala/mesosphere/marathon/AllConf.scala

@@ -0,0 +1,17 @@
+package mesosphere.marathon
+
+import mesosphere.chaos.AppConfiguration
+import mesosphere.chaos.http.HttpConf
+import mesosphere.marathon.core.plugin.PluginConfiguration
+import mesosphere.marathon.event.EventConfiguration
+import mesosphere.marathon.event.http.HttpEventConfiguration
+import org.rogach.scallop.ScallopConf
+
+class AllConf(args: Seq[String] = Nil) extends ScallopConf(args)
+  with HttpConf
+  with MarathonConf
+  with AppConfiguration
+  with EventConfiguration
+  with HttpEventConfiguration
+  with DebugConf
+  with PluginConfiguration

src/main/scala/mesosphere/marathon/DebugConf.scala

@@ -11,26 +11,31 @@ import org.rogach.scallop.ScallopConf
 
 /**
   * Options related to debugging marathon.
-  * All options should be optional and turned off by default.
   */
 trait DebugConf extends ScallopConf {
 
-  lazy val debugTracing = opt[Boolean]("enable_tracing",
-    descr = "Enable trace logging of service method calls",
+  lazy val debugTracing = toggle("tracing",
+    descrYes = "Enable trace logging of service method calls",
+    descrNo = "(Default) Disable trace logging of service method calls",
     default = Some(false),
-    noshort = true)
+    noshort = true,
+    prefix = "disable_")
 
-  @deprecated("Metrics are enabled by default. See disableMetrics.", "Since version 0.10.0")
-  lazy val enableMetrics = opt[Boolean]("enable_metrics",
-    descr = "Enable metric measurement of service method calls",
-    hidden = true,
+  lazy val deprecatedDebugTracing = opt[Boolean]("enable_tracing", hidden = true)
+
+  mutuallyExclusive(debugTracing, deprecatedDebugTracing)
+  lazy val enableDebugTracing = debugTracing() || deprecatedDebugTracing()
+
+  lazy val metrics = toggle("metrics",
+    descrYes = "(Default) Enable metric measurement of service method calls",
+    descrNo = "Disable metric measurement of service method calls",
     default = Some(true),
-    noshort = true)
+    noshort = true,
+    prefix = "disable_")
 
-  lazy val disableMetrics = opt[Boolean]("disable_metrics",
-    descr = "Disable metric measurement of service method calls",
-    default = Some(false),
-    noshort = true)
+  lazy val deprecatedEnableMetrics = opt[Boolean]("enable_metrics", default = Some(false), hidden = true)
+
+  mutuallyExclusive(metrics, deprecatedEnableMetrics)
 
   lazy val logLevel = opt[String]("logging_level",
     descr = "Set logging level to one of: off, fatal, error, warn, info, debug, trace, all",
@@ -81,8 +86,8 @@ class DebugModule(conf: DebugConf) extends AbstractModule {
     //add behaviors
     val metricsProvider = getProvider(classOf[Metrics])
 
-    val tracingBehavior = conf.debugTracing.get.filter(identity).map(_ => new TracingBehavior(metricsProvider))
-    val metricsBehavior = conf.disableMetrics.get.filterNot(identity).map(_ => new MetricsBehavior(metricsProvider))
+    val tracingBehavior = if (conf.enableDebugTracing) Some(new TracingBehavior(metricsProvider)) else None
+    val metricsBehavior = conf.metrics.get.filter(identity).map(_ => new MetricsBehavior(metricsProvider))
 
     val behaviors = (tracingBehavior :: metricsBehavior :: Nil).flatten
     if (behaviors.nonEmpty) bindInterceptor(MarathonMatcher, Matchers.any(), behaviors: _*)

src/main/scala/mesosphere/marathon/MarathonConf.scala

@@ -31,17 +31,23 @@ trait MarathonConf
     descr = "The failover_timeout for mesos in seconds (default: 1 week)",
     default = Some(604800L))
 
-  lazy val highlyAvailable = opt[Boolean]("ha",
-    descr = "Runs Marathon in HA mode with leader election. " +
+  lazy val highlyAvailable = toggle("ha",
+    descrYes = "(Default) Run Marathon in HA mode with leader election. " +
       "Allows starting an arbitrary number of other Marathons but all need " +
       "to be started in HA mode. This mode requires a running ZooKeeper",
-    noshort = true, default = Some(true))
+    descrNo = "Run Marathon in single node mode.",
+    prefix = "disable_",
+    noshort = true,
+    default = Some(true))
 
-  lazy val checkpoint = opt[Boolean]("checkpoint",
-    descr = "Enable checkpointing of tasks. " +
+  lazy val checkpoint = toggle("checkpoint",
+    descrYes = "(Default) Enable checkpointing of tasks. " +
       "Requires checkpointing enabled on slaves. Allows tasks to continue " +
       "running during mesos-slave restarts and upgrades",
-    noshort = true, default = Some(true))
+    descrNo = "Disable checkpointing of tasks.",
+    prefix = "disable_",
+    noshort = true,
+    default = Some(true))
 
   lazy val localPortMin = opt[Int]("local_port_min",
     descr = "Min port number to use when assigning globally unique service ports to apps",

src/main/scala/mesosphere/marathon/core/flow/FlowModule.scala

@@ -33,17 +33,17 @@ class FlowModule(leadershipModule: LeadershipModule) {
     offersWanted: Observable[Boolean],
     driverHolder: MarathonSchedulerDriverHolder): Option[OfferReviver] = {
 
-    if (conf.shouldReviveOffersForNewApps) {
+    if (conf.reviveOffersForNewApps()) {
       lazy val reviveOffersActor = ReviveOffersActor.props(
         clock, conf, marathonEventStream,
         offersWanted, driverHolder
       )
       val actorRef = leadershipModule.startWhenLeader(reviveOffersActor, "reviveOffersWhenWanted")
-      log.info(s"Calling reviveOffers is enabled. Use --${conf.disableReviveOffersForNewApps.name} to disable.")
+      log.info(s"Calling reviveOffers is enabled. Use --disable_revive_offers_for_new_apps to disable.")
       Some(new OfferReviverDelegate(actorRef))
     }
     else {
-      log.info(s"Calling reviveOffers is disabled. Use --${conf.disableReviveOffersForNewApps.name} to enable.")
+      log.info(s"Calling reviveOffers is disabled. Use --revive_offers_for_new_apps to enable.")
       None
     }
   }

src/main/scala/mesosphere/marathon/core/flow/ReviveOffersConfig.scala

@@ -5,21 +5,12 @@ import org.rogach.scallop.ScallopConf
 trait ReviveOffersConfig extends ScallopConf {
   //scalastyle:off magic.number
 
-  /**
-    * Separate disable option for --revive_offers_for_new_apps since there doesn't seem
-    * to be a syntax to disable a flag in Scallop.
-    */
-  lazy val disableReviveOffersForNewApps = opt[Boolean]("disable_revive_offers_for_new_apps",
-    descr = "Disable reviveOffers for new or changed apps. (Default: use reviveOffers) ",
-    default = Some(false))
-
-  lazy val reviveOffersForNewApps = opt[Boolean]("revive_offers_for_new_apps",
-    descr = "Whether to call reviveOffers for new or changed apps. " +
-      "(Default: use reviveOffers, disable with --disable_revive_offers_for_new_apps) ",
+  lazy val reviveOffersForNewApps = toggle("revive_offers_for_new_apps",
+    descrYes = "(Default) Call reviveOffers for new or changed apps.",
+    descrNo = "Disable reviveOffers for new or changed apps.",
     hidden = true,
-    default = Some(true))
-
-  lazy val shouldReviveOffersForNewApps = !disableReviveOffersForNewApps() && reviveOffersForNewApps()
+    default = Some(true),
+    prefix = "disable_")
 
   lazy val minReviveOffersInterval = opt[Long]("min_revive_offers_interval",
     descr = "Do not ask for all offers (also already seen ones) more often than this interval (ms).",

src/test/scala/mesosphere/marathon/DebugConfTest.scala

@@ -0,0 +1,61 @@
+package mesosphere.marathon
+
+class DebugConfTest extends MarathonSpec {
+  test("tracing is disabled by default") {
+    val conf = defaultConfig()
+    assert(!conf.debugTracing())
+  }
+
+  test("tracing can be enabled") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--tracing"
+    )
+    assert(conf.enableDebugTracing)
+  }
+
+  test("tracing can be enabled using the deprecated ---enable_tracing") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--enable_tracing"
+    )
+    assert(conf.enableDebugTracing)
+  }
+
+  test("tracing can be disabled") {
+    val conf = makeConfig("" +
+      "--master", "127.0.0.1:5050",
+      "--disable_tracing"
+    )
+    assert(!conf.enableDebugTracing)
+  }
+
+  test("metrics are enabled by default") {
+    val conf = defaultConfig()
+    assert(conf.metrics())
+  }
+
+  test("metrics can be enabled") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--metrics"
+    )
+    assert(conf.metrics())
+  }
+
+  test("the deprecated --enable_metrics is accepted") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--enable_metrics"
+    )
+    assert(conf.metrics())
+  }
+
+  test("metrics can be disabled") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--disable_metrics"
+    )
+    assert(!conf.metrics())
+  }
+}

src/test/scala/mesosphere/marathon/MarathonConfTest.scala

@@ -38,6 +38,32 @@ class MarathonConfTest extends MarathonSpec {
     assert(conf.mesosAuthenticationSecretFile.get == Some(secretFile))
   }
 
+  test("HA mode is enabled by default") {
+    val conf = defaultConfig()
+    assert(conf.highlyAvailable())
+  }
+
+  test("Disable HA mode") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--disable_ha"
+    )
+    assert(!conf.highlyAvailable())
+  }
+
+  test("Checkpointing is enabled by default") {
+    val conf = defaultConfig()
+    assert(conf.checkpoint())
+  }
+
+  test("Disable checkpointing") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--disable_checkpoint"
+    )
+    assert(!conf.checkpoint())
+  }
+
   test("MarathonStoreTimeOut") {
     val conf = makeConfig(
       "--master", "127.0.0.1:5050",

src/test/scala/mesosphere/marathon/MarathonTestHelper.scala

@@ -12,15 +12,14 @@ import mesosphere.marathon.state.PathId._
 import mesosphere.marathon.tasks.MarathonTasks
 import mesosphere.mesos.protos._
 import org.apache.mesos.Protos.{ CommandInfo, TaskID, TaskInfo, Offer }
-import org.rogach.scallop.ScallopConf
 import play.api.libs.json.Json
 
 trait MarathonTestHelper {
 
   import mesosphere.mesos.protos.Implicits._
 
-  def makeConfig(args: String*): MarathonConf = {
-    val opts = new ScallopConf(args) with MarathonConf {
+  def makeConfig(args: String*): AllConf = {
+    val opts = new AllConf(args) {
       // scallop will trigger sys exit
       override protected def onError(e: Throwable): Unit = throw e
     }
@@ -33,7 +32,7 @@ trait MarathonTestHelper {
     minReviveOffersInterval: Long = 100,
     mesosRole: Option[String] = None,
     acceptedResourceRoles: Option[Set[String]] = None,
-    envVarsPrefix: Option[String] = None): MarathonConf = {
+    envVarsPrefix: Option[String] = None): AllConf = {
 
     var args = Seq(
       "--master", "127.0.0.1:5050",

src/test/scala/mesosphere/marathon/core/flow/ReviveOffersConfigTest.scala

@@ -0,0 +1,18 @@
+package mesosphere.marathon.core.flow
+
+import mesosphere.marathon.MarathonSpec
+
+class ReviveOffersConfigTest extends MarathonSpec {
+  test("reviveOffersForNewApps is enabled by default") {
+    val conf = defaultConfig()
+    assert(conf.reviveOffersForNewApps())
+  }
+
+  test("disable reviveOffersForNewApps") {
+    val conf = makeConfig(
+      "--master", "127.0.0.1:5050",
+      "--disable_revive_offers_for_new_apps"
+    )
+    assert(!conf.reviveOffersForNewApps())
+  }
+}