autodiscovery: drop our own probe-race re-check, subsumed by #54368

Now that #54368 (autodiscovery: re-validate discovery probe results against
live filtering before applying) is the base of this branch, its generic
expectedFilteredTemplatesLocked re-check in applyDiscoveredConfigsLocked
already re-runs the full FilterTemplates pass -- including
filterTemplatesDiscovery's generic-integration namespace/rename checks --
immediately before scheduling a probe result. That fully subsumes our own,
narrower conflictingGenericIntegrationLocked re-check and its
discoverySuppressedDigest sentinel bookkeeping, so both are removed:

- conflictingGenericIntegrationLocked and the block that called it in
  applyDiscoveredConfigsLocked (configmgr_discovery.go).
- The discoverySuppressedDigest sentinel const and the now-dead guard in
  reconcileService's deletion loop (configmgr.go) -- #54368's mechanism never
  needs to record a suppressed digest in the first place, since nothing
  re-enqueues an already-completed probe.
- Our three mid-flight regression tests
  (TestConfigMgr_Discovery_SuppressedBySiblingAlreadyResolved,
  ...SuppressedBySiblingMetricRename, ...SuppressedByHostWideStaticNamespace),
  redundant with #54368's own
  TestConfigMgr_Discovery_StaleProbeResultSuppressedByMidFlightConflict, which
  already proves the general re-check mechanism triggers correctly. Coverage
  of our actual namespace/rename-matching logic is untouched, since it lives
  in comp/core/autodiscovery/listeners (filterTemplatesDiscovery,
  GenericIntegrationNamespaceRoots) and is tested directly there.

StaticConfigIndex and its namespace-root population in
processNewConfig/processDelConfigs are unaffected -- filterTemplatesDiscovery
(now invoked both synchronously and via #54368's re-check) still depends on
them.

Environment: Datadog workspace

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: vitkyrka

comp/core/autodiscovery/impl/configmgr.go

@@ -27,24 +27,6 @@ import (
 	"github.com/DataDog/datadog-agent/pkg/util/log"
 )
 
-// discoverySuppressedDigest is a sentinel value recorded in
-// serviceResolutions in place of a real resolved-config digest, for a
-// discovery template whose result was intentionally not scheduled because a
-// generic-integration (openmetrics/prometheus) sibling or host-wide static
-// config already claims a conflicting metric namespace (see
-// applyDiscoveredConfigsLocked in configmgr_discovery.go). Config.Digest() is
-// a hex-formatted hash and can never be empty, so "" is safe to use as a
-// sentinel without colliding with a real digest.
-//
-// Recording this (rather than leaving the digest out of serviceResolutions
-// entirely) prevents reconcileService from treating the template as
-// "not yet resolved" and re-triggering a fresh discovery probe on every
-// unrelated reconcile of the same service. The deletion loop in
-// reconcileService must skip this sentinel rather than looking it up in
-// scheduledConfigs (which would return a zero-value Config and produce a
-// bogus unschedule).
-const discoverySuppressedDigest = ""
-
 // configManager implements the logic of handling additions and removals of
 // configs (which may or may not be templates) and services, and reconciling
 // those together to resolve templates.
@@ -480,9 +462,7 @@ func (cm *reconcilingConfigManager) reconcileService(svcID string) integration.C
 	// existingResolutions in-place
 	for templateDigest, resolvedDigest := range existingResolutions {
 		if _, found = expectedResolutions[templateDigest]; !found {
-			if resolvedDigest != discoverySuppressedDigest {
-				changes.UnscheduleConfig(cm.scheduledConfigs[resolvedDigest])
-			}
+			changes.UnscheduleConfig(cm.scheduledConfigs[resolvedDigest])
 			delete(existingResolutions, templateDigest)
 			// Clear any health issue for this template+service pair
 			if tpl, ok := cm.activeConfigs[templateDigest]; ok {

comp/core/autodiscovery/impl/configmgr_discovery.go

@@ -9,7 +9,6 @@ package autodiscoveryimpl
 
 import (
 	"errors"
-	"fmt"
 	"strings"
 
 	"github.com/DataDog/datadog-agent/comp/core/autodiscovery/configresolver"
@@ -164,31 +163,12 @@ func (cm *reconcilingConfigManager) applyDiscoveredConfigsLocked(svcID, tplDiges
 	if !ok {
 		existing = map[string]string{}
 	}
-	if prevDigest, hadPrev := existing[tplDigest]; hadPrev && prevDigest != discoverySuppressedDigest {
+	if prevDigest, hadPrev := existing[tplDigest]; hadPrev {
 		if old, found := cm.scheduledConfigs[prevDigest]; found {
 			changes.UnscheduleConfig(old)
 		}
 	}
 
-	// The discovery probe was enqueued (in resolveTemplateForService) when
-	// this template first matched the service, at which point a
-	// generic-integration (openmetrics/prometheus) sibling might not have
-	// existed yet — filterTemplatesDiscovery only guards against siblings
-	// present *before* the probe starts. By the time the probe completes
-	// (at least one discoverer retry cycle later), such a sibling may now be
-	// scheduled for this same service, or a host-wide static config may now
-	// claim the namespace. Re-check here before scheduling the result, so a
-	// slow-arriving discovery probe can't schedule a duplicate on top of a
-	// sibling that showed up in the meantime.
-	if conflictName, ok := cm.conflictingGenericIntegrationLocked(svcID, tplDigest, tpl.Name); ok {
-		msg := fmt.Sprintf("discovery skipped for %s on service %s: %s already covers this metric namespace", tpl.Name, svcID, conflictName)
-		log.Debugf("autodiscovery: %s", msg)
-		errorStats.setResolveWarning(tpl.Name, msg)
-		existing[tplDigest] = discoverySuppressedDigest
-		cm.serviceResolutions[svcID] = existing
-		return cm.applyChanges(changes)
-	}
-
 	existing[tplDigest] = decrypted.Digest()
 	cm.serviceResolutions[svcID] = existing
 
@@ -197,35 +177,6 @@ func (cm *reconcilingConfigManager) applyDiscoveredConfigsLocked(svcID, tplDiges
 	return cm.applyChanges(changes)
 }
 
-// conflictingGenericIntegrationLocked reports whether a generic-integration
-// (openmetrics/prometheus) config already claims a metric namespace whose
-// root matches checkName — either a sibling already resolved for the same
-// service (svcID, excluding tplDigest itself), or a host-wide static config
-// tracked (by namespace root) in cm.staticConfigIndex. On a match, it returns
-// the conflicting config's Name (or "a static config" for the host-wide case)
-// and true. Must be called with cm.m held.
-func (cm *reconcilingConfigManager) conflictingGenericIntegrationLocked(svcID, tplDigest, checkName string) (string, bool) {
-	if cm.staticConfigIndex.Has(checkName) {
-		return "a static config", true
-	}
-
-	for otherDigest, otherResolvedDigest := range cm.serviceResolutions[svcID] {
-		if otherDigest == tplDigest || otherResolvedDigest == discoverySuppressedDigest {
-			continue
-		}
-		otherCfg, found := cm.scheduledConfigs[otherResolvedDigest]
-		if !found || !listeners.IsGenericIntegrationCheckName(otherCfg.Name) {
-			continue
-		}
-		for _, root := range listeners.GenericIntegrationNamespaceRoots(otherCfg) {
-			if root == checkName {
-				return otherCfg.Name, true
-			}
-		}
-	}
-	return "", false
-}
-
 // rewriteSource rewrites a resolved config's file-based Source to encode that
 // it was applied via a configuration-discovery probe result, and whether the
 // target service is a process or a container. Only the "file" provider is

comp/core/autodiscovery/impl/configmgr_discovery_test.go

@@ -700,174 +700,3 @@ func TestConfigMgr_Lifecycle_HostMultiNetworkBridge(t *testing.T) {
 // fastWorkerMaxAttempts is the retry budget used by the
 // EmptyDiscoveryResult lifecycle test — small so the test finishes quickly.
 const fastWorkerMaxAttempts = 3
-
-// TestConfigMgr_Discovery_SuppressedBySiblingAlreadyResolved reproduces the
-// DSCVR-626 race: the discovery probe for a template is enqueued (in
-// resolveTemplateForService) before a generic-integration (openmetrics)
-// sibling exists for the same service, and only resolves *after* that
-// sibling has already been scheduled with a matching (rooted-in) namespace.
-// filterTemplatesDiscovery's pre-probe check can't catch this — the sibling
-// wasn't there yet when it ran — so applyDiscoveredConfigsLocked must
-// re-check immediately before scheduling the (slow) probe result.
-func TestConfigMgr_Discovery_SuppressedBySiblingAlreadyResolved(t *testing.T) {
-	errorStats = newAcErrorStats() // isolate from other tests' warnings
-
-	mockResolver := MockSecretResolver{}
-	disco := newStubDiscoverer(func(_, _ string) (string, error) {
-		return `[{"instances":[{"openmetrics_endpoint":"http://%%host%%:9091/metrics"}]}]`, nil
-	})
-	cm := newReconcilingConfigManager(&mockResolver, nil, nil, disco, nil).(*reconcilingConfigManager)
-	cm.start()
-	t.Cleanup(cm.stop)
-
-	svc := &dummyService{
-		ID:            "docker://conflict-1",
-		ADIdentifiers: []string{"krakend", "docker://conflict-1"},
-		Hosts:         map[string]string{"main": "10.0.0.1"},
-	}
-
-	// The manual openmetrics sibling, matched to this same service via its
-	// own entity ID, resolves synchronously (it's not a Discovery template)
-	// as soon as both are known.
-	sibling := integration.Config{
-		Name:          "openmetrics",
-		ADIdentifiers: []string{"docker://conflict-1"},
-		Instances:     []integration.Data{[]byte("namespace: krakend.api\nopenmetrics_endpoint: http://10.0.0.1:9091/metrics")},
-	}
-	_ = cm.processNewService(svc)
-	changes, _ := cm.processNewConfig(sibling)
-	assertConfigsMatch(t, changes.Schedule, matchName("openmetrics"))
-	require.Len(t, changes.Schedule, 1, "the openmetrics sibling should resolve synchronously")
-
-	// Only now does krakend's discovery template arrive and get enqueued —
-	// by the time the (stubbed) probe "completes", the sibling above is
-	// already fully resolved and scheduled.
-	tpl := integration.Config{
-		Name:          "krakend",
-		ADIdentifiers: []string{"krakend"},
-		Discovery:     &integration.DiscoveryConfig{},
-	}
-	_, _ = cm.processNewConfig(tpl)
-
-	// No discovered config should ever be delivered for krakend.
-	select {
-	case discovered := <-cm.discoveredChanges():
-		t.Fatalf("krakend discovery should have been suppressed, got: %+v", discovered)
-	case <-time.After(500 * time.Millisecond):
-	}
-
-	cm.m.Lock()
-	resolvedDigest, found := cm.serviceResolutions[svc.ID][tpl.Digest()]
-	cm.m.Unlock()
-	require.True(t, found, "the suppressed digest should still be recorded so reconcileService doesn't reprobe")
-	assert.Equal(t, discoverySuppressedDigest, resolvedDigest)
-
-	warnings := GetResolveWarnings()
-	assert.Contains(t, warnings, "krakend")
-}
-
-// TestConfigMgr_Discovery_SuppressedBySiblingMetricRename mirrors
-// TestConfigMgr_Discovery_SuppressedBySiblingAlreadyResolved, but the sibling
-// has no namespace: field at all -- instead it renames a metric directly to
-// krakend's own fully-qualified namespace via metrics:, which (per
-// https://datadoghq.atlassian.net/browse/DSCVR-626?focusedCommentId=3493570)
-// is submitted completely unprefixed and collides just the same.
-func TestConfigMgr_Discovery_SuppressedBySiblingMetricRename(t *testing.T) {
-	errorStats = newAcErrorStats() // isolate from other tests' warnings
-
-	mockResolver := MockSecretResolver{}
-	disco := newStubDiscoverer(func(_, _ string) (string, error) {
-		return `[{"instances":[{"openmetrics_endpoint":"http://%%host%%:9091/metrics"}]}]`, nil
-	})
-	cm := newReconcilingConfigManager(&mockResolver, nil, nil, disco, nil).(*reconcilingConfigManager)
-	cm.start()
-	t.Cleanup(cm.stop)
-
-	svc := &dummyService{
-		ID:            "docker://conflict-2",
-		ADIdentifiers: []string{"krakend", "docker://conflict-2"},
-		Hosts:         map[string]string{"main": "10.0.0.1"},
-	}
-
-	sibling := integration.Config{
-		Name:          "openmetrics",
-		ADIdentifiers: []string{"docker://conflict-2"},
-		Instances: []integration.Data{[]byte(
-			"openmetrics_endpoint: http://10.0.0.1:9091/metrics\n" +
-				"metrics:\n  - krakend_requests_total: krakend.api.requests_total",
-		)},
-	}
-	_ = cm.processNewService(svc)
-	changes, _ := cm.processNewConfig(sibling)
-	assertConfigsMatch(t, changes.Schedule, matchName("openmetrics"))
-	require.Len(t, changes.Schedule, 1, "the openmetrics sibling should resolve synchronously")
-
-	tpl := integration.Config{
-		Name:          "krakend",
-		ADIdentifiers: []string{"krakend"},
-		Discovery:     &integration.DiscoveryConfig{},
-	}
-	_, _ = cm.processNewConfig(tpl)
-
-	select {
-	case discovered := <-cm.discoveredChanges():
-		t.Fatalf("krakend discovery should have been suppressed, got: %+v", discovered)
-	case <-time.After(500 * time.Millisecond):
-	}
-
-	cm.m.Lock()
-	resolvedDigest, found := cm.serviceResolutions[svc.ID][tpl.Digest()]
-	cm.m.Unlock()
-	require.True(t, found, "the suppressed digest should still be recorded so reconcileService doesn't reprobe")
-	assert.Equal(t, discoverySuppressedDigest, resolvedDigest)
-
-	warnings := GetResolveWarnings()
-	assert.Contains(t, warnings, "krakend")
-}
-
-// TestConfigMgr_Discovery_SuppressedByHostWideStaticNamespace mirrors the
-// above but for the non-service-scoped case: a static (non-template)
-// openmetrics config claiming a matching namespace anywhere on the host,
-// tracked (by namespace root) in staticConfigIndex, must also suppress a slow
-// discovery probe result at apply time.
-func TestConfigMgr_Discovery_SuppressedByHostWideStaticNamespace(t *testing.T) {
-	errorStats = newAcErrorStats()
-
-	mockResolver := MockSecretResolver{}
-	disco := newStubDiscoverer(func(_, _ string) (string, error) {
-		return `[{"instances":[{"openmetrics_endpoint":"http://%%host%%:9091/metrics"}]}]`, nil
-	})
-	// The config manager would normally add the *root* of a static
-	// generic-integration config's namespace (see listeners.NamespaceRoot),
-	// e.g. "krakend" for "krakend.api" — simulate that directly here.
-	idx := listeners.NewStaticConfigIndex()
-	idx.Add("krakend")
-	cm := newReconcilingConfigManager(&mockResolver, nil, idx, disco, nil).(*reconcilingConfigManager)
-	cm.start()
-	t.Cleanup(cm.stop)
-
-	svc := &dummyService{
-		ID:            "docker://k1",
-		ADIdentifiers: []string{"krakend"},
-		Hosts:         map[string]string{"main": "10.0.0.1"},
-	}
-	tpl := integration.Config{
-		Name:          "krakend",
-		ADIdentifiers: []string{"krakend"},
-		Discovery:     &integration.DiscoveryConfig{},
-	}
-	_, _ = cm.processNewConfig(tpl)
-	_ = cm.processNewService(svc)
-
-	select {
-	case discovered := <-cm.discoveredChanges():
-		t.Fatalf("krakend discovery should have been suppressed, got: %+v", discovered)
-	case <-time.After(500 * time.Millisecond):
-	}
-
-	cm.m.Lock()
-	resolvedDigest, found := cm.serviceResolutions[svc.ID][tpl.Digest()]
-	cm.m.Unlock()
-	require.True(t, found)
-	assert.Equal(t, discoverySuppressedDigest, resolvedDigest)
-}