Improved layer grouping

The main purpose of this change is to make layer grouping more robust. Instead of using a heuristic based on whether a library is in the same `organization` as the build (which is not always what you want, some builds may have sub projects with different organizations, and some builds may depend on external dependencies from the same organization), it checks whether the library in question is present in `projectDependencyArtifacts` (ie, whether its a dependency built by this build), if it is, then it goes in a higher layer. Doing this change however required changing `dockerLayerGrouping` to be a `TaskKey` instead of a `SettingKey`, which is a non binary compatible change. So, `dockerLayerGrouping` has been deprecated, and a new `TaskKey`, `dockerGroupLayers` has been introduced. Since a new task has been introduced, I also took advantage of that to improve the types, instead of just passing the destination path, which requires you to know how destination paths are built to check if a particular file should be in there, it passes the whole mapping, ie, the source file and the destination path, which means that you can match against the source file (which is what `projectDependencyArtifacts` gives you) as well. Also, I changed the type from a function that returns an `Option` to a `PartialFunction`, this makes composing it, to introduce new mappings, far simpler, as `PartialFunction[_, _]` composes trivially compared to `Function[_, Option[_]]`.
sbt · Apr 6, 2020 · d47d4fb · d47d4fb
1 parent 106bb94
commit d47d4fb
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 36 deletions.
diff --git a/src/main/scala/com/typesafe/sbt/packager/docker/DockerPlugin.scala b/src/main/scala/com/typesafe/sbt/packager/docker/DockerPlugin.scala
@@ -98,19 +98,25 @@ object DockerPlugin extends AutoPlugin {
     ),
     dockerUpdateLatest := false,
     dockerAutoremoveMultiStageIntermediateImages := true,
-    dockerLayerGrouping := {
+    dockerLayerGrouping := { _: String =>
+      None
+    },
+    dockerGroupLayers := {
       val dockerBaseDirectory = (defaultLinuxInstallLocation in Docker).value
-      (path: String) =>
-        {
-          val pathInWorkdir = path.stripPrefix(dockerBaseDirectory)
-          if (pathInWorkdir.startsWith(s"/lib/${organization.value}"))
-            Some(2)
-          else if (pathInWorkdir.startsWith("/lib/"))
-            Some(1)
-          else if (pathInWorkdir.startsWith("/bin/"))
-            Some(1)
-          else None
-        }
+      // Ensure this doesn't break even if the JvmPlugin isn't enabled.
+      val artifacts = projectDependencyArtifacts.?.value.getOrElse(Nil).map(_.data).toSet
+      val oldFunction = (dockerLayerGrouping in Docker).value
+
+      // By default we set this to a function that always returns None.
+      val oldPartialFunction = Function.unlift((tuple: (File, String)) => oldFunction(tuple._2))
+
+      val libDir = dockerBaseDirectory + "/lib/"
+      val binDir = dockerBaseDirectory + "/bin/"
+
+      oldPartialFunction.orElse {
+        case (file, _) if artifacts(file)                                    => 2
+        case (_, path) if path.startsWith(libDir) || path.startsWith(binDir) => 1
+      }
     },
     dockerAliases := {
       val alias = dockerAlias.value
@@ -159,7 +165,8 @@ object DockerPlugin extends AutoPlugin {
       val multiStageId = UUID.randomUUID().toString
       val generalCommands = makeFromAs(base, "mainstage") +: makeMaintainer((maintainer in Docker).value).toSeq
       val stage0name = "stage0"
-      val layerIdsAscending = (dockerLayerMappings in Docker).value.map(_.layerId).distinct.sorted
+      val layerMappings = (dockerLayerMappings in Docker).value
+      val layerIdsAscending = layerMappings.map(_.layerId).distinct.sorted
       val stage0: Seq[CmdLike] = strategy match {
         case DockerPermissionStrategy.MultiStage =>
           Seq(
@@ -172,10 +179,18 @@ object DockerPlugin extends AutoPlugin {
             Seq(makeUser("root")) ++ layerIdsAscending.map(
             l => makeChmodRecursive(dockerChmodType.value, Seq(pathInLayer(dockerBaseDirectory, l)))
           ) ++ {
-            val layerToPath = (dockerLayerGrouping in Docker).value
+            val layerToPath = (dockerGroupLayers in Docker).value
             addPerms map {
               case (tpe, v) =>
-                val layerId = layerToPath(v)
+                // Try and find the source file for the path from the mappings
+                val layerId = layerMappings
+                  .find(_.path == v)
+                  .map(_.layerId)
+                  .getOrElse {
+                    // We couldn't find a source file for the mapping, so try with a dummy source file,
+                    // in case there is an explicitly configured path based layer mapping, eg for a directory.
+                    layerToPath.lift((new File("/dev/null"), v))
+                  }
                 makeChmod(tpe, Seq(pathInLayer(v, layerId)))
             }
           } ++
@@ -263,11 +278,11 @@ object DockerPlugin extends AutoPlugin {
       stage := (stage dependsOn dockerGenerateConfig).value,
       stagingDirectory := (target in Docker).value / "stage",
       dockerLayerMappings := {
-        val dockerGroups = dockerLayerGrouping.value
+        val dockerGroups = dockerGroupLayers.value
         val dockerFinalFiles = (mappings in Docker).value
         for {
-          (file, path) <- dockerFinalFiles
-          layerIdx = dockerGroups(path)
+          mapping @ (file, path) <- dockerFinalFiles
+          layerIdx = dockerGroups.lift(mapping)
         } yield LayeredMapping(layerIdx, file, path)
       },
       target := target.value / "docker",

diff --git a/src/main/scala/com/typesafe/sbt/packager/docker/Keys.scala b/src/main/scala/com/typesafe/sbt/packager/docker/Keys.scala
@@ -57,10 +57,15 @@ private[packager] trait DockerKeysEx extends DockerKeys {
   lazy val dockerAdditionalPermissions =
     taskKey[Seq[(DockerChmodType, String)]]("Explicit chmod calls to some of the paths.")
   val dockerApiVersion = TaskKey[Option[DockerApiVersion]]("dockerApiVersion", "The docker server api version")
+  @deprecated("Use dockerGroupLayers instead", "1.7.1")
   val dockerLayerGrouping = settingKey[String => Option[Int]](
     "Group files by path into in layers to increase docker cache hits. " +
       "Lower index means the file would be a part of an earlier layer."
   )
+  val dockerGroupLayers = taskKey[PartialFunction[(File, String), Int]](
+    "Group files by mapping into layers to increase docker cache hits. " +
+      "Lower index means the file would be a part of an earlier layer."
+  )
   val dockerLayerMappings =
     taskKey[Seq[LayeredMapping]]("List of layer, source file and destination in Docker image.")
 }
diff --git a/src/sbt-test/docker/test-layer-groups/build.sbt b/src/sbt-test/docker/test-layer-groups/build.sbt
@@ -6,5 +6,7 @@ version := "0.1.0"
 
 dockerPackageMappings in Docker ++= Seq(
   (baseDirectory.value / "docker" / "spark-env.sh") -> "/opt/docker/spark/spark-env.sh",
-  (baseDirectory.value / "docker" / "log4j.properties") -> "/opt/docker/spark/log4j.properties"
+  (baseDirectory.value / "docker" / "log4j.properties") -> "/opt/docker/other/log4j.properties"
 )
+
+libraryDependencies += "org.slf4j" % "slf4j-api" % "1.7.30"
diff --git a/src/sbt-test/docker/test-layer-groups/changes/nolayers.sbt b/src/sbt-test/docker/test-layer-groups/changes/nolayers.sbt
@@ -1 +1 @@
-dockerLayerGrouping in Docker  := (_ => None)
+dockerGroupLayers in Docker := PartialFunction.empty
diff --git a/src/sbt-test/docker/test-layer-groups/layers.sbt b/src/sbt-test/docker/test-layer-groups/layers.sbt
@@ -1,14 +1,6 @@
-dockerLayerGrouping in Docker := {
+dockerGroupLayers in Docker := {
   val dockerBaseDirectory = (defaultLinuxInstallLocation in Docker).value
-  (path: String) =>
-    {
-      val pathInWorkdir = path.stripPrefix(dockerBaseDirectory)
-      if (pathInWorkdir.startsWith(s"/lib/${organization.value}"))
-        Some(2)
-      else if (pathInWorkdir.startsWith("/bin/"))
-        Some(123)
-      else if (pathInWorkdir.startsWith("/spark/"))
-        Some(54)
-      else None
-    }
+  (dockerGroupLayers in Docker).value.orElse {
+    case (_, path) if path.startsWith(dockerBaseDirectory + "/spark/") => 54
+  }
 }
diff --git a/src/sbt-test/docker/test-layer-groups/test b/src/sbt-test/docker/test-layer-groups/test
@@ -1,17 +1,27 @@
 # Generate the Docker image locally
 > docker:publishLocal
 $ exists target/docker/stage/Dockerfile
-$ exists target/docker/stage/123/opt/docker/bin/docker-groups
-$ exists target/docker/stage/opt
+$ exists target/docker/stage/1/opt/docker/bin/docker-groups
+$ exists target/docker/stage/1/opt/docker/lib/org.slf4j.slf4j-api-1.7.30.jar
+-$ exists target/docker/stage/1/opt/docker/lib/com.example.docker-groups-0.1.0.jar
+$ exists target/docker/stage/opt/docker/other
 $ exists target/docker/stage/2
+-$ exists target/docker/stage/2/opt/docker/lib/org.slf4j.slf4j-api-1.7.30.jar
+$ exists target/docker/stage/2/opt/docker/lib/com.example.docker-groups-0.1.0.jar
 $ exists target/docker/stage/54/opt/docker/spark
 
-$ exec bash -c 'docker run --rm --entrypoint=ls docker-groups:0.1.0 |tr "\n" "," | grep -q "bin,lib,spark"'
+$ exec bash -c 'docker run --rm --entrypoint=ls docker-groups:0.1.0 |tr "\n" "," | grep -q "bin,lib,other,spark"'
 $ exec bash -c 'docker rmi docker-groups:0.1.0'
 
 $ copy-file changes/nolayers.sbt layers.sbt
 > reload
+> clean
 > docker:publishLocal
 $ exists target/docker/stage/opt/docker/bin
 $ exists target/docker/stage/opt/docker/spark
-$ exec bash -c 'docker run --rm --entrypoint=ls docker-groups:0.1.0 |tr "\n" "," | grep -q "bin,lib,spark"'
+-$ exists target/docker/stage/1
+-$ exists target/docker/stage/2
+-$ exists target/docker/stage/54
+$ exists target/docker/stage/opt/docker/lib/org.slf4j.slf4j-api-1.7.30.jar
+$ exists target/docker/stage/opt/docker/lib/com.example.docker-groups-0.1.0.jar
+$ exec bash -c 'docker run --rm --entrypoint=ls docker-groups:0.1.0 |tr "\n" "," | grep -q "bin,lib,other,spark"'