Adjust invoker playbook to pull runtime and blackbox docker images wh…

…en a prefix and tag is specified.

This allows images published to be a docker registry to be used and bypassing a build step entirely.
By removing docker_registry entanglement when pulling images we also eschew retagging.
Use runtimes_default_image_prefix and runtimes_default_image_tag where no prefix and tag are specified.

Extends image name parser to handle optional registry.

ansible/group_vars/all

@@ -30,11 +30,11 @@ whisk:
 
 ##
 # 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"
+# related settings:
+#   defaultImagePrefix: the default image prefix when not given explicitly
+#   defaultImageTag: the default image 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
 #

ansible/roles/invoker/tasks/deploy.yml

@@ -13,18 +13,24 @@
   retries: "{{ docker.pull.retries }}"
   delay: "{{ docker.pull.delay }}"
 
-- name: "pull runtime action images with tag {{docker.image.tag}}"
-  shell: "docker pull {{docker_registry}}{{docker.image.prefix}}/{{item}}:{{docker.image.tag}}"
-  with_items: "{{ runtimesManifest.runtimes.values() | sum(start=[]) | selectattr('deprecated', 'equalto',false)  | map(attribute='image.name') | list | unique }}"
-  when: docker_registry != ""
+##
+# Pulls all managed images from the runtime manifest to the invoker.
+# Assumes that the image is locally built if it does not specify a prefix, and hence skips the pull in that case.
+- name: "pull runtime action images per manifest"
+  shell: "docker pull {{item.prefix | default(runtimes_default_image_prefix)}}/{{item.name}}:{{item.tag | default(runtimes_default_image_tag | default('latest'))}}"
+  with_items: "{{ runtimesManifest.runtimes.values() | sum(start=[]) | selectattr('deprecated', 'equalto',false)  | map(attribute='image') | list | unique }}"
+  when: (item.prefix is defined and item.prefix != "") or (item.tag is defined and item.tag != "")
   retries: "{{ docker.pull.retries }}"
   delay: "{{ docker.pull.delay }}"
 
-- name: "pull blackboxes action images with tag {{docker.image.tag}}"
-  shell: "docker pull {{docker_registry}}{{docker.image.prefix}}/{{item.name}}:{{docker.image.tag}}"
+##
+# Pulls all black box images from the runtime manifest to the invoker.
+# Assumes that the image is locally built if it does not specify a prefix, and hence skips the pull in that case.
+- name: "pull blackboxes action images per manifest"
+  shell: "docker pull {{item.prefix | default(runtimes_default_image_prefix)}}/{{item.name}}:{{item.tag | default(runtimes_default_image_tag | default('latest'))}}"
   with_items:
     - "{{ runtimesManifest.blackboxes }}"
-  when: docker_registry != ""
+  when: (item.prefix is defined and item.prefix != "") or (item.tag is defined and item.tag != "")
   retries: "{{ docker.pull.retries }}"
   delay: "{{ docker.pull.delay }}"
 

common/scala/src/main/scala/whisk/core/entity/ExecManifest.scala

@@ -158,21 +158,6 @@ protected[core] object ExecManifest {
       p + name + t
     }
 
-    /**
-     * The internal name of the image for an action kind. It overrides
-     * the prefix with an internal name. Optionally overrides tag.
-     */
-    def localImageName(registry: String, prefix: String, tagOverride: Option[String] = None): String = {
-      val r = Option(registry)
-        .filter(_.nonEmpty)
-        .map { reg =>
-          if (reg.endsWith("/")) reg else reg + "/"
-        }
-        .getOrElse("")
-      val p = Option(prefix).filter(_.nonEmpty).map(_ + "/").getOrElse("")
-      r + p + name + ":" + tagOverride.orElse(tag).getOrElse(ImageName.defaultImageTag)
-    }
-
     /**
      * Overrides equals to allow match on undefined tag or when tag is latest
      * in this or that.
@@ -193,6 +178,7 @@ protected[core] object ExecManifest {
     protected val defaultImageTag = "latest"
     private val componentRegex = """([a-z0-9._-]+)""".r
     private val tagRegex = """([\w.-]{0,128})""".r
+    private val registryRegex = """([a-z0-9.-]+)(:\d+)?""".r
 
     /**
      * Constructs an ImageName from a string. This method checks that the image name conforms
@@ -203,17 +189,25 @@ protected[core] object ExecManifest {
     def fromString(s: String): Try[ImageName] =
       Try {
         val parts = s.split("/")
-
         val (name, tag) = parts.last.split(":") match {
           case Array(componentRegex(s))              => (s, None)
           case Array(componentRegex(s), tagRegex(t)) => (s, Some(t))
           case _                                     => throw DeserializationException("image name is not valid")
         }
 
         val prefixParts = parts.dropRight(1)
-        if (!prefixParts.forall(componentRegex.pattern.matcher(_).matches)) {
+
+        // the first component may be a registry with a port specified
+        if (!prefixParts.headOption.forall(
+              r => componentRegex.pattern.matcher(r).matches || registryRegex.pattern.matcher(r).matches())) {
+          throw DeserializationException("image registry or prefix not is not valid")
+        }
+
+        // skip first component, since it is already checked
+        if (!prefixParts.drop(1).forall(componentRegex.pattern.matcher(_).matches)) {
           throw DeserializationException("image prefix not is not valid")
         }
+
         val prefix = if (prefixParts.nonEmpty) Some(prefixParts.mkString("/")) else None
 
         ImageName(name, prefix, tag)

common/scala/src/main/scala/whisk/core/mesos/MesosContainerFactory.scala

@@ -111,11 +111,8 @@ class MesosContainerFactory(config: WhiskConfig,
                                memory: ByteSize,
                                cpuShares: Int)(implicit config: WhiskConfig, logging: Logging): Future[Container] = {
     implicit val transid = tid
-    val image = if (userProvidedImage) {
-      actionImage.publicImageName
-    } else {
-      actionImage.localImageName(config.dockerRegistry, config.dockerImagePrefix, Some(config.dockerImageTag))
-    }
+    val image = actionImage.publicImageName
+
     val constraintStrings = if (userProvidedImage) {
       mesosConfig.blackboxConstraints
     } else {

core/invoker/src/main/scala/whisk/core/containerpool/docker/DockerContainerFactory.scala

@@ -58,11 +58,7 @@ class DockerContainerFactory(instance: InstanceId,
                                userProvidedImage: Boolean,
                                memory: ByteSize,
                                cpuShares: Int)(implicit config: WhiskConfig, logging: Logging): Future[Container] = {
-    val image = if (userProvidedImage) {
-      actionImage.publicImageName
-    } else {
-      actionImage.localImageName(config.dockerRegistry, config.dockerImagePrefix, Some(config.dockerImageTag))
-    }
+    val image = actionImage.publicImageName
 
     DockerContainer.create(
       tid,

core/invoker/src/main/scala/whisk/core/containerpool/kubernetes/KubernetesContainerFactory.scala

@@ -66,11 +66,7 @@ class KubernetesContainerFactory(label: String, config: WhiskConfig)(implicit ac
                                userProvidedImage: Boolean,
                                memory: ByteSize,
                                cpuShares: Int)(implicit config: WhiskConfig, logging: Logging): Future[Container] = {
-    val image = if (userProvidedImage) {
-      actionImage.publicImageName
-    } else {
-      actionImage.localImageName(config.dockerRegistry, config.dockerImagePrefix, Some(config.dockerImageTag))
-    }
+    val image = actionImage.publicImageName
 
     KubernetesContainer.create(
       tid,

tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerFactoryTests.scala

@@ -69,7 +69,7 @@ class DockerContainerFactoryTests
     (dockerApiStub
       .run(_: String, _: Seq[String])(_: TransactionId))
       .expects(
-        image.localImageName(config.dockerRegistry, config.dockerImagePrefix, Some(config.dockerImageTag)),
+        image.publicImageName,
         List(
           "--cpu-shares",
           "32", //should be calculated as 1024/(numcore * sharefactor) via ContainerFactory.cpuShare

tests/src/test/scala/whisk/core/containerpool/mesos/test/MesosContainerFactoryTest.scala

@@ -141,7 +141,7 @@ class MesosContainerFactoryTest
       SubmitTask(TaskDef(
         lastTaskId,
         "mesosContainer",
-        "fakeImage:" + wskConfig.dockerImageTag,
+        "fakeImage",
         mesosCpus,
         1,
         List(8080),
@@ -190,7 +190,7 @@ class MesosContainerFactoryTest
       SubmitTask(TaskDef(
         lastTaskId,
         "mesosContainer",
-        "fakeImage:" + wskConfig.dockerImageTag,
+        "fakeImage",
         mesosCpus,
         1,
         List(8080),
@@ -262,7 +262,7 @@ class MesosContainerFactoryTest
       SubmitTask(TaskDef(
         lastTaskId,
         "mesosContainer",
-        "fakeImage:" + wskConfig.dockerImageTag,
+        "fakeImage",
         mesosCpus,
         1,
         List(8080),

tests/src/test/scala/whisk/core/entity/test/ExecManifestTests.scala

@@ -51,12 +51,20 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
       "pre/img" -> ImageName("img", Some("pre")),
       "pre/img:t" -> ImageName("img", Some("pre"), Some("t")),
       "pre1/pre2/img:t" -> ImageName("img", Some("pre1/pre2"), Some("t")),
-      "pre1/pre2/img" -> ImageName("img", Some("pre1/pre2")))
+      "pre1/pre2/img" -> ImageName("img", Some("pre1/pre2")),
+      "r:443/img" -> ImageName("img", Some("r:443")),
+      "r:443/img:123" -> ImageName("img", Some("r:443"), Some("123")),
+      "r:443/pre1/pre2/img" -> ImageName("img", Some("r:443/pre1/pre2")),
+      "r:443/pre1/pre2/img:t" -> ImageName("img", Some("r:443/pre1/pre2"), Some("t")),
+      "r:443/pre1/pre2/img:123" -> ImageName("img", Some("r:443/pre1/pre2"), Some("123")))
       .foreach {
-        case (s, v) => ImageName.fromString(s) shouldBe Success(v)
+        case (s, v) =>
+          withClue(s) {
+            ImageName.fromString(s) shouldBe Success(v)
+          }
       }
 
-    Seq("ABC", "x:8080/abc", "p/a:x:y").foreach { s =>
+    Seq("ABC", "x_y:8080/abc", "x/abc:8080:tag", "x/abc:8080/abc", "p/a:x:y").foreach { s =>
       a[DeserializationException] should be thrownBy ImageName.fromString(s).get
     }
   }
@@ -174,14 +182,7 @@ class ExecManifestTests extends FlatSpec with WskActorSystem with StreamLogging
       (ExecManifest.ImageName(name, Some("pre")), s"pre/$name"),
       (ExecManifest.ImageName(name, None, Some("t")), s"$name:t"),
       (ExecManifest.ImageName(name, Some("pre"), Some("t")), s"pre/$name:t")).foreach {
-      case (image, exp) =>
-        image.publicImageName shouldBe exp
-
-        image.localImageName("", "", None) shouldBe image.tag.map(t => s"$name:$t").getOrElse(s"$name:latest")
-        image.localImageName("", "p", None) shouldBe image.tag.map(t => s"p/$name:$t").getOrElse(s"p/$name:latest")
-        image.localImageName("r", "", None) shouldBe image.tag.map(t => s"r/$name:$t").getOrElse(s"r/$name:latest")
-        image.localImageName("r", "p", None) shouldBe image.tag.map(t => s"r/p/$name:$t").getOrElse(s"r/p/$name:latest")
-        image.localImageName("r", "p", Some("tag")) shouldBe s"r/p/$name:tag"
+      case (image, exp) => image.publicImageName shouldBe exp
     }
   }