From 0b1adb0375c1eea716eded852932a91e66e10360 Mon Sep 17 00:00:00 2001 From: Rodric Rabbah Date: Fri, 18 May 2018 13:33:45 -0400 Subject: [PATCH] Adjust invoker playbook to pull runtime and blackbox docker images when 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 | 6 ++-- ansible/roles/invoker/tasks/deploy.yml | 20 ++++++++----- .../whisk/core/entity/ExecManifest.scala | 28 ++++++++----------- .../core/mesos/MesosContainerFactory.scala | 7 ++--- .../docker/DockerContainerFactory.scala | 6 +--- .../KubernetesContainerFactory.scala | 6 +--- .../test/DockerContainerFactoryTests.scala | 2 +- .../test/MesosContainerFactoryTest.scala | 6 ++-- .../core/entity/test/ExecManifestTests.scala | 23 +++++++-------- 9 files changed, 47 insertions(+), 57 deletions(-) diff --git a/ansible/group_vars/all b/ansible/group_vars/all index 1acdfbbc12d..e8a3293e727 100644 --- a/ansible/group_vars/all +++ b/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 # diff --git a/ansible/roles/invoker/tasks/deploy.yml b/ansible/roles/invoker/tasks/deploy.yml index ee4a79aaf33..e0732c13e2e 100644 --- a/ansible/roles/invoker/tasks/deploy.yml +++ b/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 }}" 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 7436d167f0c..e55678c4aea 100644 --- a/common/scala/src/main/scala/whisk/core/entity/ExecManifest.scala +++ b/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,7 +189,6 @@ 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)) @@ -211,9 +196,18 @@ protected[core] object ExecManifest { } 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) diff --git a/common/scala/src/main/scala/whisk/core/mesos/MesosContainerFactory.scala b/common/scala/src/main/scala/whisk/core/mesos/MesosContainerFactory.scala index b0d47a90a1f..71ddfffd863 100644 --- a/common/scala/src/main/scala/whisk/core/mesos/MesosContainerFactory.scala +++ b/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 { 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 3f2ee864061..50c182aef10 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 @@ -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, diff --git a/core/invoker/src/main/scala/whisk/core/containerpool/kubernetes/KubernetesContainerFactory.scala b/core/invoker/src/main/scala/whisk/core/containerpool/kubernetes/KubernetesContainerFactory.scala index e332b848613..c0ec4f01172 100644 --- a/core/invoker/src/main/scala/whisk/core/containerpool/kubernetes/KubernetesContainerFactory.scala +++ b/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, diff --git a/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerFactoryTests.scala b/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerFactoryTests.scala index 31139dc8028..e5a5a1da0c5 100644 --- a/tests/src/test/scala/whisk/core/containerpool/docker/test/DockerContainerFactoryTests.scala +++ b/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 diff --git a/tests/src/test/scala/whisk/core/containerpool/mesos/test/MesosContainerFactoryTest.scala b/tests/src/test/scala/whisk/core/containerpool/mesos/test/MesosContainerFactoryTest.scala index 51c62c5a662..f5e33cfe0f5 100644 --- a/tests/src/test/scala/whisk/core/containerpool/mesos/test/MesosContainerFactoryTest.scala +++ b/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), 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 690e4490517..746f4b5f265 100644 --- a/tests/src/test/scala/whisk/core/entity/test/ExecManifestTests.scala +++ b/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 } }