From bf0d85c16babb3dc80964d7b906028e39221be43 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 1 Dec 2023 13:29:30 -0500 Subject: [PATCH 1/5] fix --- .../StandardAsyncExecutionActor.scala | 26 +++++++++++++++---- ...cpBatchAsyncBackendJobExecutionActor.scala | 14 ++++++---- ...inesApiAsyncBackendJobExecutionActor.scala | 13 +++++----- .../TesAsyncBackendJobExecutionActor.scala | 10 +++---- ...TesAsyncBackendJobExecutionActorSpec.scala | 3 ++- 5 files changed, 44 insertions(+), 22 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala index 59eb7f08269..b2cd86c3405 100644 --- a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala +++ b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala @@ -61,6 +61,8 @@ case class DefaultStandardAsyncExecutionActorParams override val minimumRuntimeSettings: MinimumRuntimeSettings ) extends StandardAsyncExecutionActorParams +case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean) + /** * An extension of the generic AsyncBackendJobExecutionActor providing a standard abstract implementation of an * asynchronous polling backend. @@ -328,7 +330,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("", executeInSubshell = true).valid def cwd: Path = commandDirectory def rcPath: Path = cwd./(jobPaths.returnCodeFilename) @@ -426,7 +428,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 +457,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/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala index 766a8f2552f..66be28a7c8f 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/google/batch/src/main/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,14 @@ 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, executeInSubshell = true).valid + else ScriptPreambleData("", executeInSubshell = true + ).valid } private[actors] def generateInputs(jobDescriptor: BackendJobDescriptor): Set[GcpBatchInput] = { diff --git a/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala b/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala index 942838f8125..1fc13722efb 100644 --- a/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/google/pipelines/common/src/main/scala/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,14 @@ 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, executeInSubshell = true + ).valid + else ScriptPreambleData("", executeInSubshell = true).valid } override def globParentDirectory(womGlobFile: WomGlobFile): Path = { diff --git a/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala b/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala index 100ed6137e9..92391296533 100644 --- a/supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/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/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala b/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala index a28fce3d445..b9082d2e01f 100644 --- a/supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala +++ b/supportedBackends/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) } } From c0578dc2bcf981ed97f0f5f80e78cb07ad7ce314 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 1 Dec 2023 14:39:37 -0500 Subject: [PATCH 2/5] add default --- .../backend/standard/StandardAsyncExecutionActor.scala | 4 +++- .../batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala | 3 +-- .../common/PipelinesApiAsyncBackendJobExecutionActor.scala | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala index b2cd86c3405..eb660a0f420 100644 --- a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala +++ b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala @@ -61,7 +61,9 @@ case class DefaultStandardAsyncExecutionActorParams override val minimumRuntimeSettings: MinimumRuntimeSettings ) extends StandardAsyncExecutionActorParams -case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean) +// 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 diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala index 66be28a7c8f..fa05f62baee 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala @@ -671,8 +671,7 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar s"""|touch $DockerMonitoringLogPath |chmod u+x $DockerMonitoringScriptPath |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin, executeInSubshell = true).valid - else ScriptPreambleData("", executeInSubshell = true - ).valid + else ScriptPreambleData("").valid } private[actors] def generateInputs(jobDescriptor: BackendJobDescriptor): Set[GcpBatchInput] = { diff --git a/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala b/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala index 1fc13722efb..90399011cdb 100644 --- a/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala @@ -386,7 +386,7 @@ class PipelinesApiAsyncBackendJobExecutionActor(override val standardParams: Sta |chmod u+x $DockerMonitoringScriptPath |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin, executeInSubshell = true ).valid - else ScriptPreambleData("", executeInSubshell = true).valid + else ScriptPreambleData("").valid } override def globParentDirectory(womGlobFile: WomGlobFile): Path = { From 9925b30f0e68951ed506cd3b856fa817afa7c4af Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 1 Dec 2023 14:41:26 -0500 Subject: [PATCH 3/5] use default --- .../batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala index fa05f62baee..b3f9d47d316 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala @@ -670,7 +670,7 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar ScriptPreambleData( s"""|touch $DockerMonitoringLogPath |chmod u+x $DockerMonitoringScriptPath - |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin, executeInSubshell = true).valid + |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin).valid else ScriptPreambleData("").valid } From 71a348a6ded2c230e2cacc258ab05e605b2b4b23 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 1 Dec 2023 14:43:15 -0500 Subject: [PATCH 4/5] use default --- .../cromwell/backend/standard/StandardAsyncExecutionActor.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala index eb660a0f420..3b116ae1b99 100644 --- a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala +++ b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala @@ -332,7 +332,7 @@ trait StandardAsyncExecutionActor } /** Any custom code that should be run within commandScriptContents before the instantiated command. */ - def scriptPreamble: ErrorOr[ScriptPreambleData] = ScriptPreambleData("", executeInSubshell = true).valid + def scriptPreamble: ErrorOr[ScriptPreambleData] = ScriptPreambleData("").valid def cwd: Path = commandDirectory def rcPath: Path = cwd./(jobPaths.returnCodeFilename) From bdcb6e6a9b4e2a8ad0d8e355a932ca543ea64bd8 Mon Sep 17 00:00:00 2001 From: Tom Wiseman Date: Fri, 1 Dec 2023 14:45:01 -0500 Subject: [PATCH 5/5] missed one --- .../common/PipelinesApiAsyncBackendJobExecutionActor.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala b/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala index 90399011cdb..fee63573ff1 100644 --- a/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala +++ b/supportedBackends/google/pipelines/common/src/main/scala/cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala @@ -384,8 +384,7 @@ class PipelinesApiAsyncBackendJobExecutionActor(override val standardParams: Sta ScriptPreambleData( s"""|touch $DockerMonitoringLogPath |chmod u+x $DockerMonitoringScriptPath - |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin, executeInSubshell = true - ).valid + |$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin).valid else ScriptPreambleData("").valid }