From 36275a551041d507896c9c061b1d7e6398e73bba Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Thu, 6 Feb 2025 09:55:17 +0100 Subject: [PATCH] Advisor: Refactor check interfaces (#100043) --- apps/advisor/pkg/app/app.go | 2 +- .../pkg/app/checks/datasourcecheck/check.go | 98 +++++++++++++---- .../app/checks/datasourcecheck/check_test.go | 43 ++++++-- apps/advisor/pkg/app/checks/ifaces.go | 22 +++- .../pkg/app/checks/plugincheck/check.go | 100 +++++++++++++++--- .../pkg/app/checks/plugincheck/check_test.go | 13 ++- apps/advisor/pkg/app/utils.go | 24 ++++- apps/advisor/pkg/app/utils_test.go | 38 ++++++- 8 files changed, 282 insertions(+), 58 deletions(-) diff --git a/apps/advisor/pkg/app/app.go b/apps/advisor/pkg/app/app.go index 42848758d5a..554357caca2 100644 --- a/apps/advisor/pkg/app/app.go +++ b/apps/advisor/pkg/app/app.go @@ -37,7 +37,7 @@ func New(cfg app.Config) (app.App, error) { // Initialize checks checkMap := map[string]checks.Check{} for _, c := range checkRegistry.Checks() { - checkMap[c.Type()] = c + checkMap[c.ID()] = c } simpleConfig := simple.AppConfig{ diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check.go b/apps/advisor/pkg/app/checks/datasourcecheck/check.go index df05bf40688..f7dbcc53024 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check.go @@ -15,6 +15,13 @@ import ( "k8s.io/klog/v2" ) +type check struct { + DatasourceSvc datasources.DataSourceService + PluginStore pluginstore.Store + PluginContextProvider pluginContextProvider + PluginClient plugins.Client +} + func New( datasourceSvc datasources.DataSourceService, pluginStore pluginstore.Store, @@ -29,28 +36,53 @@ func New( } } -type check struct { - DatasourceSvc datasources.DataSourceService - PluginStore pluginstore.Store - PluginContextProvider pluginContextProvider - PluginClient plugins.Client +func (c *check) Items(ctx context.Context) ([]any, error) { + dss, err := c.DatasourceSvc.GetAllDataSources(ctx, &datasources.GetAllDataSourcesQuery{}) + if err != nil { + return nil, err + } + res := make([]any, len(dss)) + for i, ds := range dss { + res[i] = ds + } + return res, nil } -func (c *check) Type() string { +func (c *check) ID() string { return "datasource" } -func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.CheckV0alpha1StatusReport, error) { - // Optionally read the check input encoded in the object - // fmt.Println(obj.Data) - - dss, err := c.DatasourceSvc.GetAllDataSources(ctx, &datasources.GetAllDataSourcesQuery{}) - if err != nil { - return nil, err +func (c *check) Steps() []checks.Step { + return []checks.Step{ + &uidValidationStep{}, + &healthCheckStep{ + PluginContextProvider: c.PluginContextProvider, + PluginClient: c.PluginClient, + }, } +} + +type uidValidationStep struct{} +func (s *uidValidationStep) ID() string { + return "uid-validation" +} + +func (s *uidValidationStep) Title() string { + return "UID validation" +} + +func (s *uidValidationStep) Description() string { + return "Check if the UID of each data source is valid." +} + +func (s *uidValidationStep) Run(ctx context.Context, obj *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) { dsErrs := []advisor.CheckReportError{} - for _, ds := range dss { + for _, i := range items { + ds, ok := i.(*datasources.DataSource) + if !ok { + return nil, fmt.Errorf("invalid item type %T", i) + } // Data source UID validation err := util.ValidateUID(ds.UID) if err != nil { @@ -60,13 +92,41 @@ func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.Check Action: "Check the documentation for more information.", }) } + } + return dsErrs, nil +} + +type healthCheckStep struct { + PluginContextProvider pluginContextProvider + PluginClient plugins.Client +} + +func (s *healthCheckStep) Title() string { + return "Health check" +} + +func (s *healthCheckStep) Description() string { + return "Check if all data sources are healthy." +} + +func (s *healthCheckStep) ID() string { + return "health-check" +} + +func (s *healthCheckStep) Run(ctx context.Context, obj *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) { + dsErrs := []advisor.CheckReportError{} + for _, i := range items { + ds, ok := i.(*datasources.DataSource) + if !ok { + return nil, fmt.Errorf("invalid item type %T", i) + } // Health check execution requester, err := identity.GetRequester(ctx) if err != nil { return nil, err } - pCtx, err := c.PluginContextProvider.GetWithDataSource(ctx, ds.Type, requester, ds) + pCtx, err := s.PluginContextProvider.GetWithDataSource(ctx, ds.Type, requester, ds) if err != nil { klog.ErrorS(err, "Error creating plugin context", "datasource", ds.Name) continue @@ -75,7 +135,7 @@ func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.Check PluginContext: pCtx, Headers: map[string]string{}, } - resp, err := c.PluginClient.CheckHealth(ctx, req) + resp, err := s.PluginClient.CheckHealth(ctx, req) if err != nil { fmt.Println("Error checking health", err) continue @@ -90,11 +150,7 @@ func (c *check) Run(ctx context.Context, obj *advisor.CheckSpec) (*advisor.Check }) } } - - return &advisor.CheckV0alpha1StatusReport{ - Count: int64(len(dss)), - Errors: dsErrs, - }, nil + return dsErrs, nil } type pluginContextProvider interface { diff --git a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go index 5c3dfb913e3..c360f96ad14 100644 --- a/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go +++ b/apps/advisor/pkg/app/checks/datasourcecheck/check_test.go @@ -31,11 +31,18 @@ func TestCheck_Run(t *testing.T) { } ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - report, err := check.Run(ctx, &advisor.CheckSpec{}) + items, err := check.Items(ctx) + assert.NoError(t, err) + errs := []advisor.CheckReportError{} + for _, step := range check.Steps() { + stepErrs, err := step.Run(ctx, &advisor.CheckSpec{}, items) + assert.NoError(t, err) + errs = append(errs, stepErrs...) + } assert.NoError(t, err) - assert.Equal(t, int64(2), report.Count) - assert.Empty(t, report.Errors) + assert.Equal(t, 2, len(items)) + assert.Empty(t, errs) }) t.Run("should return errors when datasource UID is invalid", func(t *testing.T) { @@ -54,12 +61,19 @@ func TestCheck_Run(t *testing.T) { } ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - report, err := check.Run(ctx, &advisor.CheckSpec{}) + items, err := check.Items(ctx) + assert.NoError(t, err) + errs := []advisor.CheckReportError{} + for _, step := range check.Steps() { + stepErrs, err := step.Run(ctx, &advisor.CheckSpec{}, items) + assert.NoError(t, err) + errs = append(errs, stepErrs...) + } assert.NoError(t, err) - assert.Equal(t, int64(1), report.Count) - assert.Len(t, report.Errors, 1) - assert.Equal(t, "Invalid UID 'invalid uid' for data source Prometheus", report.Errors[0].Reason) + assert.Equal(t, 1, len(items)) + assert.Len(t, errs, 1) + assert.Equal(t, "Invalid UID 'invalid uid' for data source Prometheus", errs[0].Reason) }) t.Run("should return errors when datasource health check fails", func(t *testing.T) { @@ -78,12 +92,19 @@ func TestCheck_Run(t *testing.T) { } ctx := identity.WithRequester(context.Background(), &user.SignedInUser{}) - report, err := check.Run(ctx, &advisor.CheckSpec{}) + items, err := check.Items(ctx) + assert.NoError(t, err) + errs := []advisor.CheckReportError{} + for _, step := range check.Steps() { + stepErrs, err := step.Run(ctx, &advisor.CheckSpec{}, items) + assert.NoError(t, err) + errs = append(errs, stepErrs...) + } assert.NoError(t, err) - assert.Equal(t, int64(1), report.Count) - assert.Len(t, report.Errors, 1) - assert.Equal(t, "Health check failed for Prometheus", report.Errors[0].Reason) + assert.Equal(t, 1, len(items)) + assert.Len(t, errs, 1) + assert.Equal(t, "Health check failed for Prometheus", errs[0].Reason) }) } diff --git a/apps/advisor/pkg/app/checks/ifaces.go b/apps/advisor/pkg/app/checks/ifaces.go index 008f5656298..b630a84871c 100644 --- a/apps/advisor/pkg/app/checks/ifaces.go +++ b/apps/advisor/pkg/app/checks/ifaces.go @@ -6,8 +6,24 @@ import ( advisorv0alpha1 "github.com/grafana/grafana/apps/advisor/pkg/apis/advisor/v0alpha1" ) -// Check defines the methods that a check must implement to be executed. +// Check returns metadata about the check being executed and the list of Steps type Check interface { - Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec) (*advisorv0alpha1.CheckV0alpha1StatusReport, error) - Type() string + // ID returns the unique identifier of the check + ID() string + // Items returns the list of items that will be checked + Items(ctx context.Context) ([]any, error) + // Steps returns the list of steps that will be executed + Steps() []Step +} + +// Step is a single step in a check, including its metadata +type Step interface { + // ID returns the unique identifier of the step + ID() string + // Title returns the title of the step + Title() string + // Description returns the description of the step + Description() string + // Run executes the step and returns a list of errors + Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec, items []any) ([]advisorv0alpha1.CheckReportError, error) } diff --git a/apps/advisor/pkg/app/checks/plugincheck/check.go b/apps/advisor/pkg/app/checks/plugincheck/check.go index 8a20820f450..c64bc1e7579 100644 --- a/apps/advisor/pkg/app/checks/plugincheck/check.go +++ b/apps/advisor/pkg/app/checks/plugincheck/check.go @@ -36,22 +36,63 @@ type check struct { ManagedPlugins managedplugins.Manager } -func (c *check) Type() string { +func (c *check) ID() string { return "plugin" } -func (c *check) Run(ctx context.Context, _ *advisor.CheckSpec) (*advisor.CheckV0alpha1StatusReport, error) { +func (c *check) Items(ctx context.Context) ([]any, error) { ps := c.PluginStore.Plugins(ctx) + res := make([]any, len(ps)) + for i, p := range ps { + res[i] = p + } + return res, nil +} + +func (c *check) Steps() []checks.Step { + return []checks.Step{ + &deprecationStep{ + PluginRepo: c.PluginRepo, + }, + &updateStep{ + PluginRepo: c.PluginRepo, + PluginPreinstall: c.PluginPreinstall, + ManagedPlugins: c.ManagedPlugins, + }, + } +} +type deprecationStep struct { + PluginRepo repo.Service +} + +func (s *deprecationStep) Title() string { + return "Deprecation check" +} + +func (s *deprecationStep) Description() string { + return "Check if any installed plugins are deprecated." +} + +func (s *deprecationStep) ID() string { + return "deprecation" +} + +func (s *deprecationStep) Run(ctx context.Context, _ *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) { errs := []advisor.CheckReportError{} - for _, p := range ps { + for _, i := range items { + p, ok := i.(pluginstore.Plugin) + if !ok { + return nil, fmt.Errorf("invalid item type %T", i) + } + // Skip if it's a core plugin if p.IsCorePlugin() { continue } // Check if plugin is deprecated - i, err := c.PluginRepo.PluginInfo(ctx, p.ID) + i, err := s.PluginRepo.PluginInfo(ctx, p.ID) if err != nil { continue } @@ -62,13 +103,49 @@ func (c *check) Run(ctx context.Context, _ *advisor.CheckSpec) (*advisor.CheckV0 Action: "Check the documentation for recommended steps.", }) } + } + return errs, nil +} - // Check if plugin has a newer version, only if it's not managed or pinned - if c.isManaged(ctx, p.ID) || c.PluginPreinstall.IsPinned(p.ID) { +type updateStep struct { + PluginRepo repo.Service + PluginPreinstall plugininstaller.Preinstall + ManagedPlugins managedplugins.Manager +} + +func (s *updateStep) Title() string { + return "Update check" +} + +func (s *updateStep) Description() string { + return "Check if any installed plugins have a newer version available." +} + +func (s *updateStep) ID() string { + return "update" +} + +func (s *updateStep) Run(ctx context.Context, _ *advisor.CheckSpec, items []any) ([]advisor.CheckReportError, error) { + errs := []advisor.CheckReportError{} + for _, i := range items { + p, ok := i.(pluginstore.Plugin) + if !ok { + return nil, fmt.Errorf("invalid item type %T", i) + } + + // Skip if it's a core plugin + if p.IsCorePlugin() { continue } + + // Skip if it's managed or pinned + if s.isManaged(ctx, p.ID) || s.PluginPreinstall.IsPinned(p.ID) { + continue + } + + // Check if plugin has a newer version available compatOpts := repo.NewCompatOpts(services.GrafanaVersion, sysruntime.GOOS, sysruntime.GOARCH) - info, err := c.PluginRepo.GetPluginArchiveInfo(ctx, p.ID, "", compatOpts) + info, err := s.PluginRepo.GetPluginArchiveInfo(ctx, p.ID, "", compatOpts) if err != nil { continue } @@ -83,10 +160,7 @@ func (c *check) Run(ctx context.Context, _ *advisor.CheckSpec) (*advisor.CheckV0 } } - return &advisor.CheckV0alpha1StatusReport{ - Count: int64(len(ps)), - Errors: errs, - }, nil + return errs, nil } func hasUpdate(current pluginstore.Plugin, latest *repo.PluginArchiveInfo) bool { @@ -100,8 +174,8 @@ func hasUpdate(current pluginstore.Plugin, latest *repo.PluginArchiveInfo) bool return current.Info.Version != latest.Version } -func (c *check) isManaged(ctx context.Context, pluginID string) bool { - for _, managedPlugin := range c.ManagedPlugins.ManagedPlugins(ctx) { +func (s *updateStep) isManaged(ctx context.Context, pluginID string) bool { + for _, managedPlugin := range s.ManagedPlugins.ManagedPlugins(ctx) { if managedPlugin == pluginID { return true } diff --git a/apps/advisor/pkg/app/checks/plugincheck/check_test.go b/apps/advisor/pkg/app/checks/plugincheck/check_test.go index 41c0705e904..53450ce7bc5 100644 --- a/apps/advisor/pkg/app/checks/plugincheck/check_test.go +++ b/apps/advisor/pkg/app/checks/plugincheck/check_test.go @@ -126,10 +126,17 @@ func TestRun(t *testing.T) { managedPlugins := &mockManagedPlugins{managed: tt.pluginManaged} check := New(pluginStore, pluginRepo, pluginPreinstall, managedPlugins) - report, err := check.Run(context.Background(), nil) + items, err := check.Items(context.Background()) assert.NoError(t, err) - assert.Equal(t, int64(len(tt.plugins)), report.Count) - assert.Equal(t, tt.expectedErrors, report.Errors) + errs := []advisor.CheckReportError{} + for _, step := range check.Steps() { + stepErrs, err := step.Run(context.Background(), &advisor.CheckSpec{}, items) + assert.NoError(t, err) + errs = append(errs, stepErrs...) + } + assert.NoError(t, err) + assert.Equal(t, len(tt.plugins), len(items)) + assert.Equal(t, tt.expectedErrors, errs) }) } } diff --git a/apps/advisor/pkg/app/utils.go b/apps/advisor/pkg/app/utils.go index 4eae090198b..c59dbaca21e 100644 --- a/apps/advisor/pkg/app/utils.go +++ b/apps/advisor/pkg/app/utils.go @@ -72,14 +72,32 @@ func processCheck(ctx context.Context, client resource.Client, obj resource.Obje UserUID: uid, FallbackType: typ, }) - // Run the checks - report, err := check.Run(ctx, &c.Spec) + // Get the items to check + items, err := check.Items(ctx) if err != nil { setErr := setStatusAnnotation(ctx, client, obj, "error") if setErr != nil { return setErr } - return err + return fmt.Errorf("error initializing check: %w", err) + } + // Run the steps + steps := check.Steps() + errs := []advisorv0alpha1.CheckReportError{} + for _, step := range steps { + stepErrs, err := step.Run(ctx, &c.Spec, items) + if err != nil { + setErr := setStatusAnnotation(ctx, client, obj, "error") + if setErr != nil { + return setErr + } + return fmt.Errorf("error running step %s: %w", step.Title(), err) + } + errs = append(errs, stepErrs...) + } + report := &advisorv0alpha1.CheckV0alpha1StatusReport{ + Errors: errs, + Count: int64(len(items)), } err = setStatusAnnotation(ctx, client, obj, "processed") if err != nil { diff --git a/apps/advisor/pkg/app/utils_test.go b/apps/advisor/pkg/app/utils_test.go index 6626f645eb7..a13880f8209 100644 --- a/apps/advisor/pkg/app/utils_test.go +++ b/apps/advisor/pkg/app/utils_test.go @@ -115,10 +115,42 @@ func (m *mockClient) PatchInto(ctx context.Context, id resource.Identifier, req } type mockCheck struct { - checks.Check err error } -func (m *mockCheck) Run(ctx context.Context, spec *advisorv0alpha1.CheckSpec) (*advisorv0alpha1.CheckV0alpha1StatusReport, error) { - return &advisorv0alpha1.CheckV0alpha1StatusReport{}, m.err +func (m *mockCheck) ID() string { + return "mock" +} + +func (m *mockCheck) Items(ctx context.Context) ([]any, error) { + return []any{}, nil +} + +func (m *mockCheck) Steps() []checks.Step { + return []checks.Step{ + &mockStep{err: m.err}, + } +} + +type mockStep struct { + err error +} + +func (m *mockStep) Run(ctx context.Context, obj *advisorv0alpha1.CheckSpec, items []any) ([]advisorv0alpha1.CheckReportError, error) { + if m.err != nil { + return nil, m.err + } + return nil, nil +} + +func (m *mockStep) Title() string { + return "mock" +} + +func (m *mockStep) Description() string { + return "mock" +} + +func (m *mockStep) ID() string { + return "mock" }