Skip to content

Commit 85821ef

Browse files
THWisemanAlexITC
THWiseman
authored and
AlexITC
committed
[WX-1391] Fix Bash Bug (#7326)
1 parent a91499b commit 85821ef

File tree

5 files changed

+44
-22
lines changed

5 files changed

+44
-22
lines changed

‎backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ case class DefaultStandardAsyncExecutionActorParams
6161
override val minimumRuntimeSettings: MinimumRuntimeSettings
6262
) extends StandardAsyncExecutionActorParams
6363

64+
// Typically we want to "executeInSubshell" for encapsulation of bash code.
65+
// Override to `false` when we need the script to set an environment variable in the parent shell.
66+
case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean = true)
67+
6468
/**
6569
* An extension of the generic AsyncBackendJobExecutionActor providing a standard abstract implementation of an
6670
* asynchronous polling backend.
@@ -328,7 +332,7 @@ trait StandardAsyncExecutionActor
328332
}
329333

330334
/** Any custom code that should be run within commandScriptContents before the instantiated command. */
331-
def scriptPreamble: ErrorOr[String] = "".valid
335+
def scriptPreamble: ErrorOr[ScriptPreambleData] = ScriptPreambleData("").valid
332336

333337
def cwd: Path = commandDirectory
334338
def rcPath: Path = cwd./(jobPaths.returnCodeFilename)
@@ -426,7 +430,22 @@ trait StandardAsyncExecutionActor
426430
|find . -type d -exec sh -c '[ -z "$$(ls -A '"'"'{}'"'"')" ] && touch '"'"'{}'"'"'/.file' \\;
427431
|)""".stripMargin)
428432

429-
val errorOrPreamble: ErrorOr[String] = scriptPreamble
433+
val errorOrPreamble: ErrorOr[String] = scriptPreamble.map{ preambleData =>
434+
preambleData.executeInSubshell match {
435+
case true =>
436+
s"""
437+
|(
438+
|cd ${cwd.pathAsString}
439+
|${preambleData.bashString}
440+
|)
441+
|""".stripMargin
442+
case false =>
443+
s"""
444+
|cd ${cwd.pathAsString}
445+
|${preambleData.bashString}
446+
|""".stripMargin
447+
}
448+
}
430449

431450
// The `tee` trickery below is to be able to redirect to known filenames for CWL while also streaming
432451
// stdout and stderr for PAPI to periodically upload to cloud storage.
@@ -440,10 +459,9 @@ trait StandardAsyncExecutionActor
440459
|export _JAVA_OPTIONS=-Djava.io.tmpdir="$$tmpDir"
441460
|export TMPDIR="$$tmpDir"
442461
|export HOME="$home"
443-
|(
444-
|cd ${cwd.pathAsString}
462+
|
445463
|SCRIPT_PREAMBLE
446-
|)
464+
|
447465
|$out="$${tmpDir}/out.$$$$" $err="$${tmpDir}/err.$$$$"
448466
|mkfifo "$$$out" "$$$err"
449467
|trap 'rm "$$$out" "$$$err"' EXIT

‎supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ import cromwell.backend.google.batch.runnable.WorkflowOptionKeys
2323
import cromwell.backend.google.batch.util.{GcpBatchReferenceFilesMappingOperations, RuntimeOutputMapping}
2424
import cromwell.filesystems.gcs.GcsPathBuilder
2525
import cromwell.filesystems.gcs.GcsPathBuilder.ValidFullGcsPath
26+
2627
import java.io.FileNotFoundException
27-
import cromwell.backend.standard.{StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
28+
import cromwell.backend.standard.{ScriptPreambleData, StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
2829
import cromwell.core._
2930
import cromwell.core.io.IoCommandBuilder
3031
import cromwell.core.path.{DefaultPathBuilder, Path}
@@ -49,6 +50,7 @@ import wom.core.FullyQualifiedName
4950
import wom.expression.{FileEvaluation, NoIoFunctionSet}
5051
import wom.format.MemorySize
5152
import wom.values._
53+
5254
import java.io.OutputStreamWriter
5355
import java.nio.charset.Charset
5456
import java.util.Base64
@@ -663,12 +665,13 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar
663665
private val DockerMonitoringLogPath: Path = GcpBatchWorkingDisk.MountPoint.resolve(gcpBatchCallPaths.batchMonitoringLogFilename)
664666
private val DockerMonitoringScriptPath: Path = GcpBatchWorkingDisk.MountPoint.resolve(gcpBatchCallPaths.batchMonitoringScriptFilename)
665667

666-
override def scriptPreamble: ErrorOr[String] = {
667-
if (monitoringOutput.isDefined) {
668+
override def scriptPreamble: ErrorOr[ScriptPreambleData] = {
669+
if (monitoringOutput.isDefined)
670+
ScriptPreambleData(
668671
s"""|touch $DockerMonitoringLogPath
669672
|chmod u+x $DockerMonitoringScriptPath
670-
|$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin.valid
671-
} else "".valid
673+
|$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin).valid
674+
else ScriptPreambleData("").valid
672675
}
673676

674677
private[actors] def generateInputs(jobDescriptor: BackendJobDescriptor): Set[GcpBatchInput] = {

‎supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package cromwell.backend.google.pipelines.common
22

33
import java.net.SocketTimeoutException
4-
54
import _root_.io.grpc.Status
65
import akka.actor.ActorRef
76
import akka.http.scaladsl.model.{ContentType, ContentTypes}
@@ -27,7 +26,7 @@ import cromwell.backend.google.pipelines.common.errors.FailedToDelocalizeFailure
2726
import cromwell.backend.google.pipelines.common.io._
2827
import cromwell.backend.google.pipelines.common.monitoring.{CheckpointingConfiguration, MonitoringImage}
2928
import cromwell.backend.io.DirectoryFunctions
30-
import cromwell.backend.standard.{StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
29+
import cromwell.backend.standard.{ScriptPreambleData, StandardAdHocValue, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
3130
import cromwell.core._
3231
import cromwell.core.io.IoCommandBuilder
3332
import cromwell.core.path.{DefaultPathBuilder, Path}
@@ -380,12 +379,13 @@ class PipelinesApiAsyncBackendJobExecutionActor(override val standardParams: Sta
380379

381380
private lazy val isDockerImageCacheUsageRequested = runtimeAttributes.useDockerImageCache.getOrElse(useDockerImageCache(jobDescriptor.workflowDescriptor))
382381

383-
override def scriptPreamble: ErrorOr[String] = {
384-
if (monitoringOutput.isDefined) {
382+
override def scriptPreamble: ErrorOr[ScriptPreambleData] = {
383+
if (monitoringOutput.isDefined)
384+
ScriptPreambleData(
385385
s"""|touch $DockerMonitoringLogPath
386386
|chmod u+x $DockerMonitoringScriptPath
387-
|$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin
388-
}.valid else "".valid
387+
|$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin).valid
388+
else ScriptPreambleData("").valid
389389
}
390390

391391
override def globParentDirectory(womGlobFile: WomGlobFile): Path = {

‎supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import cromwell.backend.BackendJobLifecycleActor
1717
import cromwell.backend.async.{AbortedExecutionHandle, ExecutionHandle, FailedNonRetryableExecutionHandle, PendingExecutionHandle}
1818
import cromwell.backend.impl.tes.TesAsyncBackendJobExecutionActor.{determineWSMSasEndpointFromInputs, generateLocalizedSasScriptPreamble}
1919
import cromwell.backend.impl.tes.TesResponseJsonFormatter._
20-
import cromwell.backend.standard.{StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
20+
import cromwell.backend.standard.{ScriptPreambleData, StandardAsyncExecutionActor, StandardAsyncExecutionActorParams, StandardAsyncJob}
2121
import cromwell.core.logging.JobLogger
2222
import cromwell.core.path.{DefaultPathBuilder, Path}
2323
import cromwell.core.retry.Retry._
@@ -123,7 +123,7 @@ object TesAsyncBackendJobExecutionActor {
123123
|export $environmentVariableName=$$(echo "$${sas_response_json}" | jq -r '.token')
124124
|
125125
|# Echo the first characters for logging/debugging purposes. "null" indicates something went wrong.
126-
|echo Saving sas token: $${$environmentVariableName:0:4}**** to environment variable $environmentVariableName...
126+
|echo "Saving sas token: $${$environmentVariableName:0:4}**** to environment variable $environmentVariableName..."
127127
|### END ACQUIRE LOCAL SAS TOKEN ###
128128
|""".stripMargin
129129
}
@@ -213,7 +213,7 @@ class TesAsyncBackendJobExecutionActor(override val standardParams: StandardAsyn
213213
*
214214
* @return Bash code to run at the start of a task.
215215
*/
216-
override def scriptPreamble: ErrorOr[String] = {
216+
override def scriptPreamble: ErrorOr[ScriptPreambleData] = {
217217
runtimeAttributes.localizedSasEnvVar match {
218218
case Some(environmentVariableName) => { // Case: user wants a sas token. Return the computed preamble or die trying.
219219
val workflowName = workflowDescriptor.callable.name
@@ -222,9 +222,9 @@ class TesAsyncBackendJobExecutionActor(override val standardParams: StandardAsyn
222222
}
223223
val taskInputs: List[Input] = TesTask.buildTaskInputs(callInputFiles, workflowName, mapCommandLineWomFile)
224224
val computedEndpoint = determineWSMSasEndpointFromInputs(taskInputs, getPath, jobLogger)
225-
computedEndpoint.map(endpoint => generateLocalizedSasScriptPreamble(environmentVariableName, endpoint))
225+
computedEndpoint.map(endpoint => ScriptPreambleData(generateLocalizedSasScriptPreamble(environmentVariableName, endpoint), executeInSubshell = false))
226226
}.toErrorOr
227-
case _ => "".valid // Case: user doesn't want a sas token. Empty preamble is the correct preamble.
227+
case _ => ScriptPreambleData("", executeInSubshell = false).valid // Case: user doesn't want a sas token. Empty preamble is the correct preamble.
228228
}
229229
}
230230

‎supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,12 +144,13 @@ class TesAsyncBackendJobExecutionActorSpec extends AnyFlatSpec with Matchers wit
144144
| -H "Authorization: Bearer $${BEARER_TOKEN}")
145145
|""".stripMargin
146146
val exportCommandSubstring = s"""export $mockEnvironmentVariableNameFromWom=$$(echo "$${sas_response_json}" | jq -r '.token')"""
147-
147+
val echoCommandSubstring = s"""echo "Saving sas token: $${$mockEnvironmentVariableNameFromWom:0:4}**** to environment variable $mockEnvironmentVariableNameFromWom...""""
148148
val generatedBashScript = TesAsyncBackendJobExecutionActor.generateLocalizedSasScriptPreamble(mockEnvironmentVariableNameFromWom, expectedEndpoint)
149149

150150
generatedBashScript should include (beginSubstring)
151151
generatedBashScript should include (endSubstring)
152152
generatedBashScript should include (curlCommandSubstring)
153+
generatedBashScript should include (echoCommandSubstring)
153154
generatedBashScript should include (exportCommandSubstring)
154155
}
155156
}

0 commit comments

Comments
 (0)