[GitHub] rabbah closed pull request #3259: use PureConfig for misc. runtimesManifest configuration

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

rabbah closed pull request #3259: use PureConfig for misc. runtimesManifest configuration
URL: https://github.com/apache/incubator-openwhisk/pull/3259
 
 
   

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/environments/docker-machine/group_vars/all b/ansible/environments/docker-machine/group_vars/all
index efd0b5628e..6f68e730f9 100644
--- a/ansible/environments/docker-machine/group_vars/all
+++ b/ansible/environments/docker-machine/group_vars/all
@@ -3,7 +3,7 @@ config_root_dir: /Users/Shared/wskconf
 whisk_logs_dir: /Users/Shared/wsklogs
 docker_registry: ""
 docker_dns: ""
-bypass_pull_for_local_images: true
+runtimes_bypass_pull_for_local_images: true
 
 env_hosts_dir: "{{ playbook_dir }}/environments/docker-machine"
 
diff --git a/ansible/environments/local/group_vars/all b/ansible/environments/local/group_vars/all
index fbed10ebe1..bcec410650 100755
--- a/ansible/environments/local/group_vars/all
+++ b/ansible/environments/local/group_vars/all
@@ -4,7 +4,7 @@ config_root_dir: "{{ openwhisk_tmp_dir }}/wskconf"
 whisk_logs_dir: "{{ openwhisk_tmp_dir }}/wsklogs"
 docker_registry: ""
 docker_dns: ""
-bypass_pull_for_local_images: true
+runtimes_bypass_pull_for_local_images: true
 
 db_prefix: whisk_local_
 
diff --git a/ansible/files/runtimes.json b/ansible/files/runtimes.json
index 8030fa2405..42858d6945 100644
--- a/ansible/files/runtimes.json
+++ b/ansible/files/runtimes.json
@@ -1,7 +1,4 @@
 {
-    "bypassPullForLocalImages": false,
-    "defaultImagePrefix": "openwhisk",
-    "defaultImageTag": "latest",
     "runtimes": {
         "nodejs": [
             {
diff --git a/ansible/group_vars/all b/ansible/group_vars/all
index 6978fb97ac..54c2d68b0f 100644
--- a/ansible/group_vars/all
+++ b/ansible/group_vars/all
@@ -24,16 +24,8 @@ whisk:
   version:
     date: "{{ansible_date_time.iso8601}}"
 
-##
-# list of supported runtimes (see whisk.core.entity.ExecManifest for schema).
-# briefly:
-#   defaultImagePrefix: the default image prefix when not given explicitly
-#   defaultImageTag: the default image tag
-#   runtimes: set of language runtime families grouped by language (e.g., nodejs, python)
-#   blackboxes: list of pre-populated docker action images as "name" with optional "prefix" and "tag"
-#   bypassPullForLocalImages: optional, if true, allow images with a prefix that matches {{ docker.image.prefix }}
-#                             to skip docker pull in invoker even if the image is not part of the blackbox set
-#
+
+# list of supported runtimes (see whisk.core.entity.ExecManifest for schema)
 runtimesManifest: "{{ runtimes_manifest | default(lookup('file', '{{ openwhisk_home }}/ansible/files/runtimes.json') | from_json) }}"
 
 limits:
diff --git a/ansible/roles/controller/tasks/deploy.yml b/ansible/roles/controller/tasks/deploy.yml
index 8601a2985d..9c19dd4255 100644
--- a/ansible/roles/controller/tasks/deploy.yml
+++ b/ansible/roles/controller/tasks/deploy.yml
@@ -133,6 +133,11 @@
       "CONFIG_whisk_activation_payload_max": "{{ limit_activation_payload | default() }}"
 
       "RUNTIMES_MANIFEST": "{{ runtimesManifest | to_json }}"
+      "CONFIG_whisk_runtimes_defaultImagePrefix": "{{ runtimes_default_image_prefix | default() }}"
+      "CONFIG_whisk_runtimes_defaultImageTag": "{{ runtimes_default_image_tag | default() }}"
+      "CONFIG_whisk_runtimes_bypassPullForLocalImages": "{{ runtimes_bypass_pull_for_local_images | default() }}"
+      "CONFIG_whisk_runtimes_localImagePrefix": "{{ runtimes_local_image_prefix | default() }}"
+
       "CONTROLLER_LOCALBOOKKEEPING": "{{ controller.localBookkeeping }}"
       "AKKA_CLUSTER_SEED_NODES": "{{seed_nodes_list | join(' ') }}"
       "CONTROLLER_HA": "{{ controller.ha }}"
diff --git a/ansible/roles/invoker/tasks/deploy.yml b/ansible/roles/invoker/tasks/deploy.yml
index c215e395a8..be2a251346 100644
--- a/ansible/roles/invoker/tasks/deploy.yml
+++ b/ansible/roles/invoker/tasks/deploy.yml
@@ -165,6 +165,10 @@
         -e WHISK_API_HOST_PORT='{{ whisk_api_host_port | default('443') }}'
         -e WHISK_API_HOST_NAME='{{ whisk_api_host_name | default(groups['edge'] | first) }}'
         -e RUNTIMES_MANIFEST='{{ runtimesManifest | to_json }}'
+        -e CONFIG_whisk_runtimes_defaultImagePrefix='{{ runtimes_default_image_prefix | default() }}'
+        -e CONFIG_whisk_runtimes_defaultImageTag='{{ runtimes_default_image_tag | default() }}'
+        -e CONFIG_whisk_runtimes_bypassPullForLocalImages='{{ runtimes_bypass_pull_for_local_images | default() }}'
+        -e CONFIG_whisk_runtimes_localImagePrefix='{{ runtimes_local_image_prefix | default() }}'
         -e DOCKER_REGISTRY='{{ docker_registry }}'
         -e DOCKER_IMAGE_PREFIX='{{ docker.image.prefix }}'
         -e DOCKER_IMAGE_TAG='{{ docker.image.tag }}'
diff --git a/common/scala/src/main/resources/application.conf b/common/scala/src/main/resources/application.conf
index 97b1e63a30..84682862d7 100644
--- a/common/scala/src/main/resources/application.conf
+++ b/common/scala/src/main/resources/application.conf
@@ -94,6 +94,13 @@ whisk {
         stride = 1
         stride = ${?CONTROLLER_INSTANCES}
     }
+    # action runtimes configuration
+    runtimes {
+        default-image-prefix = "openwhisk"
+        default-image-tag = "latest"
+        bypass-pull-for-local-images = false
+        local-image-prefix = "whisk"
+    }
 
     activation {
         payload {
diff --git a/common/scala/src/main/scala/whisk/core/WhiskConfig.scala b/common/scala/src/main/scala/whisk/core/WhiskConfig.scala
index 22dfe232c1..5d3fadab3b 100644
--- a/common/scala/src/main/scala/whisk/core/WhiskConfig.scala
+++ b/common/scala/src/main/scala/whisk/core/WhiskConfig.scala
@@ -245,6 +245,8 @@ object ConfigKeys {
   val activation = "whisk.activation"
   val activationPayload = s"$activation.payload"
 
+  val runtimes = "whisk.runtimes"
+
   val db = "whisk.db"
 
   val docker = "whisk.docker"
diff --git a/common/scala/src/main/scala/whisk/core/entity/ExecManifest.scala b/common/scala/src/main/scala/whisk/core/entity/ExecManifest.scala
index dc446914a0..668ef609f6 100644
--- a/common/scala/src/main/scala/whisk/core/entity/ExecManifest.scala
+++ b/common/scala/src/main/scala/whisk/core/entity/ExecManifest.scala
@@ -17,10 +17,12 @@
 
 package whisk.core.entity
 
+import pureconfig.loadConfigOrThrow
+
 import scala.util.{Failure, Try}
 import spray.json._
 import spray.json.DefaultJsonProtocol._
-import whisk.core.WhiskConfig
+import whisk.core.{ConfigKeys, WhiskConfig}
 import whisk.core.entity.Attachments._
 import whisk.core.entity.Attachments.Attached._
 
@@ -41,11 +43,11 @@ protected[core] object ExecManifest {
    * singleton Runtime instance.
    *
    * @param config a valid configuration
-   * @param localDockerImagePrefix optional local docker prefix, permitting images matching prefix to bypass docker pull
    * @return the manifest if initialized successfully, or an failure
    */
-  protected[core] def initialize(config: WhiskConfig, localDockerImagePrefix: Option[String] = None): Try[Runtimes] = {
-    val mf = Try(config.runtimesManifest.parseJson.asJsObject).flatMap(runtimes(_, localDockerImagePrefix))
+  protected[core] def initialize(config: WhiskConfig): Try[Runtimes] = {
+    val rmc = loadConfigOrThrow[RuntimeManifestConfig](ConfigKeys.runtimes)
+    val mf = Try(config.runtimesManifest.parseJson.asJsObject).flatMap(runtimes(_, rmc))
     mf.foreach(m => manifest = Some(m))
     mf
   }
@@ -69,9 +71,10 @@ protected[core] object ExecManifest {
    * @param config a configuration object as JSON
    * @return Runtimes instance
    */
-  protected[entity] def runtimes(config: JsObject, localDockerImagePrefix: Option[String] = None): Try[Runtimes] = Try {
-    val prefix = config.fields.get("defaultImagePrefix").map(_.convertTo[String])
-    val tag = config.fields.get("defaultImageTag").map(_.convertTo[String])
+  protected[entity] def runtimes(config: JsObject, runtimeManifestConfig: RuntimeManifestConfig): Try[Runtimes] = Try {
+
+    val prefix = runtimeManifestConfig.defaultImagePrefix
+    val tag = runtimeManifestConfig.defaultImageTag
 
     val runtimes = config.fields
       .get("runtimes")
@@ -89,16 +92,26 @@ protected[core] object ExecManifest {
         ImageName(image.name, image.prefix.orElse(prefix), image.tag.orElse(tag))
       })
 
-    val bypassPullForLocalImages = config.fields
-      .get("bypassPullForLocalImages")
-      .map(_.convertTo[Boolean])
+    val bypassPullForLocalImages = runtimeManifestConfig.bypassPullForLocalImages
       .filter(identity)
-      .flatMap(_ => localDockerImagePrefix)
-      .map(_.trim)
+      .flatMap(_ => runtimeManifestConfig.localImagePrefix)
 
     Runtimes(runtimes.getOrElse(Set.empty), blackbox.getOrElse(Set.empty), bypassPullForLocalImages)
   }
 
+  /**
+   * Misc options related to runtime manifests
+   * @param defaultImagePrefix the default image prefix when not given explicitly
+   * @param defaultImageTag the default image tag
+   * @param bypassPullForLocalImages if true, allow images with a prefix that matches localImagePrefix
+   *                                 to skip docker pull in invoker even if the image is not part of the blackbox set
+   * @param localImagePrefix image prefix for bypassPullForLocalImages
+   */
+  protected[core] case class RuntimeManifestConfig(defaultImagePrefix: Option[String] = None,
+                                                   defaultImageTag: Option[String] = None,
+                                                   bypassPullForLocalImages: Option[Boolean] = None,
+                                                   localImagePrefix: Option[String] = None)
+
   /**
    * A runtime manifest describes the "exec" runtime support.
    *
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 ae15e90abe..4ba2214fa7 100644
--- a/core/invoker/src/main/scala/whisk/core/invoker/Invoker.scala
+++ b/core/invoker/src/main/scala/whisk/core/invoker/Invoker.scala
@@ -98,7 +98,7 @@ object Invoker {
       abort("Bad configuration, cannot start.")
     }
 
-    val execManifest = ExecManifest.initialize(config, localDockerImagePrefix = Some(config.dockerImagePrefix))
+    val execManifest = ExecManifest.initialize(config)
     if (execManifest.isFailure) {
       logger.error(this, s"Invalid runtimes manifest: ${execManifest.failed.get}")
       abort("Bad configuration, cannot start.")
diff --git a/tests/src/test/scala/whisk/core/entity/test/ExecManifestTests.scala b/tests/src/test/scala/whisk/core/entity/test/ExecManifestTests.scala
index d232081ded..71f02379e9 100644
--- a/tests/src/test/scala/whisk/core/entity/test/ExecManifestTests.scala
+++ b/tests/src/test/scala/whisk/core/entity/test/ExecManifestTests.scala
@@ -17,15 +17,12 @@
 
 package whisk.core.entity.test
 
-import java.io.{BufferedWriter, File, FileWriter}
-
 import common.{StreamLogging, WskActorSystem}
 import org.junit.runner.RunWith
 import org.scalatest.{FlatSpec, Matchers}
 import org.scalatest.junit.JUnitRunner
 import spray.json.DefaultJsonProtocol._
 import spray.json._
-import whisk.core.WhiskConfig
 import whisk.core.entity.ExecManifest
 import whisk.core.entity.ExecManifest._
 
@@ -67,7 +64,7 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
     val k2 = RuntimeManifest("k2", ImageName("???"), default = Some(true))
     val p1 = RuntimeManifest("p1", ImageName("???"))
     val mf = manifestFactory(JsObject("ks" -> Set(k1, k2).toJson, "p1" -> Set(p1).toJson))
-    val runtimes = ExecManifest.runtimes(mf).get
+    val runtimes = ExecManifest.runtimes(mf, RuntimeManifestConfig()).get
 
     Seq("k1", "k2", "p1").foreach {
       runtimes.knownContainerRuntimes.contains(_) shouldBe true
@@ -89,11 +86,10 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
     val j1 = RuntimeManifest("j1", ImageName("???", Some("ppp"), Some("ttt")))
     val k1 = RuntimeManifest("k1", ImageName("???", None, Some("ttt")))
 
-    val mf = JsObject(
-      "defaultImagePrefix" -> "pre".toJson,
-      "defaultImageTag" -> "test".toJson,
-      "runtimes" -> JsObject("is" -> Set(i1, i2).toJson, "js" -> Set(j1).toJson, "ks" -> Set(k1).toJson))
-    val runtimes = ExecManifest.runtimes(mf).get
+    val mf =
+      JsObject("runtimes" -> JsObject("is" -> Set(i1, i2).toJson, "js" -> Set(j1).toJson, "ks" -> Set(k1).toJson))
+    val rmc = RuntimeManifestConfig(defaultImagePrefix = Some("pre"), defaultImageTag = Some("test"))
+    val runtimes = ExecManifest.runtimes(mf, rmc).get
 
     runtimes.resolveDefaultRuntime("i1").get.image.publicImageName shouldBe "pre/???:test"
     runtimes.resolveDefaultRuntime("i2").get.image.publicImageName shouldBe "ppp/???:test"
@@ -109,7 +105,7 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
       ImageName("???", None, Some("ttt")))
 
     val mf = JsObject("runtimes" -> JsObject(), "blackboxes" -> imgs.toJson)
-    val runtimes = ExecManifest.runtimes(mf).get
+    val runtimes = ExecManifest.runtimes(mf, RuntimeManifestConfig()).get
 
     runtimes.blackboxImages shouldBe imgs
     imgs.foreach(img => runtimes.skipDockerPull(img) shouldBe true)
@@ -123,12 +119,9 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
       ImageName("???", Some("ppp"), Some("ttt")),
       ImageName("???", None, Some("ttt")))
 
-    val mf = JsObject(
-      "defaultImagePrefix" -> "pre".toJson,
-      "defaultImageTag" -> "test".toJson,
-      "runtimes" -> JsObject(),
-      "blackboxes" -> imgs.toJson)
-    val runtimes = ExecManifest.runtimes(mf).get
+    val mf = JsObject("runtimes" -> JsObject(), "blackboxes" -> imgs.toJson)
+    val rmc = RuntimeManifestConfig(defaultImagePrefix = Some("pre"), defaultImageTag = Some("test"))
+    val runtimes = ExecManifest.runtimes(mf, rmc).get
 
     runtimes.blackboxImages shouldBe {
       Set(
@@ -147,7 +140,7 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
     val k2 = RuntimeManifest("k2", ImageName("???"), default = Some(true))
     val mf = manifestFactory(JsObject("ks" -> Set(k1, k2).toJson))
 
-    an[IllegalArgumentException] should be thrownBy ExecManifest.runtimes(mf).get
+    an[IllegalArgumentException] should be thrownBy ExecManifest.runtimes(mf, RuntimeManifestConfig()).get
   }
 
   it should "reject finding a default when none is specified for multiple versions" in {
@@ -155,7 +148,7 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
     val k2 = RuntimeManifest("k2", ImageName("???"))
     val mf = manifestFactory(JsObject("ks" -> Set(k1, k2).toJson))
 
-    an[IllegalArgumentException] should be thrownBy ExecManifest.runtimes(mf).get
+    an[IllegalArgumentException] should be thrownBy ExecManifest.runtimes(mf, RuntimeManifestConfig()).get
   }
 
   it should "prefix image name with overrides" in {
@@ -179,18 +172,9 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
   }
 
   it should "indicate image is local if it matches deployment docker prefix" in {
-    val config_manifest = """{"bypassPullForLocalImages":true}"""
-    val file = File.createTempFile("cxt", ".txt")
-    file.deleteOnExit()
-
-    val bw = new BufferedWriter(new FileWriter(file))
-    bw.write(WhiskConfig.runtimesManifest + s"=$config_manifest\n")
-    bw.close()
-
-    val props = Map(WhiskConfig.runtimesManifest -> null)
-    val manifest =
-      ExecManifest.initialize(new WhiskConfig(props, Set(), file), localDockerImagePrefix = Some("localpre"))
-    manifest should be a 'success
+    val mf = JsObject()
+    val rmc = RuntimeManifestConfig(bypassPullForLocalImages = Some(true), localImagePrefix = Some("localpre"))
+    val manifest = ExecManifest.runtimes(mf, rmc)
 
     manifest.get.skipDockerPull(ImageName(prefix = Some("x"), name = "y")) shouldBe false
     manifest.get.skipDockerPull(ImageName(prefix = Some("localpre"), name = "y")) shouldBe true


 

----------------------------------------------------------------
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