Plugins: Move store init to dskit service (#111823)

pkg/services/pluginsintegration/plugininstaller/service.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/grafana/dskit/services"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/plugins"
 	"github.com/grafana/grafana/pkg/plugins/repo"
@@ -18,6 +19,8 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 )
 
+const ServiceName = "plugin.backgroundinstaller"
+
 var (
 	installRequestCounter = prometheus.NewCounterVec(prometheus.CounterOpts{
 		Namespace: "plugins",
@@ -36,6 +39,7 @@ var (
 )
 
 type Service struct {
+	services.NamedService
 	cfg             *setting.Cfg
 	log             log.Logger
 	pluginInstaller plugins.Installer
@@ -43,6 +47,7 @@ type Service struct {
 	pluginRepo      repo.Service
 	features        featuremgmt.FeatureToggles
 	updateChecker   pluginchecker.PluginUpdateChecker
+	installComplete chan struct{} // closed when all plugins are installed (used for testing)
 }
 
 func ProvideService(
@@ -60,21 +65,18 @@ func ProvideService(
 	})
 
 	s := &Service{
-		log:             log.New("plugin.backgroundinstaller"),
+		log:             log.New(ServiceName),
 		cfg:             cfg,
 		pluginInstaller: pluginInstaller,
 		pluginStore:     pluginStore,
 		pluginRepo:      pluginRepo,
 		features:        features,
 		updateChecker:   updateChecker,
+		installComplete: make(chan struct{}),
 	}
-	if len(cfg.PreinstallPluginsSync) > 0 {
-		// Block initialization process until plugins are installed
-		err := s.installPluginsWithTimeout(cfg.PreinstallPluginsSync)
-		if err != nil {
-			return nil, err
-		}
-	}
+
+	s.NamedService = services.NewBasicService(s.starting, s.running, nil).WithName(ServiceName)
+
 	return s, nil
 }
 
@@ -83,24 +85,6 @@ func (s *Service) IsDisabled() bool {
 	return len(s.cfg.PreinstallPluginsAsync) == 0
 }
 
-func (s *Service) installPluginsWithTimeout(pluginsToInstall []setting.InstallPlugin) error {
-	// Installation process does not timeout by default nor reuses the context
-	// passed to the request so we need to handle the timeout here.
-	// We could make this timeout configurable in the future.
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
-	defer cancel()
-	done := make(chan struct{ err error })
-	go func() {
-		done <- struct{ err error }{err: s.installPlugins(ctx, pluginsToInstall, true)}
-	}()
-	select {
-	case <-ctx.Done():
-		return fmt.Errorf("failed to install plugins: %w", ctx.Err())
-	case d := <-done:
-		return d.err
-	}
-}
-
 func (s *Service) shouldUpdate(ctx context.Context, pluginID, currentVersion string, pluginURL string) bool {
 	// If the plugin is installed from a URL, we cannot check for updates as we do not have the version information
 	// from the repository. Therefore, we assume that the plugin should be updated if the URL is provided.
@@ -166,11 +150,34 @@ func (s *Service) installPlugins(ctx context.Context, pluginsToInstall []setting
 	return nil
 }
 
-func (s *Service) Run(ctx context.Context) error {
-	err := s.installPlugins(ctx, s.cfg.PreinstallPluginsAsync, false)
-	if err != nil {
-		// Unexpected error, asynchronous installation should not return errors
-		s.log.Error("Failed to install plugins", "error", err)
+func (s *Service) starting(ctx context.Context) error {
+	if len(s.cfg.PreinstallPluginsSync) > 0 {
+		s.log.Info("Installing plugins", "plugins", s.cfg.PreinstallPluginsSync)
+		if err := s.installPlugins(ctx, s.cfg.PreinstallPluginsSync, true); err != nil {
+			s.log.Error("Failed to install plugins", "error", err)
+			return err
+		}
 	}
+	s.log.Info("Plugins installed", "plugins", s.cfg.PreinstallPluginsSync)
 	return nil
 }
+
+func (s *Service) running(ctx context.Context) error {
+	if len(s.cfg.PreinstallPluginsAsync) > 0 {
+		s.log.Info("Installing plugins", "plugins", s.cfg.PreinstallPluginsAsync)
+		if err := s.installPlugins(ctx, s.cfg.PreinstallPluginsAsync, false); err != nil {
+			s.log.Error("Failed to install plugins", "error", err)
+			return err
+		}
+	}
+	close(s.installComplete)
+	<-ctx.Done()
+	return nil
+}
+
+func (s *Service) Run(ctx context.Context) error {
+	if err := s.StartAsync(ctx); err != nil {
+		return err
+	}
+	return s.AwaitTerminated(ctx)
+}

pkg/services/pluginsintegration/plugininstaller/service_test.go

@@ -26,7 +26,7 @@ func TestService_IsDisabled(t *testing.T) {
 		&setting.Cfg{
 			PreinstallPluginsAsync: []setting.InstallPlugin{{ID: "myplugin"}},
 		},
-		pluginstore.New(registry.NewInMemory(), &fakes.FakeLoader{}),
+		pluginstore.New(registry.NewInMemory(), &fakes.FakeLoader{}, &fakes.FakeSourceRegistry{}),
 		&fakes.FakePluginInstaller{},
 		prometheus.NewRegistry(),
 		&fakes.FakePluginRepo{},
@@ -160,12 +160,14 @@ func TestService_Run(t *testing.T) {
 			}
 			installed := 0
 			installedFromURL := 0
+			store, err := pluginstore.NewPluginStoreForTest(preg, &fakes.FakeLoader{}, &fakes.FakeSourceRegistry{})
+			require.NoError(t, err)
 			s, err := ProvideService(
 				&setting.Cfg{
 					PreinstallPluginsAsync: tt.pluginsToInstall,
 					PreinstallPluginsSync:  tt.pluginsToInstallSync,
 				},
-				pluginstore.New(preg, &fakes.FakeLoader{}),
+				store,
 				&fakes.FakePluginInstaller{
 					AddFunc: func(ctx context.Context, pluginID string, version string, opts plugins.AddOpts) error {
 						for _, plugin := range tt.pluginsToFail {
@@ -203,13 +205,26 @@ func TestService_Run(t *testing.T) {
 					&pluginchecker.FakePluginPreinstall{},
 				),
 			)
+			require.NoError(t, err)
+
+			t.Cleanup(func() {
+				s.StopAsync()
+				err := s.AwaitTerminated(context.Background())
+				if tt.shouldThrowError {
+					require.ErrorContains(t, err, "Failed to install plugin")
+					return
+				}
+				require.NoError(t, err)
+			})
+
+			err = s.StartAsync(context.Background())
+			require.NoError(t, err)
+			err = s.AwaitRunning(context.Background())
 			if tt.shouldThrowError {
 				require.ErrorContains(t, err, "Failed to install plugin")
 				return
 			}
 			require.NoError(t, err)
-			err = s.Run(context.Background())
-			require.NoError(t, err)
 
 			if tt.shouldInstall {
 				expectedInstalled := 0
@@ -232,6 +247,7 @@ func TestService_Run(t *testing.T) {
 						expectedInstalled++
 					}
 				}
+				<-s.installComplete
 				require.Equal(t, expectedInstalled, installed)
 				require.Equal(t, expectedInstalledFromURL, installedFromURL)
 			}