[GitHub] markusthoemmes closed pull request #3198: move network + dns container args to pureconfig

[GitBox <gi...@apache.org>]

markusthoemmes closed pull request #3198: move network + dns container args to pureconfig
URL: https://github.com/apache/incubator-openwhisk/pull/3198
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/ansible/roles/invoker/tasks/deploy.yml b/ansible/roles/invoker/tasks/deploy.yml
index bd05da2f33..f274adcf7c 100644
--- a/ansible/roles/invoker/tasks/deploy.yml
+++ b/ansible/roles/invoker/tasks/deploy.yml
@@ -168,9 +168,11 @@
         -e DOCKER_REGISTRY='{{ docker_registry }}'
         -e DOCKER_IMAGE_PREFIX='{{ docker.image.prefix }}'
         -e DOCKER_IMAGE_TAG='{{ docker.image.tag }}'
-        -e INVOKER_CONTAINER_NETWORK='{{ invoker_container_network_name | default("bridge") }}'
+        -e CONFIG_whisk_containerFactory_containerArgs_network='{{ invoker_container_network_name | default("bridge") }}'
         -e INVOKER_CONTAINER_POLICY='{{ invoker_container_policy_name | default()}}'
-        -e INVOKER_CONTAINER_DNS='{{ invoker_container_network_dns_servers | default()}}'
+        {% for item in (invoker_container_network_dns_servers | default()).split(' ')  %}
+        -e CONFIG_whisk_containerFactory_containerArgs_dnsServers_{{loop.index0}}={{ item }}
+        {% endfor %}
         -e INVOKER_NUMCORE='{{ invoker.numcore }}'
         -e INVOKER_CORESHARE='{{ invoker.coreshare }}'
         -e INVOKER_USE_RUNC='{{ invoker.useRunc }}'
diff --git a/common/scala/src/main/scala/whisk/core/WhiskConfig.scala b/common/scala/src/main/scala/whisk/core/WhiskConfig.scala
index 0967c07b9f..61b7f2a7cd 100644
--- a/common/scala/src/main/scala/whisk/core/WhiskConfig.scala
+++ b/common/scala/src/main/scala/whisk/core/WhiskConfig.scala
@@ -58,11 +58,6 @@ class WhiskConfig(requiredProperties: Map[String, String],
   val dockerImagePrefix = this(WhiskConfig.dockerImagePrefix)
   val dockerImageTag = this(WhiskConfig.dockerImageTag)
 
-  val invokerContainerNetwork = this(WhiskConfig.invokerContainerNetwork)
-  val invokerContainerPolicy =
-    if (this(WhiskConfig.invokerContainerPolicy) == "") None else Some(this(WhiskConfig.invokerContainerPolicy))
-  val invokerContainerDns =
-    if (this(WhiskConfig.invokerContainerDns) == "") Seq() else this(WhiskConfig.invokerContainerDns).split(" ").toSeq
   val invokerNumCore = this(WhiskConfig.invokerNumCore)
   val invokerCoreShare = this(WhiskConfig.invokerCoreShare)
   val invokerUseRunc = this.getAsBoolean(WhiskConfig.invokerUseRunc, true)
@@ -191,9 +186,6 @@ object WhiskConfig {
   val dockerImagePrefix = "docker.image.prefix"
   val dockerImageTag = "docker.image.tag"
 
-  val invokerContainerNetwork = "invoker.container.network"
-  val invokerContainerPolicy = "invoker.container.policy"
-  val invokerContainerDns = "invoker.container.dns"
   val invokerNumCore = "invoker.numcore"
   val invokerCoreShare = "invoker.coreshare"
   val invokerUseRunc = "invoker.use.runc"
@@ -251,6 +243,8 @@ object ConfigKeys {
   val dockerTimeouts = s"$docker.timeouts"
   val runc = "whisk.runc"
   val runcTimeouts = s"$runc.timeouts"
+  val containerFactory = "whisk.container-factory"
+  val containerArgs = s"$containerFactory.container-args"
 
   val transactions = "whisk.transactions"
   val stride = s"$transactions.stride"
diff --git a/common/scala/src/main/scala/whisk/core/containerpool/ContainerFactory.scala b/common/scala/src/main/scala/whisk/core/containerpool/ContainerFactory.scala
index fb04f0f794..5c70d5d8d3 100644
--- a/common/scala/src/main/scala/whisk/core/containerpool/ContainerFactory.scala
+++ b/common/scala/src/main/scala/whisk/core/containerpool/ContainerFactory.scala
@@ -27,6 +27,10 @@ import whisk.core.entity.ExecManifest
 import whisk.core.entity.InstanceId
 import whisk.spi.Spi
 
+case class ContainerArgsConfig(network: String,
+                               dnsServers: Seq[String] = Seq(),
+                               extraArgs: Map[String, Set[String]] = Map())
+
 /**
  * An abstraction for Container creation
  */
diff --git a/core/invoker/src/main/resources/application.conf b/core/invoker/src/main/resources/application.conf
index e7ed0a64ea..94620dd203 100644
--- a/core/invoker/src/main/resources/application.conf
+++ b/core/invoker/src/main/resources/application.conf
@@ -19,4 +19,12 @@ whisk {
     pause: 10 seconds
     resume: 10 seconds
   }
+
+  # args for 'docker run' to use
+  container-factory.container-args {
+    network: bridge
+    dns-servers: []
+    extra-args: {}
+
+  }
 }
\ No newline at end of file
diff --git a/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainer.scala b/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainer.scala
index 2fc5f758e8..a4c3418b09 100644
--- a/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainer.scala
+++ b/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainer.scala
@@ -100,6 +100,7 @@ object DockerContainer {
       "--network",
       network) ++
       environmentArgs ++
+      dnsServers.flatMap(d => Seq("--dns", d)) ++
       name.map(n => Seq("--name", n)).getOrElse(Seq.empty) ++
       params
     val pulled = if (userProvidedImage) {
diff --git a/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainerFactory.scala b/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainerFactory.scala
index ffd18c9885..fbcd230bd0 100644
--- a/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainerFactory.scala
+++ b/core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainerFactory.scala
@@ -32,8 +32,15 @@ import whisk.core.entity.ExecManifest
 import whisk.core.entity.InstanceId
 import scala.concurrent.duration._
 import java.util.concurrent.TimeoutException
+import pureconfig._
+import whisk.core.ConfigKeys
+import whisk.core.containerpool.ContainerArgsConfig
 
-class DockerContainerFactory(config: WhiskConfig, instance: InstanceId, parameters: Map[String, Set[String]])(
+class DockerContainerFactory(config: WhiskConfig,
+                             instance: InstanceId,
+                             parameters: Map[String, Set[String]],
+                             containerArgs: ContainerArgsConfig =
+                               loadConfigOrThrow[ContainerArgsConfig](ConfigKeys.containerArgs))(
   implicit actorSystem: ActorSystem,
   ec: ExecutionContext,
   logging: Logging)
@@ -62,11 +69,11 @@ class DockerContainerFactory(config: WhiskConfig, instance: InstanceId, paramete
       memory = memory,
       cpuShares = config.invokerCoreShare.toInt,
       environment = Map("__OW_API_HOST" -> config.wskApiHost),
-      network = config.invokerContainerNetwork,
-      dnsServers = config.invokerContainerDns,
+      network = containerArgs.network,
+      dnsServers = containerArgs.dnsServers,
       name = Some(name),
       useRunc = config.invokerUseRunc,
-      parameters)
+      parameters ++ containerArgs.extraArgs)
   }
 
   /** Perform cleanup on init */
diff --git a/core/invoker/src/main/scala/whisk/core/invoker/Invoker.scala b/core/invoker/src/main/scala/whisk/core/invoker/Invoker.scala
index 648e61e072..ae15e90abe 100644
--- a/core/invoker/src/main/scala/whisk/core/invoker/Invoker.scala
+++ b/core/invoker/src/main/scala/whisk/core/invoker/Invoker.scala
@@ -66,9 +66,6 @@ object Invoker {
       dockerImageTag -> "latest",
       invokerNumCore -> "4",
       invokerCoreShare -> "2",
-      invokerContainerPolicy -> "",
-      invokerContainerDns -> "",
-      invokerContainerNetwork -> null,
       invokerUseRunc -> "true") ++
       Map(invokerName -> "")
 
diff --git a/core/invoker/src/main/scala/whisk/core/invoker/InvokerReactive.scala b/core/invoker/src/main/scala/whisk/core/invoker/InvokerReactive.scala
index c885c8581d..4efda9d17f 100644
--- a/core/invoker/src/main/scala/whisk/core/invoker/InvokerReactive.scala
+++ b/core/invoker/src/main/scala/whisk/core/invoker/InvokerReactive.scala
@@ -19,14 +19,11 @@ package whisk.core.invoker
 
 import java.nio.charset.StandardCharsets
 import java.time.Instant
-
 import scala.concurrent.Future
 import scala.concurrent.duration._
 import scala.util.Failure
 import scala.util.Success
-
 import org.apache.kafka.common.errors.RecordTooLargeException
-
 import akka.actor.ActorRefFactory
 import akka.actor.ActorSystem
 import akka.actor.Props
@@ -82,8 +79,7 @@ class InvokerReactive(config: WhiskConfig, instance: InstanceId, producer: Messa
         Map(
           "--cap-drop" -> Set("NET_RAW", "NET_ADMIN"),
           "--ulimit" -> Set("nofile=1024:1024"),
-          "--pids-limit" -> Set("1024"),
-          "--dns" -> config.invokerContainerDns.toSet) ++ logsProvider.containerParameters)
+          "--pids-limit" -> Set("1024")) ++ logsProvider.containerParameters)
   containerFactory.init()
   sys.addShutdownHook(containerFactory.cleanup())
 
diff --git a/tests/src/test/scala/whisk/core/containerpool/test/ContainerArgsConfigTest.scala b/tests/src/test/scala/whisk/core/containerpool/test/ContainerArgsConfigTest.scala
new file mode 100644
index 0000000000..9990c15b97
--- /dev/null
+++ b/tests/src/test/scala/whisk/core/containerpool/test/ContainerArgsConfigTest.scala
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package whisk.core.containerpool.test
+
+import org.junit.runner.RunWith
+import org.scalatest.FlatSpec
+import org.scalatest.Matchers
+import org.scalatest.junit.JUnitRunner
+import pureconfig._
+import whisk.core.ConfigKeys
+import whisk.core.containerpool.ContainerArgsConfig
+
+@RunWith(classOf[JUnitRunner])
+class ContainerArgsConfigTest extends FlatSpec with Matchers {
+
+  it should "use defaults for container args map" in {
+    val config = loadConfigOrThrow[ContainerArgsConfig](ConfigKeys.containerArgs)
+
+    //check defaults
+    config.network shouldBe "bridge"
+    config.dnsServers shouldBe Seq[String]()
+    config.extraArgs shouldBe Map[String, Set[String]]()
+  }
+
+  it should "override defaults from system properties" in {
+    System.setProperty("whisk.container-factory.container-args.extra-args.label.0", "l1")
+    System.setProperty("whisk.container-factory.container-args.extra-args.label.1", "l2")
+    System.setProperty("whisk.container-factory.container-args.extra-args.label.3", "l3")
+    System.setProperty("whisk.container-factory.container-args.extra-args.environment.0", "e1")
+    System.setProperty("whisk.container-factory.container-args.extra-args.environment.1", "e2")
+
+    System.setProperty("whisk.container-factory.container-args.dns-servers.0", "google.com")
+    System.setProperty("whisk.container-factory.container-args.dns-servers.1", "1.2.3.4")
+    val config = loadConfigOrThrow[ContainerArgsConfig](ConfigKeys.containerArgs)
+    //check defaults
+    config.network shouldBe "bridge"
+    config.dnsServers shouldBe Seq[String]("google.com", "1.2.3.4")
+    //check map parsing of extra-args config
+    config.extraArgs.get("label") shouldBe Some(Set("l1", "l2", "l3"))
+    config.extraArgs.get("environment") shouldBe Some(Set("e1", "e2"))
+
+  }
+}


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services