WX-1340 GCP Batch: Mount with extra colon issue and multiple zones su…

…pport (#7240) Co-authored-by: Adam Nichols <anichols@broadinstitute.org>
broadinstitute · Nov 1, 2023 · 5bc7ac3 · 5bc7ac3
1 parent 4d16f01
commit 5bc7ac3
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 7 deletions.
diff --git a/.../google/batch/src/main/scala/cromwell/backend/google/batch/runnable/RunnableBuilder.scala b/.../google/batch/src/main/scala/cromwell/backend/google/batch/runnable/RunnableBuilder.scala
@@ -4,7 +4,6 @@ import com.google.cloud.batch.v1.Runnable.Container
 import com.google.cloud.batch.v1.{Environment,Runnable, Volume}
 import cromwell.backend.google.batch.models.GcpBatchConfigurationAttributes.GcsTransferConfiguration
 import cromwell.backend.google.batch.models.{BatchParameter, GcpBatchInput, GcpBatchOutput}
-//import cromwell.backend.google.batch.runnable.RunnableLabels._
 import cromwell.core.path.Path
 import mouse.all.anySyntaxMouse
 
@@ -60,8 +59,14 @@ object RunnableBuilder {
     def withVolumes(volumes: List[Volume]): Runnable.Builder = {
       val formattedVolumes = volumes.map { volume =>
         val mountPath = volume.getMountPath
-        val mountOptions = Option(volume.getMountOptionsList).map(_.asScala.toList).getOrElse(List.empty)
-        s"$mountPath:$mountPath:${mountOptions.mkString(",")}"
+
+        val mountOptions = Option(volume.getMountOptionsList)
+          .map(_.asScala)
+          .filter(_.nonEmpty)
+          .map(_.mkString(":", ",", ""))
+          .getOrElse("")
+
+        s"$mountPath:$mountPath$mountOptions"
       }
 
       builder.setContainer(

diff --git a/...gle/batch/src/main/scala/cromwell/backend/google/batch/util/BatchUtilityConversions.scala b/...gle/batch/src/main/scala/cromwell/backend/google/batch/util/BatchUtilityConversions.scala
@@ -8,10 +8,9 @@ import wom.format.MemorySize
 
 trait BatchUtilityConversions {
 
-
   // construct zones string
   def toZonesPath(zones: Vector[String]): String = {
-    "zones/" + zones.mkString(",")
+    zones.map(zone => "zones/" + zone).mkString(" ")
   }
 
   // lowercase text to match gcp label requirements

diff --git a/...gle/batch/src/test/scala/cromwell/backend/google/batch/runnable/RunnableBuilderSpec.scala b/...gle/batch/src/test/scala/cromwell/backend/google/batch/runnable/RunnableBuilderSpec.scala
@@ -76,7 +76,7 @@ class RunnableBuilderSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matc
     runnable.getContainer.getCommandsList.asScala shouldBe expectedCommand
     runnable.getAlwaysRun shouldBe true
     runnable.getLabelsMap shouldBe memoryRetryRunnableExpectedLabels
-    runnable.getContainer.getVolumesList.asScala.toList shouldBe volumes.map(v => s"${v.getMountPath}:${v.getMountPath}:")
+    runnable.getContainer.getVolumesList.asScala.toList shouldBe volumes.map(v => s"${v.getMountPath}:${v.getMountPath}")
   }
 
   it should "return cloud sdk runnable for multiple keys in retry-with-double-memory" in {
@@ -89,6 +89,6 @@ class RunnableBuilderSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matc
     runnable.getContainer.getCommandsList.asScala shouldBe expectedCommand
     runnable.getAlwaysRun shouldBe true
     runnable.getLabelsMap shouldBe memoryRetryRunnableExpectedLabels
-    runnable.getContainer.getVolumesList.asScala.toList shouldBe volumes.map(v => s"${v.getMountPath}:${v.getMountPath}:")
+    runnable.getContainer.getVolumesList.asScala.toList shouldBe volumes.map(v => s"${v.getMountPath}:${v.getMountPath}")
   }
 }