diff --git a/discovery/manager.go b/discovery/manager.go index b7ebdb7e0..cefa90a86 100644 --- a/discovery/manager.go +++ b/discovery/manager.go @@ -394,8 +394,16 @@ func (m *Manager) updateGroup(poolKey poolKey, tgs []*targetgroup.Group) { m.targets[poolKey] = make(map[string]*targetgroup.Group) } for _, tg := range tgs { - if tg != nil { // Some Discoverers send nil target group so need to check for it to avoid panics. + // Some Discoverers send nil target group so need to check for it to avoid panics. + if tg == nil { + continue + } + if len(tg.Targets) > 0 { m.targets[poolKey][tg.Source] = tg + } else { + // The target group is empty, drop the corresponding entry to avoid leaks. + // In case the group yielded targets before, allGroups() will take care of making consumers drop them. + delete(m.targets[poolKey], tg.Source) } } } diff --git a/discovery/manager_test.go b/discovery/manager_test.go index 707c3931d..831cefe51 100644 --- a/discovery/manager_test.go +++ b/discovery/manager_test.go @@ -1051,8 +1051,8 @@ func TestDiscovererConfigs(t *testing.T) { } // TestTargetSetRecreatesEmptyStaticConfigs ensures that reloading a config file after -// removing all targets from the static_configs sends an update with empty targetGroups. -// This is required to signal the receiver that this target set has no current targets. +// removing all targets from the static_configs cleans the corresponding targetGroups entries to avoid leaks and sends an empty update. +// The update is required to signal the consumers that the previous targets should be dropped. func TestTargetSetRecreatesEmptyStaticConfigs(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -1085,16 +1085,14 @@ func TestTargetSetRecreatesEmptyStaticConfigs(t *testing.T) { discoveryManager.ApplyConfig(c) syncedTargets = <-discoveryManager.SyncCh() + require.Len(t, discoveryManager.targets, 1) p = pk("static", "prometheus", 1) targetGroups, ok := discoveryManager.targets[p] - require.True(t, ok, "'%v' should be present in target groups", p) - group, ok := targetGroups[""] - require.True(t, ok, "missing '' key in target groups %v", targetGroups) - - require.Empty(t, group.Targets, "Invalid number of targets.") - require.Len(t, syncedTargets, 1) - require.Len(t, syncedTargets["prometheus"], 1) - require.Nil(t, syncedTargets["prometheus"][0].Labels) + require.True(t, ok, "'%v' should be present in targets", p) + // Otherwise the targetGroups will leak, see https://github.com/prometheus/prometheus/issues/12436. + require.Empty(t, targetGroups, 0, "'%v' should no longer have any associated target groups", p) + require.Len(t, syncedTargets, 1, "an update with no targetGroups should still be sent.") + require.Empty(t, syncedTargets["prometheus"], 0) } func TestIdenticalConfigurationsAreCoalesced(t *testing.T) { diff --git a/scrape/manager_test.go b/scrape/manager_test.go index f260167b5..c71691c95 100644 --- a/scrape/manager_test.go +++ b/scrape/manager_test.go @@ -1178,7 +1178,7 @@ scrape_configs: ) } -// TestOnlyStaleTargetsAreDropped makes sure that when a job has multiple providers, when aone of them should no, +// TestOnlyStaleTargetsAreDropped makes sure that when a job has multiple providers, when one of them should no // longer discover targets, only the stale targets of that provier are dropped. func TestOnlyStaleTargetsAreDropped(t *testing.T) { ctx, cancel := context.WithCancel(context.Background())