loki.source.docker: update to use shared scheduler #5026

kalleep · 2025-12-08T13:16:03Z

PR Description

loki.source.docker suffered from the same issue that loki.source.file did where scheduling could take some time for new targets.

In this pr I move Scheduler to source package so I can be reused by both components. With this I also moved "target" into docker package and rename to tailer, this tailer now implements Source interface.

I also fixed a issue where stopping component could deadlock if nothing was reading from handler chan.

Which issue(s) this PR fixes

Related to: #4729

Notes to the Reviewer

Moved target and metrics from internal package to tailer.go and metrics.go and they are no longer exported.
Created a shared structure Fanout, this implements the common pattern to abort send operation if context is canceled.
Create shared function Consume to run the consume loop that will abort if context is canceled.
Create shared function Drain that can be used when component is stopped.

PR Checklist

CHANGELOG.md updated
Documentation added
Tests updated
Config converters updated

Copilot

Pull request overview

This PR refactors loki.source.docker to use a shared Scheduler pattern (previously only used by loki.source.file) to improve container scheduling performance and fix potential deadlocks during component shutdown. The scheduler and related utilities have been moved to the shared source package for reuse across components.

Key changes:

Moved Scheduler from file package to shared source package, making it reusable by both loki.source.file and loki.source.docker
Created new shared utilities (Fanout, Consume, Drain) that implement common patterns for entry forwarding and graceful shutdown
Replaced the docker component's custom manager/runner implementation with the Scheduler pattern

Reviewed changes

Copilot reviewed 17 out of 19 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
internal/component/loki/source/scheduler.go	Package renamed from `file` to `source`; removed `IsRunning()` method; added `DebugSource` interface
internal/component/loki/source/scheduler_test.go	Package renamed from `file` to `source`
internal/component/loki/source/drain.go	New shared utility for draining log entries during shutdown to prevent deadlocks
internal/component/loki/source/consume.go	New shared utility for consuming and forwarding log entries with context cancellation
internal/component/common/loki/fanout.go	New shared utility for distributing log entries to multiple receivers with thread-safe updates
internal/component/loki/source/file/file.go	Updated to use shared scheduler, fanout, consume, and drain utilities; removed custom receiver management
internal/component/loki/source/file/tailer.go	Removed `IsRunning()` method; added `DebugInfo()` method to implement `DebugSource` interface
internal/component/loki/source/file/tailer_test.go	Updated test to access `running` field directly instead of through removed `IsRunning()` method
internal/component/loki/source/file/decompresser.go	Removed `IsRunning()` method; added `DebugInfo()` method to implement `DebugSource` interface
internal/component/loki/source/docker/docker.go	Major refactoring to use scheduler pattern; replaced `manager` with `Scheduler`; uses shared `Fanout` for receiver management
internal/component/loki/source/docker/tailer.go	Moved from internal/dockertarget package; implements `Source` and `DebugSource` interfaces; added `Run()` method for scheduler integration
internal/component/loki/source/docker/tailer_test.go	Updated tests for new tailer structure; added restart and stress tests
internal/component/loki/source/docker/metrics.go	Changed from exported `Metrics` to unexported `metrics`; package comment updated
internal/component/loki/source/docker/runner.go	Deleted (replaced by scheduler)
internal/component/loki/source/docker/internal/dockertarget/target.go	Deleted (moved to tailer.go)
internal/component/loki/source/docker/docker_test.go	Updated component tests to work with new scheduler-based implementation
internal/component/loki/source/docker/testdata/flog_after_restart.log	New test data file for container restart tests
CHANGELOG.md	Added entries for scheduling improvement and deadlock fix

Comments suppressed due to low confidence (1)

internal/component/loki/source/scheduler.go:104

Inconsistent type parameter naming: The generic type parameter is lowercase k in DebugSource[k comparable], but it's uppercase K in all other generic types in this file (e.g., Source[K comparable], Scheduler[K comparable], SourceWithRetry[K comparable]). For consistency, it should be DebugSource[K comparable].

internal/component/loki/source/docker/docker.go

internal/component/loki/source/file/file.go

internal/component/loki/source/docker/docker.go

internal/component/loki/source/docker/tailer.go

internal/component/common/loki/fanout.go

internal/component/loki/source/drain.go

internal/component/loki/source/consume.go

internal/component/loki/source/docker/tailer.go

internal/component/loki/source/docker/docker.go

Copilot

Pull request overview

Copilot reviewed 19 out of 21 changed files in this pull request and generated 2 comments.

internal/component/loki/source/docker/tailer.go

internal/component/loki/source/drain.go

internal/component/common/loki/fanout.go

CHANGELOG.md

dehaansa

A lot to process here! A first pass at review, but will definitely take another.

internal/component/common/loki/fanout.go

internal/component/loki/source/docker/docker.go

internal/component/loki/source/drain_test.go

thampiotr

It looks correct, but I do feel like it's a bit of a risky change. I know you've done manual testing, so I'm approving this, but another pair of eyes will be a good idea.

CHANGELOG.md

thampiotr · 2025-12-11T14:48:34Z

internal/component/loki/source/scheduler.go

 func (s *Scheduler[K]) Stop() {
 	s.cancel()
 	s.running.Wait()
 	s.sources = make(map[K]scheduledSource[K])
 }

+// Reset will stop all running sources and wait for them to finish and reset
+// Scheduler to a usable state.
+func (s *Scheduler[K]) Reset() {
+	s.cancel()
+	s.running.Wait()
+	s.sources = make(map[K]scheduledSource[K])
+	s.ctx, s.cancel = context.WithCancel(context.Background())
+}


These two are quite similar... Can we reduce to have just one?

Maybe I can think of a better way to do it. but we don't want to create a new context when we stop a component, that would leak resources..

thampiotr · 2025-12-11T14:55:57Z

internal/component/loki/source/file/decompresser.go


-func (d *decompressor) IsRunning() bool {
-	return d.running.Load()
+func (d *decompressor) DebugInfo() any {


Another gotcha with DebugInfo functions in the past was that the lock order was not consistent and we had a deadlock. Happened at least 2x. I looked and I don't see this issue here, but the fact that we need to depend on manual review worries me here. Maybe we can refactor in the future to avoid the multiple locks sequencing issues.

yes that is a issue for sure

kalleep · 2025-12-11T15:10:30Z

It looks correct, but I do feel like it's a bit of a risky change. I know you've done manual testing, so I'm approving this, but another pair of eyes will be a good idea.

Sure I can wait for @dehaansa do to a review too.

Co-authored-by: Copilot <[email protected]>

kalleep requested a review from a team as a code owner December 8, 2025 13:16

kalleep force-pushed the kalleep/loki-source-docker-scheduling branch from 18c614f to bf16415 Compare December 8, 2025 13:17

kalleep requested a review from Copilot December 9, 2025 12:22

Copilot started reviewing on behalf of kalleep December 9, 2025 12:22 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

kalleep requested a review from Copilot December 9, 2025 13:21

Copilot started reviewing on behalf of kalleep December 9, 2025 13:22 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

internal/component/loki/source/docker/tailer.go Outdated Show resolved Hide resolved

internal/component/loki/source/docker/tailer.go Outdated Show resolved Hide resolved

thampiotr reviewed Dec 9, 2025

View reviewed changes

internal/component/loki/source/drain.go Outdated Show resolved Hide resolved

thampiotr reviewed Dec 9, 2025

View reviewed changes

internal/component/common/loki/fanout.go Show resolved Hide resolved

thampiotr reviewed Dec 9, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kalleep force-pushed the kalleep/loki-source-docker-scheduling branch from a8b6fa7 to 627a9cc Compare December 9, 2025 16:15

dehaansa reviewed Dec 9, 2025

View reviewed changes

internal/component/common/loki/fanout.go Show resolved Hide resolved

internal/component/loki/source/docker/docker.go Outdated Show resolved Hide resolved

internal/component/loki/source/drain_test.go Outdated Show resolved Hide resolved

kalleep force-pushed the kalleep/loki-source-docker-scheduling branch 3 times, most recently from 257e395 to 9eae721 Compare December 11, 2025 14:17

thampiotr approved these changes Dec 11, 2025

View reviewed changes

kalleep and others added 12 commits December 11, 2025 16:11

move scheduler to source package

f605483

Add ext interface and move info creation into sources

1af1bef

loki.source.docker: refactor to use shared scheduler

7aa65aa

Extract common function to drain handler

318fa81

Add changelog

d52fa02

update comment

7423afe

only stop position once

8f09194

Add shared Fanout and Consume loop

ff5222e

simplify unlock

af96e02

Add note

d4f1108

fix

ad8c9dc

Update internal/component/loki/source/docker/docker.go

1bea1b8

Co-authored-by: Copilot <[email protected]>

kalleep and others added 20 commits December 11, 2025 16:11

Update internal/component/loki/source/docker/tailer.go

33be1e5

Co-authored-by: Copilot <[email protected]>

Fix DebugSource

71033e5

fix logic around docker container scheduling

2e7c636

Clenup position when target is removed

15ca904

lint

a1f6f2f

Add tests

c761949

remove duplicated test

8964bc5

Add reset

87d6782

Update internal/component/loki/source/docker/tailer.go

e56183e

Co-authored-by: Copilot <[email protected]>

Check if channel was closed

6edd4fb

Update changelog

f8a21fd

remove comment

94228da

fix test

fee3e8f

rename receiver

433f556

remove unused function

49aa06e

fix debug info

82abc09

lint

97458a0

Add a note about impotance of holding read lock

fb201ab

fix changelog

5532880

Update note

797a5ae

kalleep force-pushed the kalleep/loki-source-docker-scheduling branch from 2f46b74 to 797a5ae Compare December 11, 2025 15:12

fix changelog

bf59f3f

loki.source.docker: update to use shared scheduler #5026

Are you sure you want to change the base?

loki.source.docker: update to use shared scheduler #5026

Uh oh!

Conversation

kalleep commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dehaansa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thampiotr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thampiotr Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

kalleep Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

thampiotr Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

kalleep Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

kalleep commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kalleep commented Dec 8, 2025 •

edited

Loading