Feat: Allow controlling in which task phases log links are shown

[fg91]

Why are the changes needed?

Currently, all task log links appear in the Flyte UI once the respective pod(s) are running and they are never removed.

In this PR I add config options to log links to allow showing certain log links during the queueing/initializing phase or to hide them once the task terminates.

This enables for instance the following user stories:

• As a platform engineer I want to show platform users log links already during the queuing phase of a task that help the user understand why a task pod might not be scheduleable. (Especially for task plugins relying on CRDs, the status updates shown in the Flyte UI don’t include information such as “FailedTriggerScaleUp”).
• As a platform engineer I want to hide certain log links once tasks complete as the log links are not valid anymore. Examples are Flyte-interactive VSCode links or links to the “live pod view" in the GKE "workloads" section.

What changes were proposed in this pull request?

I propose to add two new options to task logs flyteidl/protos/flyteidl/core/execution.proto:

message TaskLog {
    …
    bool ShowWhilePending = 5;
    bool HideOnceFinished = 6;
}

This example …

  logs:
    ...
    templates:
    - displayName: ShowPending
      showWhilePending: true
      templateUris:
    - displayName: ShowPendingHideFinished
      hideOnceFinished: true
      showWhilePending: true
      templateUris:

… results in the following UX:

How was this patch tested?

I ran propeller and admin with the proposed changes. I ensured that the new behaviour works for all K8s plugins (pod, kubeflow, spark, dask, ray) as well as map tasks. For each plugin, screenshots are included in comments further below.

Check all the applicable boxes

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

Related PRs

I added further tests in #5200 which ensure all plugins send new events to admin in the queuing phase if the “reason” changes. Currently this is implemented properly only for the pod plugin and had to be fixed for this PR.

Docs link

[codecov]

Codecov Report

Attention: Patch coverage is 82.35294% with 24 lines in your changes missing coverage. Please review.

Project coverage is 61.00%. Comparing base (de415af) to head (3bfba55).
Report is 128 commits behind head on master.

Files with missing lines	Patch %	Lines
flyteplugins/go/tasks/plugins/k8s/spark/spark.go	80.95%	4 Missing and 4 partials ⚠️
flyteplugins/go/tasks/plugins/k8s/dask/dask.go	83.33%	2 Missing and 2 partials ⚠️
flyteplugins/go/tasks/plugins/k8s/ray/ray.go	72.72%	2 Missing and 1 partial ⚠️
...lugins/go/tasks/plugins/k8s/kfoperators/mpi/mpi.go	60.00%	1 Missing and 1 partial ⚠️
...o/tasks/plugins/k8s/kfoperators/pytorch/pytorch.go	60.00%	1 Missing and 1 partial ⚠️
...s/plugins/k8s/kfoperators/tensorflow/tensorflow.go	60.00%	1 Missing and 1 partial ⚠️
flyteplugins/go/tasks/logs/logging_utils.go	75.00%	0 Missing and 1 partial ⚠️
...lyteplugins/go/tasks/pluginmachinery/core/phase.go	0.00%	1 Missing ⚠️
...ns/go/tasks/pluginmachinery/flytek8s/pod_helper.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4726   +/-   ##
=======================================
  Coverage   61.00%   61.00%           
=======================================
  Files         793      793           
  Lines       51378    51419   +41     
=======================================
+ Hits        31342    31370   +28     
- Misses      17149    17157    +8     
- Partials     2887     2892    +5     

Flag	Coverage Δ
unittests-datacatalog	69.31% <ø> (ø)
unittests-flyteadmin	58.71% <100.00%> (+<0.01%)	⬆️
unittests-flytecopilot	17.79% <ø> (ø)
unittests-flytectl	68.10% <ø> (+0.05%)	⬆️
unittests-flyteidl	79.04% <ø> (ø)
unittests-flyteplugins	61.83% <79.66%> (-0.01%)	⬇️
unittests-flytepropeller	57.30% <ø> (ø)
unittests-flytestdlib	65.82% <ø> (ø)

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

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

[fg91]

This logic moved without change to k8s.MaybeUpdatePhaseVersion( so that it can be reused in other plugins.

[fg91]

Now, we do want to generate task logs while we are pending.

[Tom-Newton]

Its not a big problem but I think this change might be the reason that we get quite a lot of spam in the flytepropeller logs like

{"json":{"exec_id":"fybmhjrhjorydc","node":"n0/dn1","ns":"<namespace>","res_ver":"3487679492","routine":"worker-154","tasktype":"python-task","wf":"<workflow>"},"level":"error","msg":"containerStatus IndexOutOfBound, requested [0], but total containerStatuses [0] in pod phase [Pending]","ts":"2024-10-28T13:56:14Z"}

This comes from this line, so I wonder if we should adjust the logging a bit in response to this change.

[fg91]

Thanks for bringing it to my attention that this change causes excessive logs!

Before this PR far, GetLogsForContainerInPod wasn't called if pod.Status.Phase == v1.PodPending. If in a later pod phase there still weren't any container statuses, this was unexpected, hence error log level.

Since we now optionally generate task logs already in the pending phase, if I'm not misunderstanding something, it's not unexpected that there might not be any container statuses at this point 🤔

I propose to here set the log level to debug if the pod is in pending phase and keep it at error level otherwise.

Does this make sense to you @Tom-Newton ?

[Tom-Newton]

That makes sense to me 👍

[fg91]

I'll make a PR 👍

[fg91]

#5945

[fg91]

For log links with default configuration, nothing changes:

Master:

This branch:

[fg91]

At the example of tensorflow:

Master:

Branch:

[fg91]

So far, none of the k8s plugins apart from pod have checked whether the reason changed within the same phase. This means that per task phase only a single event was sent.

For the "queued" phase, this is the default "task submitted to K8s" event emitted here.

We need to send subsequent events to admin once the reason changes as the first event doesn't contain the log links.

As a nice side effect, the status updates at the top right of flyte console now show a lot more updates within a single phase.

At the example of the tensorflow plugin/queueing phase:

Master:

4/7/2024 5:19:38 PM UTC task submitted to K8s

Branch:

4/7/2024 5:30:12 PM UTC task submitted to K8s

4/7/2024 5:30:13 PM UTC JobCreated

4/7/2024 5:30:12 PM UTC Restart policy in pod template will be overwritten by restart policy in replica spec

4/7/2024 5:30:13 PM UTC Created pod: f76767053e7b04e95aef-n0-0-chief-0

4/7/2024 5:30:13 PM UTC Created service: f76767053e7b04e95aef-n0-0-chief-0

4/7/2024 5:30:13 PM UTC Created pod: f76767053e7b04e95aef-n0-0-ps-0

4/7/2024 5:30:13 PM UTC Created service: f76767053e7b04e95aef-n0-0-ps-0

[fg91]

The behaviour for task logs with default config doesn't change:

Master:

Branch:

(The screenshots start with the "initializing" phase as opposed to "queueing" as the dask plugin sets the phases to "initializing" as soon as the pods exist, even if they still pending.)

[fg91]

Master:

Branch:

[fg91]

The diff view is unfortunate here. The only change until line 409 is that if !isQueued { is removed because we need to send all logs earlier.
The diff view suggests that "Driver logs" config was removed, the "User logs" config then turned into the "Drivers log" config, and so on.

[fg91]

This is required for map tasks which treat log links differently for performance/scalability reasons.

Master:

This branch:

Note that the task execution view with the log links only becomes available in the running phase for map tasks, meaning that there is no use case for showing log links already in the pending phase. However, log links can be removed once finished.

[eapolinario]

Note: array node map tasks replaced map tasks in flytekit 1.12.0 and this distinction is no longer true, i.e. array node map tasks log links no longer use the separate map tasks log links.

[fg91]

For all the plugins in which I add k8s.MaybeUpdatePhaseVersionFromPluginContext( - kubeflow, dask, ray, spark - I add tests that the phase version is in fact bumped if the reason changes in this separate PR in order to not overload this one further:
#5200

[fg91]

This is new:

[fg91]

If one takes a look at how the log links are currently configured for the spark plugin, one sees that "All User Logs" are sent already when queued as the log plugin's init is not guarded by if !isQueued {.

This has the effect that the "All User" logs are shown already in the init phase (current master):

(Fails because I don't have a proper spark image at hand.)

The log link is not shown in the queued phase currently even though the log link is added to info in the queued phase because the plugin doesn't currently bump the phase version when the reason changes meaning that only a single task submitted to k8s event is sent to admin in the queued phase. When bumping the phase version, the log link is shown already in the queued phase:

Given if !isQueued { I feel this might have been the original intention. In practice this makes little difference as in the init phase, the driver pod is still pending, meaning that the transition from queued to initializing phase is almost instantaneous even if the actual scheduling of the pods takes longer.

With the removal of if !isQueued { and instead setting "All User" logs to ShowWhilePending, this is how the log links behave:

[fg91]

@fg91 , this is awesome, thank you for this feature!

In terms of getting this merged, can you merge #5200 and also master? I'll approve after that.

@eapolinario, I merged #5200 and rebased onto master 🙏

[eapolinario]

Amazing. Thank you for all the work on this!