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

Jump to bottom

[WX-1168] TES Log Paths #7210

Merged
merged 3 commits into from
Aug 24, 2023
Merged

[WX-1168] TES Log Paths #7210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
731a633
add tes logs to call metadata
THWiseman Aug 22, 2023
86ad062
simple test
THWiseman Aug 22, 2023
f316e15
Merge branch 'develop' into WX-1168
THWiseman Aug 22, 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
1 change: 1 addition & 0 deletions ...ortedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesJobCachingActorHelper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,6 @@
lazy val tesConfiguration: TesConfiguration = initializationData.tesConfiguration

lazy val runtimeAttributes = TesRuntimeAttributes(validatedRuntimeAttributes, jobDescriptor.runtimeAttributes, tesConfiguration)
override protected def nonStandardMetadata: Map[String, Any] = super.nonStandardMetadata ++ tesJobPaths.azureLogPathsForMetadata

Check warning on line 22 in supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesJobCachingActorHelper.scala

View check run for this annotation

Codecov / codecov/patch

supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesJobCachingActorHelper.scala#L22 

Added line #L22 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love this naming and I'm glad you found the perfect fit to stick some extra stuff


}
14 changes: 11 additions & 3 deletions supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesJobPaths.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@
val callInputsRoot = callRoot.resolve("inputs")

/*
* tesTaskRoot: This is the root directory that TES will use for files related to this task.
* TES expects a path relative to the root of the storage container.
* We provide it to TES as a k/v pair where the key is "internal_path_prefix" and the value is the relative path string.
* tesTaskRoot: The Azure TES implementation allows us to specify a working directory that it should use for its own files.
* Once the task finishes, this directory will contain stderr.txt, stdout.txt, and some other misc files.
* Unlike other paths we provide, this one is expected as a path relative to the root of the storage container.
* We provide it as a k/v pair where the key is "internal_path_prefix" and the value is the relative path string.
* This is not a standard TES feature, but rather related to the Azure TES implementation that Terra uses.
* While passing it outside of terra won't do any harm, we could consider making this optional and/or configurable.
*/
Expand All @@ -42,6 +43,13 @@
case anyOtherPath: Path => anyOtherPath.pathAsString
}

// Like above: Nothing should rely on these files existing, since only the Azure TES implementation will actually create them.
// Used to send the Azure TES log paths to the frontend.
val azureLogPathsForMetadata : Map[String, Any] = Map(
"tes_stdout" -> taskFullPath./("stdout.txt").pathAsString,
"tes_stderr" -> taskFullPath./("stderr.txt").pathAsString

Check warning on line 50 in supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesJobPaths.scala

View check run for this annotation

Codecov / codecov/patch

supportedBackends/tes/src/main/scala/cromwell/backend/impl/tes/TesJobPaths.scala#L48-L50 

Added lines #L48 - L50 were not covered by tests
Comment on lines +49 to +50
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if the naming should reflect the purpose (frontend) but could use some context. Maybe let's sync with faces so I can get context quickly and contain the length of the semantics discussion 🙂

Semi-related, we have a bunch of metadata keys in one place here, it's unclear whether this backend-specific one actually belongs in the main list though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tangentially related comment: There's a method called queryLogs within MetadataDatabaseAccess.scala that currently looks for logs with specific keys (specifically "stdout" and "stderr" as two of them). The method basically queries for log-related metadata and processes that for the JSON response.

With two new keys we'd need to update these types of fetches (or any method that might use CallMetadataKeys to paint the metadata response) to account for these new potential keys. That said, its best to tackle these updates in another ticket.

)

// Given an output path, return a path localized to the storage file system
def storageOutput(path: String): String = {
callExecutionRoot.resolve(path).toString
Expand Down
5 changes: 5 additions & 0 deletions supportedBackends/tes/src/test/scala/cromwell/backend/impl/tes/TesJobPathsSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ class TesJobPathsSpec extends AnyFlatSpec with CromwellTimeoutSpec with Matchers
File(s"/cromwell-executions/wf_hello/$id/call-hello").pathAsString
jobPaths.callExecutionDockerRoot.toString shouldBe
File(s"/cromwell-executions/wf_hello/$id/call-hello/execution").pathAsString
jobPaths.azureLogPathsForMetadata shouldBe
Map(
"tes_stdout" -> File(s"local-cromwell-executions/wf_hello/$id/call-hello/tes_task/stdout.txt").pathAsString,
"tes_stderr" -> File(s"local-cromwell-executions/wf_hello/$id/call-hello/tes_task/stderr.txt").pathAsString
)

val jobKeySharded = BackendJobDescriptorKey(call, Option(0), 1)
val jobPathsSharded = TesJobPaths(jobKeySharded, wd, TesTestConfig.backendConfig)
Expand Down
Loading