Skip to content

Fix: Log links with showWhilePending: true are not shown while pendin…#7554

Open
fg91 wants to merge 1 commit into
masterfrom
fg91/fix/log-links-show-while-pending-ray
Open

Fix: Log links with showWhilePending: true are not shown while pendin…#7554
fg91 wants to merge 1 commit into
masterfrom
fg91/fix/log-links-show-while-pending-ray

Conversation

@fg91

@fg91 fg91 commented Jun 18, 2026

Copy link
Copy Markdown
Member

Why are the changes needed?

Log links can be shown before a task reaches the Running phase by setting showWhilePending: true. For the ray plugin, this doesn't actually work. This PR fixes this and adds a unit test that would have caught this bug.

How was this patch tested?

  • Added a unit test
  • Tested flytepropeller with this fix in our staging cluster

Check all the applicable boxes

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

…g for Ray plugin

Signed-off-by: Fabio Grätz <fabio@cusp.ai>
@fg91 fg91 added the fixed For any bug fixes label Jun 18, 2026
@github-actions github-actions Bot added the flyte label Jun 18, 2026
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.02%. Comparing base (de65211) to head (3999773).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7554      +/-   ##
==========================================
- Coverage   57.16%   57.02%   -0.14%     
==========================================
  Files         925      931       +6     
  Lines       57823    58385     +562     
==========================================
+ Hits        33052    33295     +243     
- Misses      21753    22030     +277     
- Partials     3018     3060      +42     
Flag Coverage Δ
unittests-datacatalog 53.51% <ø> (ø)
unittests-flyteadmin 53.12% <ø> (ø)
unittests-flytecopilot 43.06% <ø> (?)
unittests-flytectl 64.09% <ø> (ø)
unittests-flyteidl 75.71% <ø> (ø)
unittests-flyteplugins 60.38% <100.00%> (+0.01%) ⬆️
unittests-flytepropeller 53.78% <ø> (ø)
unittests-flytestdlib 62.76% <ø> (-0.03%) ⬇️

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

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

switch rayJob.Status.JobDeploymentStatus {
case rayv1.JobDeploymentStatusNew:
phaseInfo, err = pluginsCore.PhaseInfoQueuedWithTaskInfo(pluginsCore.DefaultPhaseVersion, "Scheduling", info), nil
case rayv1.JobDeploymentStatusInitializing:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Task info including log links is only sent to flyteadmin when either the task phase changes (e.g. queued -> initializing) or when the so-called phase-version within the same phase changes (due a different "reason").

An initial event with phase Queued and pluginsCore.DefaultPhaseVersion is sent to flyteadmin before GetTaskPhase is called/the log links are constructed.

Subsequent calls to GetTaskPhase never reach phaseVersionUpdateErr := k8s.MaybeUpdatePhaseVersionFromPluginContext(&phaseInfo, &pluginContext) where the phase version would be bumped when necessary due to the early exit that this PR removes.

Because of this, log links with showWhilePending: true are constructed by GetTaskPhase but are never sent to flyteadmin.

By removing the early exit and ensuring that MaybeUpdatePhaseVersionFromPluginContext is reached, this problem is avoided.

@fg91 fg91 requested review from Sovietaced and pingsutw June 18, 2026 11:27

// KubeRay creates a Ray cluster first, and then submits a Ray job to the cluster
switch rayJob.Status.JobDeploymentStatus {
case rayv1.JobDeploymentStatusNew:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

const rayv1.JobDeploymentStatusNew rayv1.JobDeploymentStatus = "" so this is equivalent to the prior len(rayJob.Status.JobDeploymentStatus) == 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fixed For any bug fixes flyte

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant