Skip to content

Delegate RunLogsService.TailLogs to DataProxyService to remove duplicate implementation #7252#7269

Open
Carina-TzuHsuan wants to merge 1 commit into
flyteorg:v2from
Carina-TzuHsuan:#7252
Open

Delegate RunLogsService.TailLogs to DataProxyService to remove duplicate implementation #7252#7269
Carina-TzuHsuan wants to merge 1 commit into
flyteorg:v2from
Carina-TzuHsuan:#7252

Conversation

@Carina-TzuHsuan

@Carina-TzuHsuan Carina-TzuHsuan commented Apr 23, 2026

Copy link
Copy Markdown

Remove the duplicate K8sLogStreamer from the runs service and replace it
with a thin forwarding shim that calls DataProxyService.TailLogs. The
/RunLogsService/TailLogs endpoint is kept registered for backward
compatibility — clients see no change.

Tracking issue

Closes #7252

Why are the changes needed?

RunLogsService and DataProxyService both maintained their own
K8sLogStreamer with nearly identical logic for streaming pod logs.
This duplicated maintenance burden — any fix to log streaming had to be
applied in two places.

What changes were proposed in this pull request?

  • runs/service/run_logs_service.go: Replaced the repo + K8sLogStreamer implementation with a DataProxyServiceClient field.
    TailLogs now validates the request, acquires a semaphore slot, then
    forwards to DataProxyService.TailLogs and relays responses back to
    the caller.
  • runs/service/k8s_log_streamer.go: Deleted (duplicate removed).
  • runs/config/config.go: Added DataProxyServiceURL config field
    (default: http://localhost:8088).
  • runs/setup.go: Replaced K8s streamer wiring with a
    DataProxyServiceClient injection. RunLogsService is now always
    mounted unconditionally (K8s access is DataProxy's concern).

Net result: −531 lines.

How was this patch tested?

Rewrote run_logs_service_test.go with five test cases using a real
httptest.Server backed by a fakeDataProxyHandler:

  • TestTailLogs_HappyPath — response lines forwarded correctly from DataProxy to caller
  • TestTailLogs_MissingActionID — returns CodeInvalidArgument
  • TestTailLogs_DataProxyError — DataProxy error propagated to caller
  • TestTailLogs_ConcurrencyLimit — semaphore rejects at 100 concurrent streams with CodeResourceExhausted
  • TestTailLogs_RequestForwardedToDataProxyaction_id and attempt forwarded unchanged

Labels

  • removed: duplicate K8sLogStreamer deleted from runs service
  • changed: RunLogsService now delegates to DataProxyService

Check all the applicable boxes

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

@pingsutw pingsutw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thank you, lgtm, could you resolve the merge conflict

…7252)

Remove the duplicate K8sLogStreamer from the runs service and replace it
with a thin forwarding shim that calls DataProxyService.TailLogs. The
/RunLogsService/TailLogs endpoint is kept registered for backward
compatibility — clients see no change.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Carina-TzuHsuan

Copy link
Copy Markdown
Author

@pingsutw Please review it. Thank you!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants