Background Services: Remove dskitBackgroundServices toggle (grafana#111255)

Author: toddtreece

packages/grafana-data/src/types/featureToggles.gen.ts

@@ -1169,11 +1169,6 @@ export interface FeatureToggles {
   */
   prometheusTypeMigration?: boolean;
   /**
-  * Enables dskit background service wrapper
-  * @default false
-  */
-  dskitBackgroundServices?: boolean;
-  /**
   * Enables running plugins in containers
   * @default false
   */

pkg/modules/modules.go

@@ -23,6 +23,7 @@ type Registry interface {
 }
 
 type Manager interface {
+	services.NamedService
 	Registry
 	Engine
 }

pkg/registry/backgroundsvcs/adapter/manager.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	"github.com/grafana/dskit/services"
+	"go.opentelemetry.io/otel/trace"
 
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/tracing"
@@ -16,7 +17,7 @@ var (
 	stopTimeout = 30 * time.Second
 )
 
-type managerAdapter struct {
+type ManagerAdapter struct {
 	services.NamedService
 
 	reg           registry.BackgroundServiceRegistry
@@ -32,16 +33,21 @@ type managerAdapter struct {
 //   - Graceful shutdown with proper cleanup ordering
 //
 // Services implementing CanBeDisabled that are disabled will be skipped.
-func NewManagerAdapter(reg registry.BackgroundServiceRegistry) *managerAdapter {
-	m := &managerAdapter{
+func NewManagerAdapter(reg registry.BackgroundServiceRegistry) *ManagerAdapter {
+	m := &ManagerAdapter{
 		reg:           reg,
 		dependencyMap: dependencyMap(),
 	}
 	m.NamedService = services.NewBasicService(m.starting, m.running, m.stopping).WithName("backgroundsvcs.managerAdapter")
 	return m
 }
 
-func (m *managerAdapter) starting(ctx context.Context) error {
+func (m *ManagerAdapter) WithDependencies(dependencyMap map[string][]string) *ManagerAdapter {
+	m.dependencyMap = dependencyMap
+	return m
+}
+
+func (m *ManagerAdapter) starting(ctx context.Context) error {
 	spanCtx, span := tracing.Start(ctx, "backgroundsvcs.managerAdapter.starting")
 	defer span.End()
 	logger := log.New("backgroundsvcs.managerAdapter").FromContext(spanCtx)
@@ -76,16 +82,20 @@ func (m *managerAdapter) starting(ctx context.Context) error {
 	manager.RegisterModule(BackgroundServices, nil)
 
 	m.manager = manager
-	return nil
+	if err := m.manager.StartAsync(ctx); err != nil {
+		return err
+	}
+	return m.manager.AwaitRunning(ctx)
 }
 
-func (m *managerAdapter) running(ctx context.Context) error {
-	spanCtx, span := tracing.Start(ctx, "backgroundsvcs.managerAdapter.running")
+func (m *ManagerAdapter) running(ctx context.Context) error {
+	newCtx := trace.ContextWithSpan(context.Background(), trace.SpanFromContext(ctx))
+	spanCtx, span := tracing.Start(newCtx, "backgroundsvcs.managerAdapter.running")
 	defer span.End()
-	return m.manager.Run(spanCtx)
+	return m.manager.AwaitTerminated(spanCtx)
 }
 
-func (m *managerAdapter) stopping(failure error) error {
+func (m *ManagerAdapter) stopping(failure error) error {
 	ctx, cancel := context.WithTimeout(context.Background(), stopTimeout)
 	defer cancel()
 	spanCtx, span := tracing.Start(ctx, "backgroundsvcs.managerAdapter.stopping")
@@ -98,15 +108,16 @@ func (m *managerAdapter) stopping(failure error) error {
 }
 
 // Run initializes and starts all background services using dskit's module and service patterns.
-func (m *managerAdapter) Run(ctx context.Context) error {
+func (m *ManagerAdapter) Run(ctx context.Context) error {
 	if err := m.StartAsync(ctx); err != nil {
 		return err
 	}
-	return m.AwaitTerminated(ctx)
+	stopCtx := trace.ContextWithSpan(context.Background(), trace.SpanFromContext(ctx))
+	return m.AwaitTerminated(stopCtx)
 }
 
 // Shutdown calls calls the underlying manager's Shutdown
-func (m *managerAdapter) Shutdown(ctx context.Context, reason string) error {
+func (m *ManagerAdapter) Shutdown(ctx context.Context, reason string) error {
 	m.StopAsync()
 	return m.AwaitTerminated(ctx)
 }

pkg/registry/backgroundsvcs/adapter/manager_test.go

@@ -28,10 +28,10 @@ func TestNewManagerAdapter(t *testing.T) {
 func TestManagerAdapter_Starting(t *testing.T) {
 	t.Run("empty registry initializes manager", func(t *testing.T) {
 		reg := &mockBackgroundServiceRegistry{services: []registry.BackgroundService{}}
-		adapter := NewManagerAdapter(reg)
-		adapter.dependencyMap = map[string][]string{
-			BackgroundServices: {},
-		}
+		adapter := NewManagerAdapter(reg).WithDependencies(map[string][]string{
+			BackgroundServices: {Core},
+			Core:               {},
+		})
 
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
@@ -55,10 +55,10 @@ func TestManagerAdapter_Starting(t *testing.T) {
 		reg := &mockBackgroundServiceRegistry{
 			services: []registry.BackgroundService{enabledSvc, disabledSvc, namedSvc},
 		}
-		adapter := NewManagerAdapter(reg)
-		adapter.dependencyMap = map[string][]string{
-			BackgroundServices: {},
-		}
+		adapter := NewManagerAdapter(reg).WithDependencies(map[string][]string{
+			BackgroundServices: {Core},
+			Core:               {},
+		})
 
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
@@ -110,8 +110,11 @@ func TestManagerAdapter_Starting(t *testing.T) {
 
 		// Pre-populate the dependency map with the service using the actual service name that will be used
 		serviceName := "*adapter.mockNamedService"
-		adapter.dependencyMap[serviceName] = []string{"custom-dependency"}
-		initialBgDeps := append([]string{}, adapter.dependencyMap[BackgroundServices]...)
+		adapter.WithDependencies(map[string][]string{
+			serviceName:        {BackgroundServices},
+			Core:               {},
+			BackgroundServices: {Core},
+		})
 
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
@@ -121,18 +124,20 @@ func TestManagerAdapter_Starting(t *testing.T) {
 		require.NotNil(t, adapter.manager)
 
 		// Verify the existing dependency was not overwritten
-		require.Equal(t, []string{"custom-dependency"}, adapter.dependencyMap[serviceName])
+		require.Equal(t, []string{BackgroundServices}, adapter.dependencyMap[serviceName])
 
 		// Verify BackgroundServices dependencies were not modified (should not contain the service twice)
 		finalBgDeps := adapter.dependencyMap[BackgroundServices]
-		require.Equal(t, initialBgDeps, finalBgDeps)
+		require.Equal(t, []string{Core}, finalBgDeps)
 	})
 
 	t.Run("service without NamedService interface gets wrapped", func(t *testing.T) {
-		// Create a service that doesn't implement NamedService
 		plainSvc := &mockService{}
 		reg := &mockBackgroundServiceRegistry{services: []registry.BackgroundService{plainSvc}}
-		adapter := NewManagerAdapter(reg)
+		adapter := NewManagerAdapter(reg).WithDependencies(map[string][]string{
+			BackgroundServices: {Core},
+			Core:               {},
+		})
 
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
@@ -151,11 +156,13 @@ func TestManagerAdapter_Starting(t *testing.T) {
 	})
 
 	t.Run("service without CanBeDisabled interface is always enabled", func(t *testing.T) {
-		// Create a service that doesn't implement CanBeDisabled
 		simpleSvc := &simpleBackgroundService{}
 
 		reg := &mockBackgroundServiceRegistry{services: []registry.BackgroundService{simpleSvc}}
-		adapter := NewManagerAdapter(reg)
+		adapter := NewManagerAdapter(reg).WithDependencies(map[string][]string{
+			BackgroundServices: {Core},
+			Core:               {},
+		})
 
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
@@ -168,36 +175,18 @@ func TestManagerAdapter_Starting(t *testing.T) {
 		expectedServiceName := reflect.TypeOf(simpleSvc).String()
 		require.Contains(t, adapter.dependencyMap, expectedServiceName)
 	})
-
-	t.Run("real manager integration test", func(t *testing.T) {
-		testSvc := &mockService{}
-		reg := &mockBackgroundServiceRegistry{services: []registry.BackgroundService{testSvc}}
-		adapter := NewManagerAdapter(reg)
-
-		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-		defer cancel()
-
-		// Use the real manager - this tests actual integration
-		err := adapter.starting(ctx)
-		require.NoError(t, err)
-		require.NotNil(t, adapter.manager)
-
-		// Verify the service was registered in dependency map
-		expectedServiceName := reflect.TypeOf(testSvc).String()
-		require.Contains(t, adapter.dependencyMap, expectedServiceName)
-	})
 }
 
 func TestManagerAdapter_Running(t *testing.T) {
-	t.Run("runs with real manager", func(t *testing.T) {
+	t.Run("runs with manager", func(t *testing.T) {
 		mock := &mockNamedService{name: "mock"}
 		reg := &mockBackgroundServiceRegistry{services: []registry.BackgroundService{
 			mock,
 		}}
-		adapter := NewManagerAdapter(reg)
-		adapter.dependencyMap = map[string][]string{
-			BackgroundServices: {},
-		}
+		adapter := NewManagerAdapter(reg).WithDependencies(map[string][]string{
+			BackgroundServices: {Core},
+			Core:               {},
+		})
 
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
@@ -208,25 +197,6 @@ func TestManagerAdapter_Running(t *testing.T) {
 		err = adapter.AwaitRunning(ctx)
 		require.NoError(t, err)
 	})
-
-	t.Run("running delegates to manager", func(t *testing.T) {
-		reg := &mockBackgroundServiceRegistry{services: []registry.BackgroundService{}}
-		adapter := NewManagerAdapter(reg)
-
-		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-		defer cancel()
-
-		// Initialize with real manager
-		err := adapter.starting(ctx)
-		require.NoError(t, err)
-
-		// Test running method directly - this will likely fail due to missing production modules
-		// but it covers the running method code path
-		err = adapter.running(ctx)
-		if err != nil {
-			require.Contains(t, err.Error(), "no such module")
-		}
-	})
 }
 
 func TestManagerAdapter_Stopping(t *testing.T) {
@@ -262,7 +232,6 @@ func TestManagerAdapter_Stopping(t *testing.T) {
 		ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
 		defer cancel()
 
-		// Initialize the manager first - need to go through starting to initialize manager
 		err := adapter.starting(ctx)
 		require.NoError(t, err)
 		require.NotNil(t, adapter.manager)