From e1f8d228256d315a45d697aa4610b05b2714d566 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Thu, 12 Sep 2024 20:51:35 -0400 Subject: [PATCH 01/11] sketch --- CHANGELOG.md | 4 ++-- .../scala/cromwell/backend/io/JobPaths.scala | 7 +++++-- .../StandardAsyncExecutionActor.scala | 8 +++++-- core/src/main/scala/cromwell/core/core.scala | 2 +- .../services/metadata/CallMetadataKeys.scala | 1 + ...cpBatchAsyncBackendJobExecutionActor.scala | 21 ++++--------------- .../GcpBatchJobCachingActorHelper.scala | 3 --- .../batch/api/GcpBatchRequestFactory.scala | 4 +--- .../api/GcpBatchRequestFactoryImpl.scala | 5 +---- .../models/CreateGcpBatchParameters.scala | 4 +--- 10 files changed, 22 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a43caa93d4..108c8194fd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,8 +33,8 @@ be found [here](https://cromwell.readthedocs.io/en/stable/backends/HPC/#optional - Fixes the reference disk feature. - Fixes pulling Docker image metadata from private GCR repositories. - Fixed `google_project` and `google_compute_service_account` workflow options not taking effect when using GCP Batch backend -- Added a way to use a custom LogsPolicy for the job execution, setting `backend.providers.batch.config.batch.logs-policy` to "CLOUD_LOGGING" (default) keeps the current behavior, or, set it to "PATH" to save the logs into the the mounted disk, at the end, this log file gets copied to the google cloud storage bucket with "task.log" as the name. -- When "CLOUD_LOGGING" is used, many more Cromwell / WDL labels for workflow, root workflow, call, shard etc. are now assigned to GCP Batch log entries. +- A task log file with the name "task.log" that combines standard output and standard error is now streamed to the task directory in Google Cloud Storage. +- Many more Cromwell / WDL labels for workflow, root workflow, call, shard etc. are now assigned to GCP Batch log entries in Cloud Logging. ### Improved handling of Life Sciences API quota errors diff --git a/backend/src/main/scala/cromwell/backend/io/JobPaths.scala b/backend/src/main/scala/cromwell/backend/io/JobPaths.scala index 05ad6a56dc1..d7c36f409b7 100644 --- a/backend/src/main/scala/cromwell/backend/io/JobPaths.scala +++ b/backend/src/main/scala/cromwell/backend/io/JobPaths.scala @@ -43,6 +43,7 @@ trait JobPaths { def memoryRetryRCFilename: String = "memory_retry_rc" def defaultStdoutFilename = "stdout" def defaultStderrFilename = "stderr" + def defaultTaskLogFilename = "task.log" def isDocker: Boolean = false // In this non-Docker version of `JobPaths` there is no distinction between host and container roots so this is @@ -73,7 +74,8 @@ trait JobPaths { // enable dynamic standard output and error file names for languages like CWL that support this feature. var standardPaths: StandardPaths = StandardPaths( output = callExecutionRoot.resolve(defaultStdoutFilename), - error = callExecutionRoot.resolve(defaultStderrFilename) + error = callExecutionRoot.resolve(defaultStderrFilename), + taskLog = callExecutionRoot.resolve(defaultTaskLogFilename) ) lazy val script = callExecutionRoot.resolve(scriptFilename) @@ -85,7 +87,8 @@ trait JobPaths { // standard output and error file names. def standardOutputAndErrorPaths: Map[String, Path] = Map( CallMetadataKeys.Stdout -> standardPaths.output, - CallMetadataKeys.Stderr -> standardPaths.error + CallMetadataKeys.Stderr -> standardPaths.error, + CallMetadataKeys.TaskLog -> standardPaths.taskLog ) private lazy val commonMetadataPaths: Map[String, Path] = diff --git a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala index 8f8312023f1..251c60d9e19 100644 --- a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala +++ b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala @@ -380,6 +380,7 @@ trait StandardAsyncExecutionActor instantiatedCommand.evaluatedStdoutOverride.getOrElse(jobPaths.defaultStdoutFilename) |> absolutizeContainerPath def executionStderr: String = instantiatedCommand.evaluatedStderrOverride.getOrElse(jobPaths.defaultStderrFilename) |> absolutizeContainerPath + def executionTaskLog: String = jobPaths.defaultTaskLogFilename |> absolutizeContainerPath /* * Ensures the standard paths are correct w.r.t overridden paths. This is called in two places: when generating the command and @@ -393,9 +394,10 @@ trait StandardAsyncExecutionActor // .get's are safe on stdout and stderr after falling back to default names above. jobPaths.standardPaths = StandardPaths( output = hostPathFromContainerPath(executionStdout), - error = hostPathFromContainerPath(executionStderr) + error = hostPathFromContainerPath(executionStderr), + taskLog = hostPathFromContainerPath(executionTaskLog) ) - // Re-publish stdout and stderr paths that were possibly just updated. + // Re-publish stdout, stderr and task log paths that were possibly just updated. tellMetadata(jobPaths.standardOutputAndErrorPaths) jobPathsUpdated = true } @@ -423,6 +425,7 @@ trait StandardAsyncExecutionActor val stdinRedirection = executionStdin.map("< " + _.shellQuote).getOrElse("") val stdoutRedirection = executionStdout.shellQuote val stderrRedirection = executionStderr.shellQuote + val taskLogRedirection = executionTaskLog.shellQuote val rcTmpPath = rcPath.plusExt("tmp") val errorOrDirectoryOutputs: ErrorOr[List[WomUnlistedDirectory]] = @@ -491,6 +494,7 @@ trait StandardAsyncExecutionActor |touch $stdoutRedirection $stderrRedirection |tee $stdoutRedirection < "$$$out" & |tee $stderrRedirection < "$$$err" >&2 & + |tail -q -f $stdoutRedirection $stderrRedirection > $taskLogRedirection & |( |cd ${cwd.pathAsString} |ENVIRONMENT_VARIABLES diff --git a/core/src/main/scala/cromwell/core/core.scala b/core/src/main/scala/cromwell/core/core.scala index 60ead3fcf7a..c2da5d32e7e 100644 --- a/core/src/main/scala/cromwell/core/core.scala +++ b/core/src/main/scala/cromwell/core/core.scala @@ -8,7 +8,7 @@ import mouse.boolean._ import scala.concurrent.duration.FiniteDuration import scala.util.control.NoStackTrace -case class StandardPaths(output: Path, error: Path) +case class StandardPaths(output: Path, error: Path, taskLog: Path) case class CallContext(root: Path, standardPaths: StandardPaths, isDocker: Boolean) diff --git a/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala b/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala index cb7f31730f0..3c4ea21abce 100644 --- a/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala +++ b/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala @@ -16,6 +16,7 @@ object CallMetadataKeys { val Failures = "failures" val Stdout = "stdout" val Stderr = "stderr" + val TaskLog = "task.log" val BackendLogsPrefix = "backendLogs" val BackendStatus = "backendStatus" val JobId = "jobId" 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 def59339847..1ef8e16f9c6 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 @@ -838,16 +838,6 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar contentType = plainTextContentType ) - val logFileOutput = GcpBatchFileOutput( - logFilename, - logGcsPath, - DefaultPathBuilder.get(logFilename), - workingDisk, - optional = true, - secondary = false, - contentType = plainTextContentType - ) - val memoryRetryRCFileOutput = GcpBatchFileOutput( memoryRetryRCFilename, memoryRetryRCGcsPath, @@ -864,7 +854,8 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar val standardStreams = List( StandardStream("stdout", _.output), - StandardStream("stderr", _.error) + StandardStream("stderr", _.error), + StandardStream("taskLog", _.taskLog) ) map { s => GcpBatchFileOutput( s.name, @@ -888,8 +879,7 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar DetritusOutputParameters( monitoringScriptOutputParameter = monitoringOutput, rcFileOutputParameter = rcFileOutput, - memoryRetryRCFileOutputParameter = memoryRetryRCFileOutput, - logFileOutputParameter = logFileOutput + memoryRetryRCFileOutputParameter = memoryRetryRCFileOutput ), List.empty ) @@ -908,10 +898,7 @@ class GcpBatchAsyncBackendJobExecutionActor(override val standardParams: Standar runtimeAttributes = runtimeAttributes, batchAttributes = batchAttributes, projectId = batchAttributes.project, - region = batchAttributes.location, - logfile = createParameters.commandScriptContainerPath.sibling( - batchParameters.detritusOutputParameters.logFileOutputParameter.name - ) + region = batchAttributes.location ) drsLocalizationManifestCloudPath = jobPaths.callExecutionRoot / GcpBatchJobPaths.DrsLocalizationManifestName diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchJobCachingActorHelper.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchJobCachingActorHelper.scala index edb778d3928..773222fb07f 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchJobCachingActorHelper.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchJobCachingActorHelper.scala @@ -34,9 +34,6 @@ trait GcpBatchJobCachingActorHelper extends StandardCachingActorHelper { lazy val memoryRetryRCFilename: String = gcpBatchCallPaths.memoryRetryRCFilename lazy val memoryRetryRCGcsPath: Path = gcpBatchCallPaths.memoryRetryRC - lazy val logFilename: String = "task.log" - lazy val logGcsPath: Path = gcpBatchCallPaths.callExecutionRoot.resolve(logFilename) - lazy val batchAttributes: GcpBatchConfigurationAttributes = batchConfiguration.batchAttributes lazy val defaultLabels: Labels = { diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactory.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactory.scala index d2f8242b213..5aa633dab4b 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactory.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactory.scala @@ -41,11 +41,9 @@ object GcpBatchRequestFactory { case class DetritusOutputParameters( monitoringScriptOutputParameter: Option[GcpBatchFileOutput], rcFileOutputParameter: GcpBatchFileOutput, - memoryRetryRCFileOutputParameter: GcpBatchFileOutput, - logFileOutputParameter: GcpBatchFileOutput + memoryRetryRCFileOutputParameter: GcpBatchFileOutput ) { def all: List[GcpBatchFileOutput] = memoryRetryRCFileOutputParameter :: - logFileOutputParameter :: rcFileOutputParameter :: monitoringScriptOutputParameter.toList } diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala index 8071acbf9e8..875f7319764 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala @@ -242,10 +242,7 @@ class GcpBatchRequestFactoryImpl()(implicit gcsTransferConfiguration: GcsTransfe case GcpBatchLogsPolicy.CloudLogging => LogsPolicy.newBuilder.setDestination(Destination.CLOUD_LOGGING).build case GcpBatchLogsPolicy.Path => - LogsPolicy.newBuilder - .setDestination(Destination.PATH) - .setLogsPath(data.gcpBatchParameters.logfile.toString) - .build + ??? } val googleLabels = data.createParameters.googleLabels.map(l => Label(l.key, l.value)) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/CreateGcpBatchParameters.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/CreateGcpBatchParameters.scala index 0c47b95fb7d..456f7115ae8 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/CreateGcpBatchParameters.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/CreateGcpBatchParameters.scala @@ -1,12 +1,10 @@ package cromwell.backend.google.batch.models import cromwell.backend.BackendJobDescriptor -import cromwell.core.path.Path case class CreateGcpBatchParameters(jobDescriptor: BackendJobDescriptor, runtimeAttributes: GcpBatchRuntimeAttributes, batchAttributes: GcpBatchConfigurationAttributes, projectId: String, - region: String, - logfile: Path + region: String ) From 92e3efdc1556dc988a81b2410540d18f0bf1f082 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Fri, 13 Sep 2024 06:36:17 -0400 Subject: [PATCH 02/11] cleanup --- .../src/main/scala/cromwell/backend/io/JobPaths.scala | 9 +++++---- .../backend/standard/StandardAsyncExecutionActor.scala | 2 +- .../cromwell/services/metadata/CallMetadataKeys.scala | 2 +- .../backend/google/batch/models/GcpBatchJobPaths.scala | 2 ++ .../GcpBatchAsyncBackendJobExecutionActorSpec.scala | 3 ++- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/io/JobPaths.scala b/backend/src/main/scala/cromwell/backend/io/JobPaths.scala index d7c36f409b7..ca2801d203a 100644 --- a/backend/src/main/scala/cromwell/backend/io/JobPaths.scala +++ b/backend/src/main/scala/cromwell/backend/io/JobPaths.scala @@ -14,6 +14,7 @@ object JobPaths { val ScriptPathKey = "script" val StdoutPathKey = "stdout" val StdErrPathKey = "stderr" + val TaskLogPathKey = "taskLog" val ReturnCodePathKey = "returnCode" val CallRootPathKey = "callRootPath" val DockerCidPathKey = "dockerCidPath" @@ -45,6 +46,7 @@ trait JobPaths { def defaultStderrFilename = "stderr" def defaultTaskLogFilename = "task.log" def isDocker: Boolean = false + def implementsTaskLogging: Boolean = false // In this non-Docker version of `JobPaths` there is no distinction between host and container roots so this is // just called 'rootWithSlash'. @@ -87,9 +89,8 @@ trait JobPaths { // standard output and error file names. def standardOutputAndErrorPaths: Map[String, Path] = Map( CallMetadataKeys.Stdout -> standardPaths.output, - CallMetadataKeys.Stderr -> standardPaths.error, - CallMetadataKeys.TaskLog -> standardPaths.taskLog - ) + CallMetadataKeys.Stderr -> standardPaths.error + ) ++ (if (implementsTaskLogging) Map(CallMetadataKeys.TaskLog -> standardPaths.taskLog) else Map.empty) private lazy val commonMetadataPaths: Map[String, Path] = standardOutputAndErrorPaths + (CallMetadataKeys.CallRoot -> callRoot) @@ -102,7 +103,7 @@ trait JobPaths { JobPaths.StdoutPathKey -> standardPaths.output, JobPaths.StdErrPathKey -> standardPaths.error, JobPaths.ReturnCodePathKey -> returnCode - ) + ) ++ (if (implementsTaskLogging) Map(JobPaths.TaskLogPathKey -> standardPaths.taskLog) else Map.empty) private lazy val commonLogPaths: Map[String, Path] = Map( JobPaths.StdoutPathKey -> standardPaths.output, diff --git a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala index 251c60d9e19..eafa34b95c1 100644 --- a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala +++ b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala @@ -494,7 +494,7 @@ trait StandardAsyncExecutionActor |touch $stdoutRedirection $stderrRedirection |tee $stdoutRedirection < "$$$out" & |tee $stderrRedirection < "$$$err" >&2 & - |tail -q -f $stdoutRedirection $stderrRedirection > $taskLogRedirection & + |${if (jobPaths.implementsTaskLogging) s"tail -q -f $stdoutRedirection $stderrRedirection > $taskLogRedirection &" else ""} |( |cd ${cwd.pathAsString} |ENVIRONMENT_VARIABLES diff --git a/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala b/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala index 3c4ea21abce..bbab5db826d 100644 --- a/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala +++ b/services/src/main/scala/cromwell/services/metadata/CallMetadataKeys.scala @@ -16,7 +16,7 @@ object CallMetadataKeys { val Failures = "failures" val Stdout = "stdout" val Stderr = "stderr" - val TaskLog = "task.log" + val TaskLog = "taskLog" val BackendLogsPrefix = "backendLogs" val BackendStatus = "backendStatus" val JobId = "jobId" diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala index 6da61d9bcf6..9e39e277540 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala @@ -28,6 +28,8 @@ case class GcpBatchJobPaths(override val workflowPaths: GcpBatchWorkflowPaths, s"${jobKey.node.localName}$index" } + override def implementsTaskLogging: Boolean = true + val batchLogFilename: String = s"$batchLogBasename.log" lazy val batchLogPath: Path = callExecutionRoot.resolve(batchLogFilename) diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala index a162165f765..f9ba8d636e7 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala @@ -1202,7 +1202,8 @@ class GcpBatchAsyncBackendJobExecutionActorSpec "runtimeAttributes:zones" -> "us-central1-b,us-central1-a", "runtimeAttributes:maxRetries" -> "0", "stderr" -> s"$batchGcsRoot/wf_hello/$workflowId/call-goodbye/stderr", - "stdout" -> s"$batchGcsRoot/wf_hello/$workflowId/call-goodbye/stdout" + "stdout" -> s"$batchGcsRoot/wf_hello/$workflowId/call-goodbye/stdout", + "taskLog" -> s"$batchGcsRoot/wf_hello/$workflowId/call-goodbye/task.log" ) ) } From 579b8d3d1c7db6e04f8d5b58535db1a36528a9b8 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Fri, 13 Sep 2024 09:05:41 -0400 Subject: [PATCH 03/11] scalafmt --- .../backend/standard/StandardAsyncExecutionActor.scala | 7 ++++++- 1 file changed, 6 insertions(+), 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 eafa34b95c1..4a9b75135e1 100644 --- a/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala +++ b/backend/src/main/scala/cromwell/backend/standard/StandardAsyncExecutionActor.scala @@ -474,6 +474,10 @@ trait StandardAsyncExecutionActor } } + val taskLoggingCommand = + if (jobPaths.implementsTaskLogging) s"tail -q -f $stdoutRedirection $stderrRedirection > $taskLogRedirection &" + else "" + // 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. // https://stackoverflow.com/questions/692000/how-do-i-write-stderr-to-a-file-while-using-tee-with-a-pipe @@ -494,7 +498,7 @@ trait StandardAsyncExecutionActor |touch $stdoutRedirection $stderrRedirection |tee $stdoutRedirection < "$$$out" & |tee $stderrRedirection < "$$$err" >&2 & - |${if (jobPaths.implementsTaskLogging) s"tail -q -f $stdoutRedirection $stderrRedirection > $taskLogRedirection &" else ""} + |TASK_LOGGING_COMMAND |( |cd ${cwd.pathAsString} |ENVIRONMENT_VARIABLES @@ -515,6 +519,7 @@ trait StandardAsyncExecutionActor .replace("INSTANTIATED_COMMAND", commandString) .replace("SCRIPT_EPILOGUE", scriptEpilogue) .replace("DOCKER_OUTPUT_DIR_LINK", dockerOutputDir) + .replace("TASK_LOGGING_COMMAND", taskLoggingCommand) ) } From e2f677a71997d6c2b816e651940c65ed0d80e1f8 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Fri, 13 Sep 2024 09:14:53 -0400 Subject: [PATCH 04/11] simplify --- .../backend/google/batch/models/GcpBatchJobPaths.scala | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala index 9e39e277540..732a09a3213 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchJobPaths.scala @@ -21,16 +21,9 @@ case class GcpBatchJobPaths(override val workflowPaths: GcpBatchWorkflowPaths, override val isCallCacheCopyAttempt: Boolean = false ) extends JobPaths { - def batchLogBasename = { - val index = jobKey.index - .map(s => s"-$s") - .getOrElse("") - s"${jobKey.node.localName}$index" - } - override def implementsTaskLogging: Boolean = true - val batchLogFilename: String = s"$batchLogBasename.log" + val batchLogFilename: String = "task.log" lazy val batchLogPath: Path = callExecutionRoot.resolve(batchLogFilename) val batchMonitoringLogFilename: String = s"${GcpBatchJobPaths.BatchMonitoringKey}.log" From 1c606e773e093bd5d450b259f763d57b1743aaea Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Fri, 13 Sep 2024 09:28:00 -0400 Subject: [PATCH 05/11] words --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 108c8194fd1..9d7babe76ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,7 @@ be found [here](https://cromwell.readthedocs.io/en/stable/backends/HPC/#optional - Fixes pulling Docker image metadata from private GCR repositories. - Fixed `google_project` and `google_compute_service_account` workflow options not taking effect when using GCP Batch backend - A task log file with the name "task.log" that combines standard output and standard error is now streamed to the task directory in Google Cloud Storage. -- Many more Cromwell / WDL labels for workflow, root workflow, call, shard etc. are now assigned to GCP Batch log entries in Cloud Logging. +- When Cloud Logging is enabled, many more Cromwell / WDL labels for workflow, root workflow, call, shard etc. are now assigned to GCP Batch log entries. ### Improved handling of Life Sciences API quota errors From bb1f6fc9a46088eb0a06e2ca2a14097620c60cc0 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Fri, 13 Sep 2024 11:22:21 -0400 Subject: [PATCH 06/11] fix unit tests --- .../backend/google/batch/models/GcpBatchJobPathsSpec.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchJobPathsSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchJobPathsSpec.scala index 933d2958689..45e8241b66a 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchJobPathsSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchJobPathsSpec.scala @@ -41,7 +41,7 @@ class GcpBatchJobPathsSpec extends TestKitSuite with AnyFlatSpecLike with Matche callPaths.returnCodeFilename should be("rc") callPaths.stderr.getFileName.pathAsString should be("gs://my-cromwell-workflows-bucket/stderr") callPaths.stdout.getFileName.pathAsString should be("gs://my-cromwell-workflows-bucket/stdout") - callPaths.batchLogFilename should be("hello.log") + callPaths.batchLogFilename should be("task.log") } it should "map the correct paths" in { @@ -69,7 +69,7 @@ class GcpBatchJobPathsSpec extends TestKitSuite with AnyFlatSpecLike with Matche callPaths.stderr.pathAsString should be(s"gs://my-cromwell-workflows-bucket/wf_hello/${workflowDescriptor.id}/call-hello/stderr") callPaths.batchLogPath.pathAsString should - be(s"gs://my-cromwell-workflows-bucket/wf_hello/${workflowDescriptor.id}/call-hello/hello.log") + be(s"gs://my-cromwell-workflows-bucket/wf_hello/${workflowDescriptor.id}/call-hello/task.log") } it should "map the correct call context" in { From 2ea1c350a23ce6c01f2e567263264f0aecdecfd7 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Fri, 13 Sep 2024 12:23:35 -0400 Subject: [PATCH 07/11] oops --- .../actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala index f9ba8d636e7..74edb009ae4 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/actors/GcpBatchAsyncBackendJobExecutionActorSpec.scala @@ -1084,7 +1084,7 @@ class GcpBatchAsyncBackendJobExecutionActorSpec "gs://path/to/gcs_root/wf_hello/e6236763-c518-41d0-9688-432549a8bf7c/call-hello/stderr" batchBackend.gcpBatchCallPaths.batchLogPath should be(a[GcsPath]) batchBackend.gcpBatchCallPaths.batchLogPath.pathAsString shouldBe - "gs://path/to/gcs_root/wf_hello/e6236763-c518-41d0-9688-432549a8bf7c/call-hello/hello.log" + "gs://path/to/gcs_root/wf_hello/e6236763-c518-41d0-9688-432549a8bf7c/call-hello/task.log" } it should "return Batch log paths for scattered call" in { @@ -1132,7 +1132,7 @@ class GcpBatchAsyncBackendJobExecutionActorSpec "gs://path/to/gcs_root/w/e6236763-c518-41d0-9688-432549a8bf7d/call-B/shard-2/stderr" batchBackend.gcpBatchCallPaths.batchLogPath should be(a[GcsPath]) batchBackend.gcpBatchCallPaths.batchLogPath.pathAsString shouldBe - "gs://path/to/gcs_root/w/e6236763-c518-41d0-9688-432549a8bf7d/call-B/shard-2/B-2.log" + "gs://path/to/gcs_root/w/e6236763-c518-41d0-9688-432549a8bf7d/call-B/shard-2/task.log" } it should "return the project from the workflow options in the start metadata" in { From 75e5d3545b0fd0ff37c02f5a7cde066c2b4c72f4 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Mon, 16 Sep 2024 12:11:05 -0400 Subject: [PATCH 08/11] cleanup --- .../google/batch/api/GcpBatchRequestFactoryImpl.scala | 2 -- .../batch/models/GcpBatchConfigurationAttributes.scala | 1 - .../backend/google/batch/models/GcpBatchLogsPolicy.scala | 1 - .../batch/models/GcpBatchConfigurationAttributesSpec.scala | 6 ------ 4 files changed, 10 deletions(-) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala index 875f7319764..41c18e192e8 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/api/GcpBatchRequestFactoryImpl.scala @@ -241,8 +241,6 @@ class GcpBatchRequestFactoryImpl()(implicit gcsTransferConfiguration: GcsTransfe val logsPolicy = data.gcpBatchParameters.batchAttributes.logsPolicy match { case GcpBatchLogsPolicy.CloudLogging => LogsPolicy.newBuilder.setDestination(Destination.CLOUD_LOGGING).build - case GcpBatchLogsPolicy.Path => - ??? } val googleLabels = data.createParameters.googleLabels.map(l => Label(l.key, l.value)) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributes.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributes.scala index cf71a1a3426..e594819b10f 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributes.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributes.scala @@ -216,7 +216,6 @@ object GcpBatchConfigurationAttributes extends GcpBatchReferenceFilesMappingOper val logsPolicy: ErrorOr[GcpBatchLogsPolicy] = validate { backendConfig.as[Option[String]]("batch.logs-policy").getOrElse("CLOUD_LOGGING") match { case "CLOUD_LOGGING" => GcpBatchLogsPolicy.CloudLogging - case "PATH" => GcpBatchLogsPolicy.Path case other => throw new IllegalArgumentException( s"Unrecognized logs policy entry: $other. Supported strategies are CLOUD_LOGGING and PATH." diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchLogsPolicy.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchLogsPolicy.scala index bcc8fab8d13..d43714e4956 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchLogsPolicy.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/models/GcpBatchLogsPolicy.scala @@ -4,5 +4,4 @@ sealed trait GcpBatchLogsPolicy extends Product with Serializable object GcpBatchLogsPolicy { case object CloudLogging extends GcpBatchLogsPolicy - case object Path extends GcpBatchLogsPolicy } diff --git a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributesSpec.scala b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributesSpec.scala index 19805debbed..39dc0a69b13 100644 --- a/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributesSpec.scala +++ b/supportedBackends/google/batch/src/test/scala/cromwell/backend/google/batch/models/GcpBatchConfigurationAttributesSpec.scala @@ -135,12 +135,6 @@ class GcpBatchConfigurationAttributesSpec gcpBatchAttributes.logsPolicy should be(GcpBatchLogsPolicy.CloudLogging) } - it should "parse logs-policy = PATH" in { - val backendConfig = ConfigFactory.parseString(configString(batch = "logs-policy = PATH")) - val gcpBatchAttributes = GcpBatchConfigurationAttributes(googleConfig, backendConfig, "batch") - gcpBatchAttributes.logsPolicy should be(GcpBatchLogsPolicy.Path) - } - it should "reject invalid logs-policy" in { val expected = "Google Cloud Batch configuration is not valid: Errors:\nUnrecognized logs policy entry: INVALID. Supported strategies are CLOUD_LOGGING and PATH." From 26de61f147b2d8d0768ce3fdcefa7a6f89fd4b31 Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Thu, 26 Sep 2024 13:35:04 -0400 Subject: [PATCH 09/11] fix test --- .../main/scala/cromwell/backend/io/JobPaths.scala | 1 - .../actors/GcpBatchInitializationActor.scala | 15 ++++----------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/backend/src/main/scala/cromwell/backend/io/JobPaths.scala b/backend/src/main/scala/cromwell/backend/io/JobPaths.scala index ca2801d203a..817aa1dc76f 100644 --- a/backend/src/main/scala/cromwell/backend/io/JobPaths.scala +++ b/backend/src/main/scala/cromwell/backend/io/JobPaths.scala @@ -17,7 +17,6 @@ object JobPaths { val TaskLogPathKey = "taskLog" val ReturnCodePathKey = "returnCode" val CallRootPathKey = "callRootPath" - val DockerCidPathKey = "dockerCidPath" def callPathBuilder(root: Path, jobKey: JobKey, isCallCacheCopyAttempt: Boolean) = { val callName = jobKey.node.localName diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala index d78100dd340..fc23fc2b064 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala @@ -16,18 +16,11 @@ import com.google.auth.oauth2.OAuth2Credentials import cromwell.backend.google.batch._ import cromwell.backend.google.batch.actors.GcpBatchInitializationActor._ import cromwell.backend.google.batch.api.GcpBatchRequestFactoryImpl -import cromwell.backend.google.batch.models.GcpBatchConfigurationAttributes.{ - VirtualPrivateCloudConfiguration, - VirtualPrivateCloudLabels, - VirtualPrivateCloudLiterals -} +import cromwell.backend.google.batch.models.GcpBatchConfigurationAttributes.{VirtualPrivateCloudConfiguration, VirtualPrivateCloudLabels, VirtualPrivateCloudLiterals} import cromwell.backend.google.batch.models._ import cromwell.backend.google.batch.runnable.WorkflowOptionKeys -import cromwell.backend.standard.{ - StandardInitializationActor, - StandardInitializationActorParams, - StandardValidatedRuntimeAttributesBuilder -} +import cromwell.backend.io.JobPaths +import cromwell.backend.standard.{StandardInitializationActor, StandardInitializationActorParams, StandardValidatedRuntimeAttributesBuilder} import cromwell.backend.{BackendConfigurationDescriptor, BackendInitializationData, BackendWorkflowDescriptor} import cromwell.cloudsupport.gcp.auth.GoogleAuthMode.{httpTransport, jsonFactory} import cromwell.cloudsupport.gcp.auth.{GoogleAuthMode, UserServiceAccountMode} @@ -274,7 +267,7 @@ object GcpBatchInitializationActor { // For metadata publishing purposes default to using the name of a standard stream as the stream's filename. def defaultStandardStreamNameToFileNameMetadataMapper(gcpBatchJobPaths: GcpBatchJobPaths, streamName: String - ): String = streamName + ): String = if (streamName == JobPaths.TaskLogPathKey) gcpBatchJobPaths.batchLogFilename else streamName def encryptKms(keyName: String, credentials: OAuth2Credentials, plainText: String): String = { val httpCredentialsAdapter = new HttpCredentialsAdapter(credentials) From 451cc7e2bda1458859f948490016fd9375c05d5f Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Thu, 26 Sep 2024 13:35:40 -0400 Subject: [PATCH 10/11] scalafmt --- .../batch/actors/GcpBatchInitializationActor.scala | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala index fc23fc2b064..95c0a74022c 100644 --- a/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala +++ b/supportedBackends/google/batch/src/main/scala/cromwell/backend/google/batch/actors/GcpBatchInitializationActor.scala @@ -16,11 +16,19 @@ import com.google.auth.oauth2.OAuth2Credentials import cromwell.backend.google.batch._ import cromwell.backend.google.batch.actors.GcpBatchInitializationActor._ import cromwell.backend.google.batch.api.GcpBatchRequestFactoryImpl -import cromwell.backend.google.batch.models.GcpBatchConfigurationAttributes.{VirtualPrivateCloudConfiguration, VirtualPrivateCloudLabels, VirtualPrivateCloudLiterals} +import cromwell.backend.google.batch.models.GcpBatchConfigurationAttributes.{ + VirtualPrivateCloudConfiguration, + VirtualPrivateCloudLabels, + VirtualPrivateCloudLiterals +} import cromwell.backend.google.batch.models._ import cromwell.backend.google.batch.runnable.WorkflowOptionKeys import cromwell.backend.io.JobPaths -import cromwell.backend.standard.{StandardInitializationActor, StandardInitializationActorParams, StandardValidatedRuntimeAttributesBuilder} +import cromwell.backend.standard.{ + StandardInitializationActor, + StandardInitializationActorParams, + StandardValidatedRuntimeAttributesBuilder +} import cromwell.backend.{BackendConfigurationDescriptor, BackendInitializationData, BackendWorkflowDescriptor} import cromwell.cloudsupport.gcp.auth.GoogleAuthMode.{httpTransport, jsonFactory} import cromwell.cloudsupport.gcp.auth.{GoogleAuthMode, UserServiceAccountMode} From faf0d63d91f617de3a84b7921a09161ab53f8fdc Mon Sep 17 00:00:00 2001 From: Miguel Covarrubias Date: Thu, 26 Sep 2024 14:56:43 -0400 Subject: [PATCH 11/11] adjust expected counts for presence of task logs --- .../resources/standardTestCases/gcpbatch_exhaustive_delete.test | 2 +- .../standardTestCases/gcpbatch_sub_workflow_delete.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/centaur/src/main/resources/standardTestCases/gcpbatch_exhaustive_delete.test b/centaur/src/main/resources/standardTestCases/gcpbatch_exhaustive_delete.test index 0f00ec0a5ce..29f3cb54d43 100644 --- a/centaur/src/main/resources/standardTestCases/gcpbatch_exhaustive_delete.test +++ b/centaur/src/main/resources/standardTestCases/gcpbatch_exhaustive_delete.test @@ -18,7 +18,7 @@ metadata { fileSystemCheck: "gcs" outputExpectations: { "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/exhaustive_delete/<>/call-exhaustive/delete.txt": 0 - "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/exhaustive_delete/<>/call-exhaustive/": 8 + "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/exhaustive_delete/<>/call-exhaustive/": 9 "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/exhaustive_delete/<>/call-exhaustive/gcs_delocalization.sh": 1 "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/exhaustive_delete/<>/call-exhaustive/gcs_localization.sh": 1 "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/exhaustive_delete/<>/call-exhaustive/gcs_transfer.sh": 1 diff --git a/centaur/src/main/resources/standardTestCases/gcpbatch_sub_workflow_delete.test b/centaur/src/main/resources/standardTestCases/gcpbatch_sub_workflow_delete.test index cae387a6148..b68f4095055 100644 --- a/centaur/src/main/resources/standardTestCases/gcpbatch_sub_workflow_delete.test +++ b/centaur/src/main/resources/standardTestCases/gcpbatch_sub_workflow_delete.test @@ -22,7 +22,7 @@ metadata { fileSystemCheck: "gcs" outputExpectations: { # No current way to match on the subworkflow id, so for now just make sure the total directory count matches. - "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/sub_workflow_delete/<>/call-sub_call/sub_workflow_delete_import/": 8 + "gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/sub_workflow_delete/<>/call-sub_call/sub_workflow_delete_import/": 9 #"gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/sub_workflow_delete/<>/call-sub_call/sub_workflow_delete_import/<>/call-sub_workflow_task/gcs_delocalization.sh": 1 #"gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/sub_workflow_delete/<>/call-sub_call/sub_workflow_delete_import/<>/call-sub_workflow_task/gcs_localization.sh": 1 #"gs://cloud-cromwell-dev-self-cleaning/cromwell_execution/ci/sub_workflow_delete/<>/call-sub_call/sub_workflow_delete_import/<>/call-sub_workflow_task/gcs_transfer.sh": 1