From 91872b4639cf6567187e76a757492e4db09ec5ae Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 08:36:29 +0100 Subject: [PATCH 1/7] Remove unused clientConfigs argument from NewTargetsManagers --- clients/pkg/promtail/targets/manager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/clients/pkg/promtail/targets/manager.go b/clients/pkg/promtail/targets/manager.go index 9944174b87c1a..fc53ac9669171 100644 --- a/clients/pkg/promtail/targets/manager.go +++ b/clients/pkg/promtail/targets/manager.go @@ -59,7 +59,6 @@ func NewTargetManagers( client api.EntryHandler, scrapeConfigs []scrapeconfig.Config, targetConfig *file.Config, - clientConfigs ...client.Config, ) (*TargetManagers, error) { var targetManagers []targetManager targetScrapeConfigs := make(map[string][]scrapeconfig.Config, 4) From 7e0c9002447960065d8ec4968b3c8b6afb07ead4 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 08:38:22 +0100 Subject: [PATCH 2/7] Do not create targetScrapeConfigs if stdin target is used --- clients/pkg/promtail/targets/manager.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clients/pkg/promtail/targets/manager.go b/clients/pkg/promtail/targets/manager.go index fc53ac9669171..65ced62cceb89 100644 --- a/clients/pkg/promtail/targets/manager.go +++ b/clients/pkg/promtail/targets/manager.go @@ -60,19 +60,18 @@ func NewTargetManagers( scrapeConfigs []scrapeconfig.Config, targetConfig *file.Config, ) (*TargetManagers, error) { - var targetManagers []targetManager - targetScrapeConfigs := make(map[string][]scrapeconfig.Config, 4) - if targetConfig.Stdin { level.Debug(logger).Log("msg", "configured to read from stdin") stdin, err := stdin.NewStdinTargetManager(reg, logger, app, client, scrapeConfigs) if err != nil { return nil, err } - targetManagers = append(targetManagers, stdin) - return &TargetManagers{targetManagers: targetManagers}, nil + return &TargetManagers{targetManagers: []targetManager{stdin}}, nil } + var targetManagers []targetManager + targetScrapeConfigs := make(map[string][]scrapeconfig.Config, 4) + for _, cfg := range scrapeConfigs { switch { case cfg.HasServiceDiscoveryConfig(): From 441e19515fb541f41215596b67a51ed94f9f0448 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 08:40:43 +0100 Subject: [PATCH 3/7] fixup! Remove unused clientConfigs argument from NewTargetsManagers --- clients/pkg/promtail/targets/manager.go | 1 - 1 file changed, 1 deletion(-) diff --git a/clients/pkg/promtail/targets/manager.go b/clients/pkg/promtail/targets/manager.go index 65ced62cceb89..97842eee5c676 100644 --- a/clients/pkg/promtail/targets/manager.go +++ b/clients/pkg/promtail/targets/manager.go @@ -9,7 +9,6 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/grafana/loki/clients/pkg/promtail/api" - "github.com/grafana/loki/clients/pkg/promtail/client" "github.com/grafana/loki/clients/pkg/promtail/positions" "github.com/grafana/loki/clients/pkg/promtail/scrapeconfig" "github.com/grafana/loki/clients/pkg/promtail/targets/cloudflare" From a152246e0b143d5ad6e7ac3fc0bc9ee0396dff24 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 09:52:38 +0100 Subject: [PATCH 4/7] Close file watcher when target is removed Signed-off-by: Christian Haudum --- clients/pkg/promtail/targets/file/filetargetmanager.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/clients/pkg/promtail/targets/file/filetargetmanager.go b/clients/pkg/promtail/targets/file/filetargetmanager.go index b6a6d1cb97a78..587687e91de00 100644 --- a/clients/pkg/promtail/targets/file/filetargetmanager.go +++ b/clients/pkg/promtail/targets/file/filetargetmanager.go @@ -319,6 +319,10 @@ func (s *targetSyncer) sync(groups []*targetgroup.Group, targetEventHandler chan target.Stop() s.metrics.targetsActive.Add(-1.) delete(s.targets, key) + + // close related file event watcher + close(s.fileEventWatchers[target.path]) + delete(s.fileEventWatchers, target.path) } } s.droppedTargets = dropped @@ -376,6 +380,7 @@ func (s *targetSyncer) ready() bool { } return false } + func (s *targetSyncer) stop() { s.mtx.Lock() defer s.mtx.Unlock() @@ -385,6 +390,7 @@ func (s *targetSyncer) stop() { target.Stop() delete(s.targets, key) } + for key, watcher := range s.fileEventWatchers { close(watcher) delete(s.fileEventWatchers, key) From d942e73eb411644c596b7f71ed0e638922325cb8 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 15:42:45 +0100 Subject: [PATCH 5/7] fixup! Close file watcher when target is removed Signed-off-by: Christian Haudum --- .../targets/file/filetargetmanager_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/clients/pkg/promtail/targets/file/filetargetmanager_test.go b/clients/pkg/promtail/targets/file/filetargetmanager_test.go index bb3ee974a93f0..18db511ef306e 100644 --- a/clients/pkg/promtail/targets/file/filetargetmanager_test.go +++ b/clients/pkg/promtail/targets/file/filetargetmanager_test.go @@ -6,11 +6,13 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/go-kit/log" "github.com/prometheus/common/model" "github.com/prometheus/prometheus/discovery" "github.com/prometheus/prometheus/discovery/targetgroup" + "gopkg.in/fsnotify.v1" "github.com/grafana/loki/clients/pkg/promtail/api" "github.com/grafana/loki/clients/pkg/promtail/client/fake" @@ -447,3 +449,54 @@ func TestGlobWithMultipleFiles(t *testing.T) { t.Error("Handler did not receive the correct number of messages, expected 20 received", len(client.Received())) } } + +func TestDeadlockTargetManager(t *testing.T) { + client := fake.New(func() {}) + defer client.Stop() + + targetEventHandler := make(chan fileTargetEvent) + defer func() { + close(targetEventHandler) + }() + + syncer := &targetSyncer{ + metrics: NewMetrics(nil), + log: log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)), + positions: nil, + entryHandler: client, + hostname: "localhost", + fileEventWatchers: make(map[string]chan fsnotify.Event), + targets: make(map[string]*FileTarget), + targetConfig: &Config{ + SyncPeriod: time.Hour, + }, + } + + syncer.sync([]*targetgroup.Group{ + { + Targets: []model.LabelSet{ + { + hostLabel: "localhost", + pathLabel: "baz", + "job": "bar", + }, + }, + }, + }, targetEventHandler) + + require.Equal(t, len(syncer.targets), 1) + require.Equal(t, len(syncer.fileEventWatchers), 1) + + syncer.sync([]*targetgroup.Group{ + { + Targets: []model.LabelSet{ + {}, + }, + }, + }, targetEventHandler) + + syncer.sendFileCreateEvent(fsnotify.Event{Name: "baz"}) + + require.Equal(t, len(syncer.targets), 0) + require.Equal(t, len(syncer.fileEventWatchers), 0) +} From 87dedea0d82ca213fda402b7f150433c899133e0 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 15:52:30 +0100 Subject: [PATCH 6/7] fixup: add changelog entry Signed-off-by: Christian Haudum --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d69a360074cc..022897ad842c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Main +* [5170](https://github.com/grafana/loki/pull/5170) **chaudum** Fix deadlock in Promtail caused when targets got removed from a target group by the discovery manager. * [5163](https://github.com/grafana/loki/pull/5163) **chaudum** Fix regression in fluentd plugin introduced with #5107 that caused `NoMethodError` when parsing non-string values of log lines. * [5144](https://github.com/grafana/loki/pull/5144) **dannykopping** Ruler: fix remote write basic auth credentials. * [5107](https://github.com/grafana/loki/pull/5107) **chaudum** Fix bug in fluentd plugin that caused log lines containing non UTF-8 characters to be dropped. From 72822a564eac16359fd974f57c7d96277238d279 Mon Sep 17 00:00:00 2001 From: Christian Haudum Date: Tue, 18 Jan 2022 16:10:22 +0100 Subject: [PATCH 7/7] fixup! fixup! Close file watcher when target is removed Signed-off-by: Christian Haudum --- clients/pkg/promtail/targets/file/filetargetmanager.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/clients/pkg/promtail/targets/file/filetargetmanager.go b/clients/pkg/promtail/targets/file/filetargetmanager.go index 587687e91de00..fe6c596803c96 100644 --- a/clients/pkg/promtail/targets/file/filetargetmanager.go +++ b/clients/pkg/promtail/targets/file/filetargetmanager.go @@ -321,8 +321,13 @@ func (s *targetSyncer) sync(groups []*targetgroup.Group, targetEventHandler chan delete(s.targets, key) // close related file event watcher - close(s.fileEventWatchers[target.path]) - delete(s.fileEventWatchers, target.path) + k := target.path + if _, ok := s.fileEventWatchers[k]; ok { + close(s.fileEventWatchers[k]) + delete(s.fileEventWatchers, k) + } else { + level.Warn(s.log).Log("msg", "failed to remove file event watcher", "path", k) + } } } s.droppedTargets = dropped