Add a config option to ignore the noAddress runtime attribute [BA-5739]…

… (#5569)
broadinstitute · Jul 10, 2020 · 017de78 · 017de78
1 parent 8aabeca
commit 017de78
Show file tree

Hide file tree

Showing 14 changed files with 91 additions and 21 deletions.
diff --git a/centaur/src/main/resources/standardTestCases/ignore_noAddress.test b/centaur/src/main/resources/standardTestCases/ignore_noAddress.test
@@ -0,0 +1,14 @@
+# NB: To request this test by name, make it lowercase, eg sbt "centaur/it:testOnly * -- -n ignore_noaddress"
+name: ignore_noAddress
+backends: [Papi,Papiv2]
+backendsMode: any
+testFormat: workflowsuccess
+
+files {
+  workflow: ignore_noAddress/ignore_noAddress.wdl
+}
+
+metadata {
+  workflowName: ignore_noAddress
+  status: Succeeded
+}
diff --git a/centaur/src/main/resources/standardTestCases/ignore_noAddress/ignore_noAddress.wdl b/centaur/src/main/resources/standardTestCases/ignore_noAddress/ignore_noAddress.wdl
@@ -0,0 +1,17 @@
+task task_with_noAddress {
+
+    command { echo "hello, world" }
+
+    output { String s = read_string(stdout()) }
+
+    runtime {
+      docker: "ubuntu:latest"
+      # NB: This is ignored because of the config in the centaur configuration.
+      # If we didn't have that, this task would run forever - or at least until PAPI times it out.
+      noAddress: true
+    }
+}
+
+workflow ignore_noAddress {
+    call task_with_noAddress
+}
diff --git a/cromwell.example.backends/PAPIv2.conf b/cromwell.example.backends/PAPIv2.conf
@@ -28,6 +28,11 @@ backend {
         # Emit a warning if jobs last longer than this amount of time. This might indicate that something got stuck in PAPI.
         slow-job-warning-time: 24 hours
 
+        # Whether to allow users to specify the 'noAddress' runtime attribute when submitting jobs.
+        # Defaults to true.
+        # If set to false, the noAddress attribute will be ignored and public addresses will always be assigned.
+        allow-noAddress-attribute: true
+
         # Set this to the lower of the two values "Queries per 100 seconds" and "Queries per 100 seconds per user" for
         # your project.
         #

diff --git a/src/ci/resources/papi_v1_v2alpha1_provider_config.inc.conf b/src/ci/resources/papi_v1_v2alpha1_provider_config.inc.conf
@@ -16,3 +16,5 @@ filesystems {
 }
 
 slow-job-warning-time: 20 minutes
+
+allow-noAddress-attribute: false
diff --git a/src/ci/resources/papi_v2beta_provider_config.inc.conf b/src/ci/resources/papi_v2beta_provider_config.inc.conf
@@ -17,3 +17,5 @@ filesystems {
 }
 
 slow-job-warning-time: 20 minutes
+
+allow-noAddress-attribute: false
diff --git a/.../cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala b/.../cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala
@@ -458,6 +458,7 @@ class PipelinesApiAsyncBackendJobExecutionActor(override val standardParams: Sta
           virtualPrivateCloudConfiguration = jesAttributes.virtualPrivateCloudConfiguration,
           retryWithMoreMemoryKeys = jesAttributes.memoryRetryConfiguration.map(_.errorKeys),
           fuseEnabled = fuseEnabled(jobDescriptor.workflowDescriptor),
+          allowNoAddress = pipelinesConfiguration.papiAttributes.allowNoAddress
         )
       case Some(other) =>
         throw new RuntimeException(s"Unexpected initialization data: $other")

diff --git a/.../scala/cromwell/backend/google/pipelines/common/PipelinesApiConfigurationAttributes.scala b/.../scala/cromwell/backend/google/pipelines/common/PipelinesApiConfigurationAttributes.scala
@@ -43,7 +43,8 @@ case class PipelinesApiConfigurationAttributes(project: String,
                                                gcsTransferConfiguration: GcsTransferConfiguration,
                                                virtualPrivateCloudConfiguration: Option[VirtualPrivateCloudConfiguration],
                                                batchRequestTimeoutConfiguration: BatchRequestTimeoutConfiguration,
-                                               memoryRetryConfiguration: Option[MemoryRetryConfiguration])
+                                               memoryRetryConfiguration: Option[MemoryRetryConfiguration],
+                                               allowNoAddress: Boolean)
 
 object PipelinesApiConfigurationAttributes {
 
@@ -64,6 +65,8 @@ object PipelinesApiConfigurationAttributes {
 
   lazy val DefaultMemoryRetryFactor: GreaterEqualRefined = refineMV[GreaterEqualOne](2.0)
 
+  val allowNoAddressAttributeKey = "allow-noAddress-attribute"
+
   private val papiKeys = CommonBackendConfigurationAttributes.commonValidConfigurationAttributeKeys ++ Set(
     "project",
     "root",
@@ -98,6 +101,7 @@ object PipelinesApiConfigurationAttributes {
     "virtual-private-cloud.auth",
     "memory-retry.error-keys",
     "memory-retry.multiplier",
+    allowNoAddressAttributeKey
   )
 
   private val deprecatedJesKeys: Map[String, String] = Map(
@@ -149,6 +153,7 @@ object PipelinesApiConfigurationAttributes {
     val genomicsAuthName: ErrorOr[String] = validate { backendConfig.as[String]("genomics.auth") }
     val genomicsRestrictMetadataAccess: ErrorOr[Boolean] = validate { backendConfig.as[Option[Boolean]]("genomics.restrict-metadata-access").getOrElse(false) }
     val genomicsEnableFuse: ErrorOr[Boolean] = validate { backendConfig.as[Option[Boolean]]("genomics.enable-fuse").getOrElse(false) }
+    val allowNoAddress: ErrorOr[Boolean] = validate { backendConfig.as[Option[Boolean]](allowNoAddressAttributeKey).getOrElse(true) }
     val gcsFilesystemAuthName: ErrorOr[String] = validate { backendConfig.as[String]("filesystems.gcs.auth") }
     val qpsValidation = validateQps(backendConfig)
     val duplicationStrategy = validate { backendConfig.as[Option[String]]("filesystems.gcs.caching.duplication-strategy").getOrElse("copy") match {
@@ -217,7 +222,8 @@ object PipelinesApiConfigurationAttributes {
                                                        gcsTransferConfiguration: GcsTransferConfiguration,
                                                        virtualPrivateCloudConfiguration: Option[VirtualPrivateCloudConfiguration],
                                                        batchRequestTimeoutConfiguration: BatchRequestTimeoutConfiguration,
-                                                       memoryRetryConfig: Option[MemoryRetryConfiguration]): ErrorOr[PipelinesApiConfigurationAttributes] =
+                                                       memoryRetryConfig: Option[MemoryRetryConfiguration],
+                                                       allowNoAddress: Boolean): ErrorOr[PipelinesApiConfigurationAttributes] =
       (googleConfig.auth(genomicsName), googleConfig.auth(gcsName)) mapN {
         (genomicsAuth, gcsAuth) =>
           PipelinesApiConfigurationAttributes(
@@ -239,6 +245,7 @@ object PipelinesApiConfigurationAttributes {
             virtualPrivateCloudConfiguration = virtualPrivateCloudConfiguration,
             batchRequestTimeoutConfiguration = batchRequestTimeoutConfiguration,
             memoryRetryConfiguration = memoryRetryConfig,
+            allowNoAddress
           )
     }
 
@@ -257,6 +264,7 @@ object PipelinesApiConfigurationAttributes {
       virtualPrivateCloudConfiguration,
       batchRequestTimeoutConfigurationValidation,
       memoryRetryConfig,
+      allowNoAddress
     ) flatMapN authGoogleConfigForPapiConfigurationAttributes match {
       case Valid(r) => r
       case Invalid(f) =>

diff --git a/.../main/scala/cromwell/backend/google/pipelines/common/api/PipelinesApiRequestFactory.scala b/.../main/scala/cromwell/backend/google/pipelines/common/api/PipelinesApiRequestFactory.scala
@@ -82,10 +82,14 @@ object PipelinesApiRequestFactory {
                                       adjustedSizeDisks: Seq[PipelinesApiAttachedDisk],
                                       virtualPrivateCloudConfiguration: Option[VirtualPrivateCloudConfiguration],
                                       retryWithMoreMemoryKeys: Option[List[String]],
-                                      fuseEnabled: Boolean) {
+                                      fuseEnabled: Boolean,
+                                      allowNoAddress: Boolean) {
     def literalInputs = inputOutputParameters.literalInputParameters
     def inputParameters = inputOutputParameters.fileInputParameters
     def outputParameters = inputOutputParameters.fileOutputParameters
     def allParameters = inputParameters ++ outputParameters
+
+    // Takes into account the 'noAddress' runtime attribute and the allowNoAddress configuration option:
+    def effectiveNoAddressValue: Boolean = allowNoAddress && runtimeAttributes.noAddress
   }
 }
diff --git a/...c/test/scala/cromwell/backend/google/pipelines/common/PipelinesApiConfigurationSpec.scala b/...c/test/scala/cromwell/backend/google/pipelines/common/PipelinesApiConfigurationSpec.scala
@@ -126,6 +126,21 @@ class PipelinesApiConfigurationSpec extends FlatSpec with Matchers with TableDri
     dockerConf.get.token shouldBe "dockerToken"
   }
 
+  it should "correctly default allowNoAddress to true" in {
+    val noAddressConf = new PipelinesApiConfiguration(BackendConfigurationDescriptor(backendConfig, globalConfig), genomicsFactory, googleConfiguration, papiAttributes)
+    noAddressConf.papiAttributes.allowNoAddress should be(true)
+  }
+
+  it should "be able to set allowNoAddress to false" in {
+    val updatedBackendConfig = backendConfig.withValue(
+      PipelinesApiConfigurationAttributes.allowNoAddressAttributeKey,
+      ConfigValueFactory.fromAnyRef(false)
+    )
+    val updatedPapiAttributes = PipelinesApiConfigurationAttributes(googleConfiguration, updatedBackendConfig, "papi")
+    val noAddressConf = new PipelinesApiConfiguration(BackendConfigurationDescriptor(updatedBackendConfig, globalConfig), genomicsFactory, googleConfiguration, updatedPapiAttributes)
+    noAddressConf.papiAttributes.allowNoAddress should be(false)
+  }
+
   it should "have correct needAuthFileUpload" in {
     val configs = Table(
       ("backendConfig", "globalConfig"),

diff --git a/...end/google/pipelines/common/callcaching/PipelinesApiBackendCacheHitCopyingActorSpec.scala b/...end/google/pipelines/common/callcaching/PipelinesApiBackendCacheHitCopyingActorSpec.scala
@@ -422,6 +422,7 @@ class PipelinesApiBackendCacheHitCopyingActorSpec extends TestKitSuite("Pipeline
       virtualPrivateCloudConfiguration = None,
       batchRequestTimeoutConfiguration = null,
       memoryRetryConfiguration = None,
+      allowNoAddress = true
     )
 
     val papiConfiguration = mock[PipelinesApiConfiguration]

diff --git a/.../v1alpha2/src/main/scala/cromwell/backend/google/pipelines/v1alpha2/GenomicsFactory.scala b/.../v1alpha2/src/main/scala/cromwell/backend/google/pipelines/v1alpha2/GenomicsFactory.scala
@@ -37,7 +37,7 @@ case class GenomicsFactory(applicationName: String, authMode: GoogleAuthMode, en
         val commandLine = s"/bin/bash ${createPipelineParameters.commandScriptContainerPath.pathAsString}"
 
         val pipelineInfoBuilder = if (createPipelineParameters.preemptible) PreemptiblePipelineInfoBuilder else NonPreemptiblePipelineInfoBuilder
-        val pipelineInfo = pipelineInfoBuilder.build(commandLine, createPipelineParameters.runtimeAttributes, createPipelineParameters.dockerImage)
+        val pipelineInfo = pipelineInfoBuilder.build(commandLine, createPipelineParameters)
 
         val inputParameters: Map[String, PipelinesApiInput] = createPipelineParameters.inputParameters.map({ i => i.name -> i }).toMap
 
@@ -57,7 +57,7 @@ case class GenomicsFactory(applicationName: String, authMode: GoogleAuthMode, en
 
         // disks cannot have mount points at runtime, so set them null
         val runtimePipelineResources = {
-          val resources = pipelineInfoBuilder.build(commandLine, createPipelineParameters.runtimeAttributes, createPipelineParameters.dockerImage).resources
+          val resources = pipelineInfoBuilder.build(commandLine, createPipelineParameters).resources
           val disksWithoutMountPoint = resources.getDisks.asScala map {
             _.setMountPoint(null)
           }

diff --git a/...pha2/src/main/scala/cromwell/backend/google/pipelines/v1alpha2/api/PipelinesApiInfo.scala b/...pha2/src/main/scala/cromwell/backend/google/pipelines/v1alpha2/api/PipelinesApiInfo.scala
@@ -1,6 +1,7 @@
 package cromwell.backend.google.pipelines.v1alpha2.api
 
 import com.google.api.services.genomics.model.{DockerExecutor, PipelineResources}
+import cromwell.backend.google.pipelines.common.api.PipelinesApiRequestFactory.CreatePipelineParameters
 import cromwell.backend.google.pipelines.common.{GpuResource, PipelinesApiRuntimeAttributes}
 import cromwell.backend.google.pipelines.v1alpha2.PipelinesConversions._
 import wdl4s.parser.MemoryUnit
@@ -27,32 +28,33 @@ trait PipelineInfoBuilder {
     }
   }
 
-  def buildResources(runtimeAttributes: PipelinesApiRuntimeAttributes): PipelineResources = {
+  def buildResources(createPipelineParameters: CreatePipelineParameters): PipelineResources = {
+    val runtimeAttributes =createPipelineParameters.runtimeAttributes
     val resources = new PipelineResources()
       .setMinimumRamGb(runtimeAttributes.memory.to(MemoryUnit.GB).amount)
       .setMinimumCpuCores(runtimeAttributes.cpu.value)
       .setZones(runtimeAttributes.zones.asJava)
       .setDisks(runtimeAttributes.disks.map(_.toGoogleDisk).asJava)
       .setBootDiskSizeGb(runtimeAttributes.bootDiskSize)
-      .setNoAddress(runtimeAttributes.noAddress)
+      .setNoAddress(createPipelineParameters.effectiveNoAddressValue)
 
       setGpu(resources, runtimeAttributes)
   }
 
-  def build(commandLine: String, runtimeAttributes: PipelinesApiRuntimeAttributes, docker: String): PipelineInfo
+  def build(commandLine: String, createPipelineParameters: CreatePipelineParameters): PipelineInfo
 }
 
 object NonPreemptiblePipelineInfoBuilder extends PipelineInfoBuilder {
-  def build(commandLine: String, runtimeAttributes: PipelinesApiRuntimeAttributes, dockerImage: String): PipelineInfo = {
-    val resources = buildResources(runtimeAttributes).setPreemptible(false)
-    new NonPreemptiblePipelineInfoBuilder(resources, buildDockerExecutor(commandLine, dockerImage))
+  def build(commandLine: String, createPipelineParameters: CreatePipelineParameters): PipelineInfo = {
+    val resources = buildResources(createPipelineParameters).setPreemptible(false)
+    new NonPreemptiblePipelineInfoBuilder(resources, buildDockerExecutor(commandLine, createPipelineParameters.dockerImage))
   }
 }
 
 object PreemptiblePipelineInfoBuilder extends PipelineInfoBuilder {
-  def build(commandLine: String, runtimeAttributes: PipelinesApiRuntimeAttributes, dockerImage: String): PipelineInfo = {
-    val resources = buildResources(runtimeAttributes).setPreemptible(true)
-    new PreemptiblePipelineInfoBuilder(resources, buildDockerExecutor(commandLine, dockerImage))
+  def build(commandLine: String, createPipelineParameters: CreatePipelineParameters): PipelineInfo = {
+    val resources = buildResources(createPipelineParameters).setPreemptible(true)
+    new PreemptiblePipelineInfoBuilder(resources, buildDockerExecutor(commandLine, createPipelineParameters.dockerImage))
   }
 }
 

diff --git a/.../v2alpha1/src/main/scala/cromwell/backend/google/pipelines/v2alpha1/GenomicsFactory.scala b/.../v2alpha1/src/main/scala/cromwell/backend/google/pipelines/v2alpha1/GenomicsFactory.scala
@@ -96,15 +96,15 @@ case class GenomicsFactory(applicationName: String, authMode: GoogleAuthMode, en
         networkLabelOption match {
           case Some(networkLabel) =>
             val network = new Network()
-              .setUsePrivateAddress(createPipelineParameters.runtimeAttributes.noAddress)
+              .setUsePrivateAddress(createPipelineParameters.effectiveNoAddressValue)
               .setName(VirtualPrivateCloudNetworkPath.format(createPipelineParameters.projectId, networkLabel._2))
 
             subnetworkLabelOption foreach { case(_, subnet) => network.setSubnetwork(subnet) }
             network
           case None =>
             // Falling back to running the job on default network since the project does not provide the custom network
             // specifying keys in its metadata
-            new Network().setUsePrivateAddress(createPipelineParameters.runtimeAttributes.noAddress)
+            new Network().setUsePrivateAddress(createPipelineParameters.effectiveNoAddressValue)
         }
       }
 
@@ -120,7 +120,7 @@ case class GenomicsFactory(applicationName: String, authMode: GoogleAuthMode, en
 
       def createNetwork(): Network = {
         createPipelineParameters.virtualPrivateCloudConfiguration match {
-          case None => new Network().setUsePrivateAddress(createPipelineParameters.runtimeAttributes.noAddress)
+          case None => new Network().setUsePrivateAddress(createPipelineParameters.effectiveNoAddressValue)
           case Some(vpcConfig) =>
             createNetworkWithVPC(vpcConfig).handleErrorWith {
               e => IO.raiseError(new RuntimeException(s"Failed to create Network object for project `${createPipelineParameters.projectId}`. " +

diff --git a/.../v2beta/src/main/scala/cromwell/backend/google/pipelines/v2beta/LifeSciencesFactory.scala b/.../v2beta/src/main/scala/cromwell/backend/google/pipelines/v2beta/LifeSciencesFactory.scala
@@ -88,23 +88,22 @@ case class LifeSciencesFactory(applicationName: String, authMode: GoogleAuthMode
         }
       }
 
-
       def networkFromLabels(vpcConfig: VirtualPrivateCloudConfiguration, projectLabels: ProjectLabels): Network = {
         val networkLabelOption = projectLabels.labels.find(l => l._1.equals(vpcConfig.name))
         val subnetworkLabelOption = vpcConfig.subnetwork.flatMap(s => projectLabels.labels.find(l => l._1.equals(s)))
 
         networkLabelOption match {
           case Some(networkLabel) =>
             val network = new Network()
-              .setUsePrivateAddress(createPipelineParameters.runtimeAttributes.noAddress)
+              .setUsePrivateAddress(createPipelineParameters.effectiveNoAddressValue)
               .setNetwork(VirtualPrivateCloudNetworkPath.format(createPipelineParameters.projectId, networkLabel._2))
 
             subnetworkLabelOption foreach { case(_, subnet) => network.setSubnetwork(subnet) }
             network
           case None =>
             // Falling back to running the job on default network since the project does not provide the custom network
             // specifying keys in its metadata
-            new Network().setUsePrivateAddress(createPipelineParameters.runtimeAttributes.noAddress)
+            new Network().setUsePrivateAddress(createPipelineParameters.effectiveNoAddressValue)
         }
       }
 
@@ -120,7 +119,7 @@ case class LifeSciencesFactory(applicationName: String, authMode: GoogleAuthMode
 
       def createNetwork(): Network = {
         createPipelineParameters.virtualPrivateCloudConfiguration match {
-          case None => new Network().setUsePrivateAddress(createPipelineParameters.runtimeAttributes.noAddress)
+          case None => new Network().setUsePrivateAddress(createPipelineParameters.effectiveNoAddressValue)
           case Some(vpcConfig) =>
             createNetworkWithVPC(vpcConfig).handleErrorWith {
               e => IO.raiseError(new RuntimeException(s"Failed to create Network object for project `${createPipelineParameters.projectId}`. " +
Original file line number	Diff line number	Diff line change
Expand Up		@@ -16,3 +16,5 @@ filesystems {
		}

		slow-job-warning-time: 20 minutes

		allow-noAddress-attribute: false
Original file line number	Diff line number	Diff line change
Expand Up		@@ -17,3 +17,5 @@ filesystems {
		}

		slow-job-warning-time: 20 minutes

		allow-noAddress-attribute: false