[WX-1391] Fix Bash Bug (#7326)

broadinstitute · Jan 14, 2024 · 85821ef · 85821ef
1 parent a91499b
commit 85821ef
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 22 deletions.
diff --git a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala
@@ -61,6 +61,10 @@ case class DefaultStandardAsyncExecutionActorParams
   override val minimumRuntimeSettings: MinimumRuntimeSettings
 ) extends StandardAsyncExecutionActorParams
 
+// Typically we want to "executeInSubshell" for encapsulation of bash code.
+// Override to `false` when we need the script to set an environment variable in the parent shell.
+case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean = true)
+
 /**
   * An extension of the generic AsyncBackendJobExecutionActor providing a standard abstract implementation of an
   * asynchronous polling backend.
@@ -328,7 +332,7 @@ trait StandardAsyncExecutionActor
   }
 
   /** Any custom code that should be run within commandScriptContents before the instantiated command. */
-  def scriptPreamble: ErrorOr[String] = "".valid
+  def scriptPreamble: ErrorOr[ScriptPreambleData] = ScriptPreambleData("").valid
 
   def cwd: Path = commandDirectory
   def rcPath: Path = cwd./(jobPaths.returnCodeFilename)
@@ -426,7 +430,22 @@ trait StandardAsyncExecutionActor
            |find . -type d -exec sh -c '[ -z "$$(ls -A '"'"'{}'"'"')" ] && touch '"'"'{}'"'"'/.file' \\;
            |)""".stripMargin)
 
-    val errorOrPreamble: ErrorOr[String] = scriptPreamble
+    val errorOrPreamble: ErrorOr[String] = scriptPreamble.map{ preambleData =>
+        preambleData.executeInSubshell match {
+          case true =>
+            s"""
+               |(
+               |cd ${cwd.pathAsString}
+               |${preambleData.bashString}
+               |)
+               |""".stripMargin
+          case false =>
+            s"""
+               |cd ${cwd.pathAsString}
+               |${preambleData.bashString}
+               |""".stripMargin
+        }
+    }
 
     // The `tee` trickery below is to be able to redirect to known filenames for CWL while also streaming
     // stdout and stderr for PAPI to periodically upload to cloud storage.
@@ -440,10 +459,9 @@ trait StandardAsyncExecutionActor
         |export _JAVA_OPTIONS=-Djava.io.tmpdir="$$tmpDir"
         |export TMPDIR="$$tmpDir"
         |export HOME="$home"
-        |(
-        |cd ${cwd.pathAsString}
+        |
         |SCRIPT_PREAMBLE
-        |)
+        |
         |$out="$${tmpDir}/out.$$$$" $err="$${tmpDir}/err.$$$$"
         |mkfifo "$$$out" "$$$err"
         |trap 'rm "$$$out" "$$$err"' EXIT

diff --git a/...in/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala b/...in/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala
@@ -23,8 +23,9 @@ import cromwell.backend.google.batch.runnable.WorkflowOptionKeys
 import cromwell.backend.google.batch.util.{GcpBatchReferenceFilesMappingOperations, RuntimeOutputMapping}
 import cromwell.filesystems.gcs.GcsPathBuilder
 import cromwell.filesystems.gcs.GcsPathBuilder.ValidFullGcsPath
+
 import java.io.FileNotFoundException
-import cromwell.backend.standard.{StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
+import cromwell.backend.standard.{ScriptPreambleData, StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
 import cromwell.core._
 import cromwell.core.io.IoCommandBuilder
 import cromwell.core.path.{DefaultPathBuilder, Path}
@@ -49,6 +50,7 @@ import wom.core.FullyQualifiedName
 import wom.expression.{FileEvaluation, NoIoFunctionSet}
 import wom.format.MemorySize
 import wom.values._
+
 import java.io.OutputStreamWriter
 import java.nio.charset.Charset
 import java.util.Base64
@@ -663,12 +665,13 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar
   private val DockerMonitoringLogPath: Path = GcpBatchWorkingDisk.MountPoint.resolve(gcpBatchCallPaths.batchMonitoringLogFilename)
   private val DockerMonitoringScriptPath: Path = GcpBatchWorkingDisk.MountPoint.resolve(gcpBatchCallPaths.batchMonitoringScriptFilename)
 
-  override def scriptPreamble: ErrorOr[String] = {
-    if (monitoringOutput.isDefined) {
+  override def scriptPreamble: ErrorOr[ScriptPreambleData] = {
+    if (monitoringOutput.isDefined)
+      ScriptPreambleData(
       s"""|touch $DockerMonitoringLogPath
           |chmod u+x $DockerMonitoringScriptPath
-          |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin.valid
-    } else "".valid
+          |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin).valid
+    else ScriptPreambleData("").valid
   }
 
   private[actors] def generateInputs(jobDescriptor: BackendJobDescriptor): Set[GcpBatchInput] = {

diff --git a/.../cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala b/.../cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala
@@ -1,7 +1,6 @@
 package cromwell.backend.google.pipelines.common
 
 import java.net.SocketTimeoutException
-
 import _root_.io.grpc.Status
 import akka.actor.ActorRef
 import akka.http.scaladsl.model.{ContentType, ContentTypes}
@@ -27,7 +26,7 @@ import cromwell.backend.google.pipelines.common.errors.FailedToDelocalizeFailure
 import cromwell.backend.google.pipelines.common.io._
 import cromwell.backend.google.pipelines.common.monitoring.{CheckpointingConfiguration, MonitoringImage}
 import cromwell.backend.io.DirectoryFunctions
-import cromwell.backend.standard.{StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
+import cromwell.backend.standard.{ScriptPreambleData, StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
 import cromwell.core._
 import cromwell.core.io.IoCommandBuilder
 import cromwell.core.path.{DefaultPathBuilder, Path}
@@ -380,12 +379,13 @@ class PipelinesApiAsyncBackendJobExecutionActor(override val standardParams: Sta
 
   private lazy val isDockerImageCacheUsageRequested = runtimeAttributes.useDockerImageCache.getOrElse(useDockerImageCache(jobDescriptor.workflowDescriptor))
 
-  override def scriptPreamble: ErrorOr[String] = {
-    if (monitoringOutput.isDefined) {
+  override def scriptPreamble: ErrorOr[ScriptPreambleData] = {
+    if (monitoringOutput.isDefined)
+      ScriptPreambleData(
       s"""|touch $DockerMonitoringLogPath
           |chmod u+x $DockerMonitoringScriptPath
-          |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin
-    }.valid else "".valid
+          |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin).valid
+     else ScriptPreambleData("").valid
   }
 
   override def globParentDirectory(womGlobFile: WomGlobFile): Path = {

diff --git a/...kends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala b/...kends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala
@@ -17,7 +17,7 @@ import cromwell.backend.BackendJobLifecycleActor
 import cromwell.backend.async.{AbortedExecutionHandle, ExecutionHandle, FailedNonRetryableExecutionHandle, PendingExecutionHandle}
 import cromwell.backend.impl.tes.TesAsyncBackendJobExecutionActor.{determineWSMSasEndpointFromInputs, generateLocalizedSasScriptPreamble}
 import cromwell.backend.impl.tes.TesResponseJsonFormatter._
-import cromwell.backend.standard.{StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
+import cromwell.backend.standard.{ScriptPreambleData, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
 import cromwell.core.logging.JobLogger
 import cromwell.core.path.{DefaultPathBuilder, Path}
 import cromwell.core.retry.Retry._
@@ -123,7 +123,7 @@ object TesAsyncBackendJobExecutionActor {
        |export $environmentVariableName=$$(echo "$${sas_response_json}" | jq -r '.token')
        |
        |# Echo the first characters for logging/debugging purposes. "null" indicates something went wrong.
-       |echo Saving sas token: $${$environmentVariableName:0:4}**** to environment variable $environmentVariableName...
+       |echo "Saving sas token: $${$environmentVariableName:0:4}**** to environment variable $environmentVariableName..."
        |### END ACQUIRE LOCAL SAS TOKEN ###
        |""".stripMargin
   }
@@ -213,7 +213,7 @@ class TesAsyncBackendJobExecutionActor(override val standardParams: StandardAsyn
    *
    * @return Bash code to run at the start of a task.
    */
-  override def scriptPreamble: ErrorOr[String] = {
+  override def scriptPreamble: ErrorOr[ScriptPreambleData] = {
     runtimeAttributes.localizedSasEnvVar match {
       case Some(environmentVariableName) => { // Case: user wants a sas token. Return the computed preamble or die trying.
         val workflowName = workflowDescriptor.callable.name
@@ -222,9 +222,9 @@ class TesAsyncBackendJobExecutionActor(override val standardParams: StandardAsyn
         }
         val taskInputs: List[Input] = TesTask.buildTaskInputs(callInputFiles, workflowName, mapCommandLineWomFile)
         val computedEndpoint = determineWSMSasEndpointFromInputs(taskInputs, getPath, jobLogger)
-        computedEndpoint.map(endpoint => generateLocalizedSasScriptPreamble(environmentVariableName, endpoint))
+        computedEndpoint.map(endpoint => ScriptPreambleData(generateLocalizedSasScriptPreamble(environmentVariableName, endpoint), executeInSubshell =  false))
       }.toErrorOr
-      case _ => "".valid // Case: user doesn't want a sas token. Empty preamble is the correct preamble.
+      case _ => ScriptPreambleData("", executeInSubshell = false).valid // Case: user doesn't want a sas token. Empty preamble is the correct preamble.
     }
   }
 

diff --git a/...s/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala b/...s/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala
@@ -144,12 +144,13 @@ class TesAsyncBackendJobExecutionActorSpec extends AnyFlatSpec with Matchers wit
         |                    -H "Authorization: Bearer $${BEARER_TOKEN}")
         |""".stripMargin
     val exportCommandSubstring = s"""export $mockEnvironmentVariableNameFromWom=$$(echo "$${sas_response_json}" | jq -r '.token')"""
-
+    val echoCommandSubstring = s"""echo "Saving sas token: $${$mockEnvironmentVariableNameFromWom:0:4}**** to environment variable $mockEnvironmentVariableNameFromWom...""""
     val generatedBashScript = TesAsyncBackendJobExecutionActor.generateLocalizedSasScriptPreamble(mockEnvironmentVariableNameFromWom, expectedEndpoint)
 
     generatedBashScript should include (beginSubstring)
     generatedBashScript should include (endSubstring)
     generatedBashScript should include (curlCommandSubstring)
+    generatedBashScript should include (echoCommandSubstring)
     generatedBashScript should include (exportCommandSubstring)
   }
 }