Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WX-1391] Fix Bash Bug #7326

Merged
merged 5 commits into from
Dec 1, 2023
Merged

[WX-1391] Fix Bash Bug #7326

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bf0d85c
fix
THWiseman Dec 1, 2023
c0578dc
add default
THWiseman Dec 1, 2023
9925b30
use default
THWiseman Dec 1, 2023
71a348a
use default
THWiseman Dec 1, 2023
bdcb6e6
missed one
THWiseman Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ case class DefaultStandardAsyncExecutionActorParams
override val minimumRuntimeSettings: MinimumRuntimeSettings
) extends StandardAsyncExecutionActorParams

case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Case classes can have defaults
  • Introducing a default gives a good opportunity to explain why it is what it is
Suggested change
case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean)
// Typically we want subshell for encapsulation
// Override to `false` when we want the script to set an environment variable in the parent shell
case class ScriptPreambleData(bashString: String, executeInSubshell: Boolean = true)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set a default executeInSubshell = true here? That avoids us needing to set it in the PAPI backend and communicates to future developers that it's the safe "normal" choice.


/**
* An extension of the generic AsyncBackendJobExecutionActor providing a standard abstract implementation of an
* asynchronous polling backend.
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
14 changes: 9 additions & 5 deletions ...in/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@
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}
Expand All @@ -49,6 +50,7 @@
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
Expand Down Expand Up @@ -663,12 +665,14 @@
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(

Check warning on line 670 in supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala

View check run for this annotation

Codecov / codecov/patch

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

Added line #L670 was not covered by tests
s"""|touch $DockerMonitoringLogPath
|chmod u+x $DockerMonitoringScriptPath
|$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin.valid
} else "".valid
|$DockerMonitoringScriptPath > $DockerMonitoringLogPath &""".stripMargin, executeInSubshell = true).valid

Check warning on line 673 in supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActor.scala

View check run for this annotation

Codecov / codecov/patch

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

Added line #L673 was not covered by tests
else ScriptPreambleData("", executeInSubshell = true
).valid
}

private[actors] def generateInputs(jobDescriptor: BackendJobDescriptor): Set[GcpBatchInput] = {
Expand Down
13 changes: 7 additions & 6 deletions .../cromwell/backend/google/pipelines/common/PipelinesApiAsyncBackendJobExecutionActor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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}
Expand All @@ -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}
Expand Down Expand Up @@ -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 = {
Expand Down
10 changes: 5 additions & 5 deletions ...kends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
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._
Expand Down Expand Up @@ -123,7 +123,7 @@
|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
}
Expand Down Expand Up @@ -213,7 +213,7 @@
*
* @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
Expand All @@ -222,9 +222,9 @@
}
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))

Check warning on line 225 in supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala

View check run for this annotation

Codecov / codecov/patch

supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActor.scala#L225 

Added line #L225 was not covered by tests
}.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.
}
}

Expand Down
3 changes: 2 additions & 1 deletion ...s/tes/src/test/scala/cromwell/backend/impl/tes/TesAsyncBackendJobExecutionActorSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Loading