Fix race with targets update during ApplyConfig (#9656)

I ended up extending the lock so refTargets remains valid for the
duration of the update.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

discovery/manager.go

@@ -200,14 +200,14 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error {
 		// refTargets keeps reference targets used to populate new subs' targets
 		var refTargets map[string]*targetgroup.Group
 		prov.mu.Lock()
+
+		m.targetsMtx.Lock()
 		for s := range prov.subs {
 			keep = true
 			refTargets = m.targets[poolKey{s, prov.name}]
 			// Remove obsolete subs' targets.
 			if _, ok := prov.newSubs[s]; !ok {
-				m.targetsMtx.Lock()
 				delete(m.targets, poolKey{s, prov.name})
-				m.targetsMtx.Unlock()
 				discoveredTargets.DeleteLabelValues(m.name, s)
 			}
 		}
@@ -223,6 +223,8 @@ func (m *Manager) ApplyConfig(cfg map[string]Configs) error {
 				}
 			}
 		}
+		m.targetsMtx.Unlock()
+
 		prov.subs = prov.newSubs
 		prov.newSubs = map[string]struct{}{}
 		prov.mu.Unlock()

discovery/manager_test.go

@@ -1394,3 +1394,91 @@ func (o onceProvider) Run(_ context.Context, ch chan<- []*targetgroup.Group) {
 	}
 	close(ch)
 }
+
+// TestTargetSetTargetGroupsUpdateDuringApplyConfig is used to detect races when
+// ApplyConfig happens at the same time as targets update.
+func TestTargetSetTargetGroupsUpdateDuringApplyConfig(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	discoveryManager := NewManager(ctx, log.NewNopLogger())
+	discoveryManager.updatert = 100 * time.Millisecond
+	go discoveryManager.Run()
+
+	td := newTestDiscoverer()
+
+	c := map[string]Configs{
+		"prometheus": {
+			td,
+		},
+	}
+	discoveryManager.ApplyConfig(c)
+
+	var wg sync.WaitGroup
+	wg.Add(2000)
+
+	start := make(chan struct{})
+	for i := 0; i < 1000; i++ {
+		go func() {
+			<-start
+			td.update([]*targetgroup.Group{
+				{
+					Targets: []model.LabelSet{
+						{model.AddressLabel: model.LabelValue("127.0.0.1:9090")},
+					},
+				},
+			})
+			wg.Done()
+		}()
+	}
+
+	for i := 0; i < 1000; i++ {
+		go func(i int) {
+			<-start
+			c := map[string]Configs{
+				fmt.Sprintf("prometheus-%d", i): {
+					td,
+				},
+			}
+			discoveryManager.ApplyConfig(c)
+			wg.Done()
+		}(i)
+	}
+
+	close(start)
+	wg.Wait()
+}
+
+// testDiscoverer is a config and a discoverer that can adjust targets with a
+// simple function.
+type testDiscoverer struct {
+	up    chan<- []*targetgroup.Group
+	ready chan struct{}
+}
+
+func newTestDiscoverer() *testDiscoverer {
+	return &testDiscoverer{
+		ready: make(chan struct{}),
+	}
+}
+
+// Name implements Config.
+func (t *testDiscoverer) Name() string {
+	return "test"
+}
+
+// NewDiscoverer implements Config.
+func (t *testDiscoverer) NewDiscoverer(DiscovererOptions) (Discoverer, error) {
+	return t, nil
+}
+
+// Run implements Discoverer.
+func (t *testDiscoverer) Run(ctx context.Context, up chan<- []*targetgroup.Group) {
+	t.up = up
+	close(t.ready)
+	<-ctx.Done()
+}
+
+func (t *testDiscoverer) update(tgs []*targetgroup.Group) {
+	<-t.ready
+	t.up <- tgs
+}