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

Passthrough unique node ID in task execution ID for generating log te… #4380

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

jeevb
Copy link
Contributor

@jeevb jeevb commented Nov 8, 2023

…mplate vars

Tracking issue

Describe your changes

Passes through a node's unique ID in pluginsCore.TaskExecutionID as the {{.NodeID}} template variable for log template links.

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Screenshot 2023-11-07 at 20 08 27 Screenshot 2023-11-07 at 20 11 33

Note to reviewers

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (bec7bbb) 59.11% compared to head (2ebbab1) 59.36%.
Report is 1 commits behind head on master.

❗ Current head 2ebbab1 differs from pull request most recent head c599167. Consider uploading reports for the commit c599167 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4380      +/-   ##
==========================================
+ Coverage   59.11%   59.36%   +0.24%     
==========================================
  Files         614      544      -70     
  Lines       52073    38989   -13084     
==========================================
- Hits        30785    23147    -7638     
+ Misses      18839    13562    -5277     
+ Partials     2449     2280     -169     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
flyteplugins/go/tasks/logs/logging_utils.go 89.70% <100.00%> (+1.61%) ⬆️
...ugins/go/tasks/pluginmachinery/tasklog/template.go 98.62% <100.00%> (+0.50%) ⬆️
flyteplugins/go/tasks/plugins/k8s/dask/dask.go 80.93% <100.00%> (+2.62%) ⬆️
flyteplugins/go/tasks/plugins/k8s/pod/plugin.go 80.41% <100.00%> (+2.81%) ⬆️
flyteplugins/go/tasks/plugins/k8s/spark/spark.go 76.87% <100.00%> (+2.92%) ⬆️
...ller/pkg/controller/nodes/task/taskexec_context.go 80.89% <100.00%> (+1.88%) ⬆️
.../plugins/k8s/kfoperators/common/common_operator.go 64.55% <88.00%> (+4.76%) ⬆️
flyteplugins/go/tasks/plugins/k8s/ray/ray.go 83.68% <0.00%> (+0.07%) ⬆️

... and 544 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jeevb jeevb force-pushed the jeev/child-task-node-id branch 4 times, most recently from 0680e1f to b7dedc9 Compare November 8, 2023 04:31
@jeevb jeevb requested review from EngHabu and hamersaw November 8, 2023 04:36
@jeevb jeevb marked this pull request as ready for review November 8, 2023 04:36
@jeevb jeevb force-pushed the jeev/child-task-node-id branch from b7dedc9 to ac764a9 Compare November 8, 2023 05:46
Signed-off-by: Jeev B <[email protected]>
EngHabu
EngHabu previously approved these changes Nov 8, 2023
@@ -53,7 +54,7 @@ func GetLogsForContainerInPod(ctx context.Context, logPlugin tasklog.Plugin, tas
PodRFC3339FinishTime: time.Unix(finishTime, 0).Format(time.RFC3339),
PodUnixStartTime: startTime,
PodUnixFinishTime: finishTime,
TaskExecutionIdentifier: taskExecID,
TaskExecutionID: taskExecID,
Copy link
Contributor

Choose a reason for hiding this comment

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

Backwards compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn’t think of issues with backward compatibility since this will only affect new log links. Maybe I’m missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, the Input object becomes your interface, right? if somebody is referencing {.TaskIdentifier.Project} that will no longer resolve if I'm reading correctly, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, the external-facing template variables remain unchanged.

Signed-off-by: Jeev B <[email protected]>
@jeevb jeevb merged commit bda9aca into master Nov 8, 2023
38 of 39 checks passed
@jeevb jeevb deleted the jeev/child-task-node-id branch November 8, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants