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.

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-927 Task log streaming for GCP Batch #7540

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from
Open

Conversation

mcovarr
Copy link
Contributor

@mcovarr mcovarr commented Sep 13, 2024

Description

Update: DO NOT MERGE, per 2024-09-27 sync meeting Google and Burwood are going to revisit a streaming file-based solution that includes localization, task logs, and delocalization.

Task log streaming for GCP Batch. Note this is not a perfect substitute for PAPI v2-style task logs as it does not include output from container setup, localization or delocalization.

Release Notes Confirmation

CHANGELOG.md

  • I updated CHANGELOG.md in this PR
  • [] I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

  • I added a suggested release notes entry in this Jira ticket
  • I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

.setDestination(Destination.PATH)
.setLogsPath(data.gcpBatchParameters.logfile.toString)
.build
???
Copy link
Contributor Author

@mcovarr mcovarr Sep 13, 2024

Choose a reason for hiding this comment

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

IMHO TODO: turn Cloud Logging into a boolean option that defaults to true, while file-based task logging is always on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also rather than being config-driven, we probably want to be able to drive Cloud Logging from workflow options so Terra users can click a checkbox to get this behavior iff they want it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made WX-1852 for workflow-option cloud logging

@@ -16,6 +16,7 @@ object CallMetadataKeys {
val Failures = "failures"
val Stdout = "stdout"
val Stderr = "stderr"
val TaskLog = "task.log"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I was never a huge fan of dynamic task log names, but others may be...

@@ -498,6 +501,7 @@ trait StandardAsyncExecutionActor
|touch $stdoutRedirection $stderrRedirection
|tee $stdoutRedirection < "$$$out" &
|tee $stderrRedirection < "$$$err" >&2 &
|tail -q -f $stdoutRedirection $stderrRedirection > $taskLogRedirection &
Copy link
Contributor Author

@mcovarr mcovarr Sep 13, 2024

Choose a reason for hiding this comment

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

NB this change was really meant only for the GCP Batch backend but here has been made in code that is common with the PAPI v2 backend

@mcovarr mcovarr changed the title WX-927 Sketch of task log streaming WX-927 Task log streaming Sep 26, 2024
@mcovarr mcovarr changed the title WX-927 Task log streaming WX-927 Task log streaming for GCP Batch Sep 26, 2024
@mcovarr mcovarr marked this pull request as ready for review September 27, 2024 15:56
@mcovarr mcovarr requested a review from a team as a code owner September 27, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant