From b2eeb0dd6ed5957d4eb76df3aafcd8e4dc46902e Mon Sep 17 00:00:00 2001 From: Alexander Akhmetov Date: Tue, 13 Aug 2024 12:26:26 +0200 Subject: [PATCH] Alerting: update rule versions on folder move (#88376) * Alerting: update rule versions on folder move (#88361) * Add tracing to folder.Move and folder.Update --- pkg/api/dashboard_test.go | 2 +- pkg/api/folder_bench_test.go | 2 +- pkg/events/events.go | 9 ++- .../annotationsimpl/annotations_test.go | 2 +- .../database/database_folder_test.go | 2 +- .../dashboards/database/database_test.go | 4 +- pkg/services/folder/folderimpl/folder.go | 59 ++++++++++++++++--- pkg/services/folder/folderimpl/folder_test.go | 12 +++- .../libraryelements/libraryelements_test.go | 4 +- .../librarypanels/librarypanels_test.go | 4 +- .../ngalert/api/api_provisioning_test.go | 2 +- pkg/services/ngalert/api/persist.go | 4 +- pkg/services/ngalert/ngalert.go | 11 ++-- pkg/services/ngalert/ngalert_test.go | 44 +++++++++----- .../ngalert/provisioning/alert_rules_test.go | 2 +- pkg/services/ngalert/store/alert_rule.go | 18 ++++-- pkg/services/ngalert/store/alert_rule_test.go | 52 ++++++++++++++++ pkg/services/ngalert/tests/fakes/rules.go | 13 ++-- pkg/services/ngalert/testutil/testutil.go | 3 +- .../sqlstore/permissions/dashboard_test.go | 2 +- .../permissions/dashboards_bench_test.go | 2 +- 21 files changed, 196 insertions(+), 57 deletions(-) diff --git a/pkg/api/dashboard_test.go b/pkg/api/dashboard_test.go index 09855077aac..617d836f22a 100644 --- a/pkg/api/dashboard_test.go +++ b/pkg/api/dashboard_test.go @@ -834,7 +834,7 @@ func getDashboardShouldReturn200WithConfig(t *testing.T, sc *scenarioContext, pr dashboardPermissions := accesscontrolmock.NewMockedPermissionsService() folderSvc := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), - dashboardStore, folderStore, db.InitTestDB(t), features, supportbundlestest.NewFakeBundleService(), nil) + dashboardStore, folderStore, db.InitTestDB(t), features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) if dashboardService == nil { dashboardService, err = service.ProvideDashboardServiceImpl( diff --git a/pkg/api/folder_bench_test.go b/pkg/api/folder_bench_test.go index 02f729ff2b2..dc4a844254b 100644 --- a/pkg/api/folder_bench_test.go +++ b/pkg/api/folder_bench_test.go @@ -458,7 +458,7 @@ func setupServer(b testing.TB, sc benchScenario, features featuremgmt.FeatureTog folderStore := folderimpl.ProvideDashboardFolderStore(sc.db.DB()) ac := acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()) - folderServiceWithFlagOn := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, folderStore, sc.db.DB(), features, supportbundlestest.NewFakeBundleService(), nil) + folderServiceWithFlagOn := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, folderStore, sc.db.DB(), features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) cfg := setting.NewCfg() actionSets := resourcepermissions.NewActionSetService(features) diff --git a/pkg/events/events.go b/pkg/events/events.go index 9ad8d51a6f6..38b6dce4090 100644 --- a/pkg/events/events.go +++ b/pkg/events/events.go @@ -71,10 +71,13 @@ type DataSourceCreated struct { OrgID int64 `json:"org_id"` } -type FolderTitleUpdated struct { +// FolderFullPathUpdated is emitted when the full path of the folder(s) is updated. +// For example, when the folder is renamed or moved to another folder. +// It does not contain the full path of the folders because calculating +// it requires more resources and not needed in the event at the moment. +type FolderFullPathUpdated struct { Timestamp time.Time `json:"timestamp"` - Title string `json:"name"` ID int64 `json:"id"` - UID string `json:"uid"` + UIDs []string `json:"uids"` OrgID int64 `json:"org_id"` } diff --git a/pkg/services/annotations/annotationsimpl/annotations_test.go b/pkg/services/annotations/annotationsimpl/annotations_test.go index 8e8a7e92d06..99f797de0cd 100644 --- a/pkg/services/annotations/annotationsimpl/annotations_test.go +++ b/pkg/services/annotations/annotationsimpl/annotations_test.go @@ -225,7 +225,7 @@ func TestIntegrationAnnotationListingWithInheritedRBAC(t *testing.T) { }) ac := acimpl.ProvideAccessControl(features, zanzana.NewNoopClient()) - folderSvc := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, folderimpl.ProvideDashboardFolderStore(sql), sql, features, supportbundlestest.NewFakeBundleService(), nil) + folderSvc := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, folderimpl.ProvideDashboardFolderStore(sql), sql, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) cfg.AnnotationMaximumTagsLength = 60 diff --git a/pkg/services/dashboards/database/database_folder_test.go b/pkg/services/dashboards/database/database_folder_test.go index ffbf6413c79..6f52c415e55 100644 --- a/pkg/services/dashboards/database/database_folder_test.go +++ b/pkg/services/dashboards/database/database_folder_test.go @@ -303,7 +303,7 @@ func TestIntegrationDashboardInheritedFolderRBAC(t *testing.T) { guardian.New = origNewGuardian }) - folderSvc := folderimpl.ProvideService(mock.New(), bus.ProvideBus(tracer), dashboardWriteStore, folderimpl.ProvideDashboardFolderStore(sqlStore.DB()), sqlStore.DB(), features, supportbundlestest.NewFakeBundleService(), nil) + folderSvc := folderimpl.ProvideService(mock.New(), bus.ProvideBus(tracer), dashboardWriteStore, folderimpl.ProvideDashboardFolderStore(sqlStore.DB()), sqlStore.DB(), features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) parentUID := "" for i := 0; ; i++ { diff --git a/pkg/services/dashboards/database/database_test.go b/pkg/services/dashboards/database/database_test.go index b36337db2d8..0e77bfee2d5 100644 --- a/pkg/services/dashboards/database/database_test.go +++ b/pkg/services/dashboards/database/database_test.go @@ -830,7 +830,7 @@ func TestIntegrationFindDashboardsByTitle(t *testing.T) { ac := acimpl.ProvideAccessControl(features, zanzana.NewNoopClient()) folderStore := folderimpl.ProvideDashboardFolderStore(sqlStore) - folderServiceWithFlagOn := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil) + folderServiceWithFlagOn := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) user := &user.SignedInUser{ OrgID: 1, @@ -948,7 +948,7 @@ func TestIntegrationFindDashboardsByFolder(t *testing.T) { ac := acimpl.ProvideAccessControl(features, zanzana.NewNoopClient()) folderStore := folderimpl.ProvideDashboardFolderStore(sqlStore) - folderServiceWithFlagOn := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil) + folderServiceWithFlagOn := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) user := &user.SignedInUser{ OrgID: 1, diff --git a/pkg/services/folder/folderimpl/folder.go b/pkg/services/folder/folderimpl/folder.go index c3b36588454..48cc32b7b76 100644 --- a/pkg/services/folder/folderimpl/folder.go +++ b/pkg/services/folder/folderimpl/folder.go @@ -13,6 +13,8 @@ import ( "github.com/grafana/dskit/concurrency" "github.com/prometheus/client_golang/prometheus" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/trace" "golang.org/x/exp/slices" "github.com/grafana/grafana/pkg/apimachinery/identity" @@ -20,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/events" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/metrics" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/dashboards/dashboardaccess" @@ -44,12 +47,14 @@ type Service struct { dashboardFolderStore folder.FolderStore features featuremgmt.FeatureToggles accessControl accesscontrol.AccessControl - // bus is currently used to publish event in case of title change + // bus is currently used to publish event in case of folder full path change. + // For example when a folder is moved to another folder or when a folder is renamed. bus bus.Bus mutex sync.RWMutex registry map[string]folder.RegistryService metrics *foldersMetrics + tracer tracing.Tracer } func ProvideService( @@ -61,6 +66,7 @@ func ProvideService( features featuremgmt.FeatureToggles, supportBundles supportbundles.Service, r prometheus.Registerer, + tracer tracing.Tracer, ) folder.Service { store := ProvideStore(db) srv := &Service{ @@ -74,6 +80,7 @@ func ProvideService( db: db, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(r), + tracer: tracer, } srv.DBMigration(db) @@ -655,6 +662,9 @@ func (s *Service) Create(ctx context.Context, cmd *folder.CreateFolderCommand) ( } func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) (*folder.Folder, error) { + ctx, span := s.tracer.Start(ctx, "folder.Update") + defer span.End() + if cmd.SignedInUser == nil { return nil, folder.ErrBadRequest.Errorf("missing signed in user") } @@ -679,14 +689,8 @@ func (s *Service) Update(ctx context.Context, cmd *folder.UpdateFolderCommand) ( if cmd.NewTitle != nil { metrics.MFolderIDsServiceCount.WithLabelValues(metrics.Folder).Inc() - if err := s.bus.Publish(ctx, &events.FolderTitleUpdated{ - Timestamp: foldr.Updated, - Title: foldr.Title, - ID: dashFolder.ID, // nolint:staticcheck - UID: dashFolder.UID, - OrgID: cmd.OrgID, - }); err != nil { - s.log.ErrorContext(ctx, "failed to publish FolderTitleUpdated event", "folder", foldr.Title, "user", cmd.SignedInUser.GetID(), "error", err) + + if err := s.publishFolderFullPathUpdatedEvent(ctx, foldr.Updated, cmd.OrgID, cmd.UID); err != nil { return err } } @@ -873,6 +877,9 @@ func (s *Service) legacyDelete(ctx context.Context, cmd *folder.DeleteFolderComm } func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*folder.Folder, error) { + ctx, span := s.tracer.Start(ctx, "folder.Move") + defer span.End() + if cmd.SignedInUser == nil { return nil, folder.ErrBadRequest.Errorf("missing signed in user") } @@ -947,6 +954,10 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol return folder.ErrInternal.Errorf("failed to move legacy folder: %w", err) } + if err := s.publishFolderFullPathUpdatedEvent(ctx, f.Updated, cmd.OrgID, cmd.UID); err != nil { + return err + } + return nil }); err != nil { return nil, err @@ -954,6 +965,36 @@ func (s *Service) Move(ctx context.Context, cmd *folder.MoveFolderCommand) (*fol return f, nil } +func (s *Service) publishFolderFullPathUpdatedEvent(ctx context.Context, timestamp time.Time, orgID int64, folderUID string) error { + ctx, span := s.tracer.Start(ctx, "folder.publishFolderFullPathUpdatedEvent") + defer span.End() + + descFolders, err := s.store.GetDescendants(ctx, orgID, folderUID) + if err != nil { + s.log.ErrorContext(ctx, "Failed to get descendants of the folder", "folderUID", folderUID, "orgID", orgID, "error", err) + return err + } + uids := make([]string, 0, len(descFolders)+1) + uids = append(uids, folderUID) + for _, f := range descFolders { + uids = append(uids, f.UID) + } + span.AddEvent("found folder descendants", trace.WithAttributes( + attribute.Int64("folders", int64(len(uids))), + )) + + if err := s.bus.Publish(ctx, &events.FolderFullPathUpdated{ + Timestamp: timestamp, + UIDs: uids, + OrgID: orgID, + }); err != nil { + s.log.ErrorContext(ctx, "Failed to publish FolderFullPathUpdated event", "folderUID", folderUID, "orgID", orgID, "descendantsUIDs", uids, "error", err) + return err + } + + return nil +} + func (s *Service) canMove(ctx context.Context, cmd *folder.MoveFolderCommand) (bool, error) { // Check that the user is allowed to move the folder to the destination folder var evaluator accesscontrol.Evaluator diff --git a/pkg/services/folder/folderimpl/folder_test.go b/pkg/services/folder/folderimpl/folder_test.go index ee18314c129..b55017af13f 100644 --- a/pkg/services/folder/folderimpl/folder_test.go +++ b/pkg/services/folder/folderimpl/folder_test.go @@ -63,7 +63,7 @@ func TestIntegrationProvideFolderService(t *testing.T) { t.Run("should register scope resolvers", func(t *testing.T) { ac := acmock.New() db := db.InitTestDB(t) - ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil) + ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), nil, nil, db, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) require.Len(t, ac.Calls.RegisterAttributeScopeResolver, 3) }) @@ -100,6 +100,7 @@ func TestIntegrationFolderService(t *testing.T) { accessControl: acimpl.ProvideAccessControl(features, zanzana.NewNoopClient()), metrics: newFoldersMetrics(nil), registry: make(map[string]folder.RegistryService), + tracer: tracing.InitializeTracerForTest(), } require.NoError(t, service.RegisterService(alertingStore)) @@ -440,6 +441,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { accessControl: ac, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } signedInUser := user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{ @@ -553,6 +555,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { db: db, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } origNewGuardian := guardian.New @@ -630,6 +633,7 @@ func TestIntegrationNestedFolderService(t *testing.T) { db: db, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } testCases := []struct { @@ -805,6 +809,7 @@ func TestNestedFolderServiceFeatureToggle(t *testing.T) { features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), accessControl: acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } t.Run("create folder", func(t *testing.T) { nestedFolderStore.ExpectedFolder = &folder.Folder{ParentUID: util.GenerateShortUID()} @@ -841,6 +846,7 @@ func TestFolderServiceDualWrite(t *testing.T) { features: featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders), accessControl: acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient()), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), bus: bus.ProvideBus(tracing.InitializeTracerForTest()), } @@ -1475,6 +1481,7 @@ func TestIntegrationNestedFolderSharedWithMe(t *testing.T) { accessControl: ac, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } dashboardPermissions := acmock.NewMockedPermissionsService() @@ -1977,6 +1984,7 @@ func TestFolderServiceGetFolders(t *testing.T) { accessControl: ac, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } signedInAdminUser := user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{ @@ -2063,6 +2071,7 @@ func TestGetChildrenFilterByPermission(t *testing.T) { accessControl: ac, registry: make(map[string]folder.RegistryService), metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } origGuardian := guardian.New @@ -2523,6 +2532,7 @@ func setup(t *testing.T, dashStore dashboards.Store, dashboardFolderStore folder accessControl: ac, db: db, metrics: newFoldersMetrics(nil), + tracer: tracing.InitializeTracerForTest(), } } diff --git a/pkg/services/libraryelements/libraryelements_test.go b/pkg/services/libraryelements/libraryelements_test.go index 3b3b3d77fd5..9937ceb6aae 100644 --- a/pkg/services/libraryelements/libraryelements_test.go +++ b/pkg/services/libraryelements/libraryelements_test.go @@ -329,7 +329,7 @@ func createFolder(t *testing.T, sc scenarioContext, title string) *folder.Folder require.NoError(t, err) folderStore := folderimpl.ProvideDashboardFolderStore(sc.sqlStore) - s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sc.sqlStore, features, supportbundlestest.NewFakeBundleService(), nil) + s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sc.sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) t.Logf("Creating folder with title and UID %q", title) ctx := identity.WithRequester(context.Background(), &sc.user) folder, err := s.Create(ctx, &folder.CreateFolderCommand{ @@ -463,7 +463,7 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo Cfg: cfg, features: featuremgmt.WithFeatures(), SQLStore: sqlStore, - folderService: folderimpl.ProvideService(ac, bus.ProvideBus(tracer), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil), + folderService: folderimpl.ProvideService(ac, bus.ProvideBus(tracer), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()), } // deliberate difference between signed in user and user in db to make it crystal clear diff --git a/pkg/services/librarypanels/librarypanels_test.go b/pkg/services/librarypanels/librarypanels_test.go index 3ff533995e6..a5389988491 100644 --- a/pkg/services/librarypanels/librarypanels_test.go +++ b/pkg/services/librarypanels/librarypanels_test.go @@ -753,7 +753,7 @@ func createFolder(t *testing.T, sc scenarioContext, title string) *folder.Folder dashboardStore, err := database.ProvideDashboardStore(sc.replStore, cfg, features, tagimpl.ProvideService(sc.sqlStore), quotaService) require.NoError(t, err) folderStore := folderimpl.ProvideDashboardFolderStore(sc.sqlStore) - s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sc.sqlStore, features, supportbundlestest.NewFakeBundleService(), nil) + s := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sc.sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) t.Logf("Creating folder with title and UID %q", title) ctx := identity.WithRequester(context.Background(), sc.user) @@ -836,7 +836,7 @@ func testScenario(t *testing.T, desc string, fn func(t *testing.T, sc scenarioCo dashboardStore, err := database.ProvideDashboardStore(replStore, cfg, featuremgmt.WithFeatures(), tagimpl.ProvideService(sqlStore), quotaService) require.NoError(t, err) features := featuremgmt.WithFeatures() - folderService := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil) + folderService := folderimpl.ProvideService(ac, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) elementService := libraryelements.ProvideService(cfg, sqlStore, routing.NewRouteRegister(), folderService, featuremgmt.WithFeatures(), ac) service := LibraryPanelService{ diff --git a/pkg/services/ngalert/api/api_provisioning_test.go b/pkg/services/ngalert/api/api_provisioning_test.go index 5bc102cd90a..de7b337353d 100644 --- a/pkg/services/ngalert/api/api_provisioning_test.go +++ b/pkg/services/ngalert/api/api_provisioning_test.go @@ -1819,7 +1819,7 @@ func createTestEnv(t *testing.T, testConfig string) testEnvironment { require.NoError(t, err) folderStore := folderimpl.ProvideDashboardFolderStore(sqlStore) - folderService := folderimpl.ProvideService(actest.FakeAccessControl{ExpectedEvaluate: true}, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil) + folderService := folderimpl.ProvideService(actest.FakeAccessControl{ExpectedEvaluate: true}, bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardStore, folderStore, sqlStore, featuremgmt.WithFeatures(), supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) store := store.DBstore{ Logger: log, SQLStore: sqlStore, diff --git a/pkg/services/ngalert/api/persist.go b/pkg/services/ngalert/api/persist.go index 28c984dd22b..faecfa2ccc4 100644 --- a/pkg/services/ngalert/api/persist.go +++ b/pkg/services/ngalert/api/persist.go @@ -26,8 +26,8 @@ type RuleStore interface { UpdateAlertRules(ctx context.Context, rule []ngmodels.UpdateRule) error DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error - // IncreaseVersionForAllRulesInNamespace Increases version for all rules that have specified namespace. Returns all rules that belong to the namespace - IncreaseVersionForAllRulesInNamespace(ctx context.Context, orgID int64, namespaceUID string) ([]ngmodels.AlertRuleKeyWithVersion, error) + // IncreaseVersionForAllRulesInNamespaces Increases version for all rules that have specified namespace uids + IncreaseVersionForAllRulesInNamespaces(ctx context.Context, orgID int64, namespaceUIDs []string) ([]ngmodels.AlertRuleKeyWithVersion, error) accesscontrol.RuleUIDToNamespaceStore } diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index b85aa73a871..e315e302fcb 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -487,15 +487,16 @@ func (ng *AlertNG) init() error { } func subscribeToFolderChanges(logger log.Logger, bus bus.Bus, dbStore api.RuleStore) { - // if folder title is changed, we update all alert rules in that folder to make sure that all peers (in HA mode) will update folder title and + // if full path to the folder is changed, we update all alert rules in that folder to make sure that all peers (in HA mode) will update folder title and // clean up the current state - bus.AddEventListener(func(ctx context.Context, evt *events.FolderTitleUpdated) error { - logger.Info("Got folder title updated event. updating rules in the folder", "folderUID", evt.UID) - _, err := dbStore.IncreaseVersionForAllRulesInNamespace(ctx, evt.OrgID, evt.UID) + bus.AddEventListener(func(ctx context.Context, evt *events.FolderFullPathUpdated) error { + logger.Info("Got folder full path updated event. updating rules in the folders", "folderUIDs", evt.UIDs) + updatedKeys, err := dbStore.IncreaseVersionForAllRulesInNamespaces(ctx, evt.OrgID, evt.UIDs) if err != nil { - logger.Error("Failed to update alert rules in the folder after its title was changed", "error", err, "folderUID", evt.UID, "folder", evt.Title) + logger.Error("Failed to update alert rules in the folders after their full paths were changed", "error", err, "folderUIDs", evt.UIDs, "orgID", evt.OrgID) return err } + logger.Info("Updated version for alert rules", "keys", updatedKeys) return nil }) } diff --git a/pkg/services/ngalert/ngalert_test.go b/pkg/services/ngalert/ngalert_test.go index b64dbb5092d..82542eeba71 100644 --- a/pkg/services/ngalert/ngalert_test.go +++ b/pkg/services/ngalert/ngalert_test.go @@ -9,6 +9,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/testutil" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/bus" @@ -25,37 +26,52 @@ import ( ) func Test_subscribeToFolderChanges(t *testing.T) { + getRecordedCommand := func(ruleStore *fakes.RuleStore) []fakes.GenericRecordedQuery { + results := ruleStore.GetRecordedCommands(func(cmd any) (any, bool) { + c, ok := cmd.(fakes.GenericRecordedQuery) + if !ok || c.Name != "IncreaseVersionForAllRulesInNamespaces" { + return nil, false + } + return c, ok + }) + var result []fakes.GenericRecordedQuery + for _, cmd := range results { + result = append(result, cmd.(fakes.GenericRecordedQuery)) + } + return result + } + orgID := rand.Int63() - folder := &folder.Folder{ + folder1 := &folder.Folder{ + UID: util.GenerateShortUID(), + Title: "Folder" + util.GenerateShortUID(), + } + folder2 := &folder.Folder{ UID: util.GenerateShortUID(), Title: "Folder" + util.GenerateShortUID(), } gen := models.RuleGen - rules := gen.With(gen.WithOrgID(orgID), gen.WithNamespace(folder)).GenerateManyRef(5) + rules := gen.With(gen.WithOrgID(orgID), gen.WithNamespace(folder1)).GenerateManyRef(5) bus := bus.ProvideBus(tracing.InitializeTracerForTest()) db := fakes.NewRuleStore(t) - db.Folders[orgID] = append(db.Folders[orgID], folder) + db.Folders[orgID] = append(db.Folders[orgID], folder1) db.PutRule(context.Background(), rules...) subscribeToFolderChanges(log.New("test"), bus, db) - err := bus.Publish(context.Background(), &events.FolderTitleUpdated{ + err := bus.Publish(context.Background(), &events.FolderFullPathUpdated{ Timestamp: time.Now(), - Title: "Folder" + util.GenerateShortUID(), - UID: folder.UID, + UIDs: []string{folder1.UID, folder2.UID}, OrgID: orgID, }) require.NoError(t, err) - require.Eventuallyf(t, func() bool { - return len(db.GetRecordedCommands(func(cmd any) (any, bool) { - c, ok := cmd.(fakes.GenericRecordedQuery) - if !ok || c.Name != "IncreaseVersionForAllRulesInNamespace" { - return nil, false - } - return c, true - })) > 0 + require.EventuallyWithT(t, func(c *assert.CollectT) { + recordedCommands := getRecordedCommand(db) + require.Len(c, recordedCommands, 1) + require.Equal(c, recordedCommands[0].Params[0].(int64), orgID) + require.ElementsMatch(c, recordedCommands[0].Params[1].([]string), []string{folder1.UID, folder2.UID}) }, time.Second, 10*time.Millisecond, "expected to call db store method but nothing was called") } diff --git a/pkg/services/ngalert/provisioning/alert_rules_test.go b/pkg/services/ngalert/provisioning/alert_rules_test.go index 88e95d24c37..5728b976230 100644 --- a/pkg/services/ngalert/provisioning/alert_rules_test.go +++ b/pkg/services/ngalert/provisioning/alert_rules_test.go @@ -1479,7 +1479,7 @@ func TestProvisiongWithFullpath(t *testing.T) { _, dashboardStore := testutil.SetupDashboardService(t, sqlStore, folderStore, cfg) ac := acmock.New() features := featuremgmt.WithFeatures(featuremgmt.FlagNestedFolders) - folderService := folderimpl.ProvideService(ac, inProcBus, dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil) + folderService := folderimpl.ProvideService(ac, inProcBus, dashboardStore, folderStore, sqlStore, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) ruleService := createAlertRuleService(t, folderService) var orgID int64 = 1 diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index 6198f2f5e77..547b9174886 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -75,16 +75,26 @@ func (st DBstore) DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUI }) } -// IncreaseVersionForAllRulesInNamespace Increases version for all rules that have specified namespace. Returns all rules that belong to the namespace -func (st DBstore) IncreaseVersionForAllRulesInNamespace(ctx context.Context, orgID int64, namespaceUID string) ([]ngmodels.AlertRuleKeyWithVersion, error) { +// IncreaseVersionForAllRulesInNamespaces Increases version for all rules that have specified namespace. Returns all rules that belong to the namespaces +func (st DBstore) IncreaseVersionForAllRulesInNamespaces(ctx context.Context, orgID int64, namespaceUIDs []string) ([]ngmodels.AlertRuleKeyWithVersion, error) { var keys []ngmodels.AlertRuleKeyWithVersion err := st.SQLStore.WithTransactionalDbSession(ctx, func(sess *db.Session) error { now := TimeNow() - _, err := sess.Exec("UPDATE alert_rule SET version = version + 1, updated = ? WHERE namespace_uid = ? AND org_id = ?", now, namespaceUID, orgID) + namespaceUIDsArgs, in := getINSubQueryArgs(namespaceUIDs) + sql := fmt.Sprintf( + "UPDATE alert_rule SET version = version + 1, updated = ? WHERE org_id = ? AND namespace_uid IN (%s)", + strings.Join(in, ","), + ) + args := make([]interface{}, 0, 3+len(namespaceUIDsArgs)) + args = append(args, sql, now, orgID) + args = append(args, namespaceUIDsArgs...) + + _, err := sess.Exec(args...) if err != nil { return err } - return sess.Table(ngmodels.AlertRule{}).Where("namespace_uid = ? AND org_id = ?", namespaceUID, orgID).Find(&keys) + + return sess.Table(ngmodels.AlertRule{}).Where("org_id = ?", orgID).In("namespace_uid", namespaceUIDs).Find(&keys) }) return keys, err } diff --git a/pkg/services/ngalert/store/alert_rule_test.go b/pkg/services/ngalert/store/alert_rule_test.go index a6ac6aaf0b7..0733a264221 100644 --- a/pkg/services/ngalert/store/alert_rule_test.go +++ b/pkg/services/ngalert/store/alert_rule_test.go @@ -1184,6 +1184,58 @@ func TestIntegrationRuleGroupsCaseSensitive(t *testing.T) { }) } +func TestIncreaseVersionForAllRulesInNamespaces(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + cfg := setting.NewCfg() + cfg.UnifiedAlerting = setting.UnifiedAlertingSettings{BaseInterval: time.Duration(rand.Int63n(100)+1) * time.Second} + sqlStore := db.InitTestReplDB(t) + store := &DBstore{ + SQLStore: sqlStore, + Cfg: cfg.UnifiedAlerting, + FolderService: setupFolderService(t, sqlStore, cfg, featuremgmt.WithFeatures()), + Logger: &logtest.Fake{}, + } + orgID := int64(1) + gen := models.RuleGen + gen = gen.With(gen.WithIntervalMatching(store.Cfg.BaseInterval)).With(gen.WithOrgID(orgID)) + + alertRules := []*models.AlertRule{} + for i := 0; i < 5; i++ { + alertRules = append(alertRules, createRule(t, store, gen)) + } + alertRuleNamespaceUIDs := make([]string, 0, len(alertRules)) + for _, rule := range alertRules { + alertRuleNamespaceUIDs = append(alertRuleNamespaceUIDs, rule.NamespaceUID) + } + alertRuleInAnotherNamespace := createRule(t, store, gen) + + requireAlertRuleVersion := func(t *testing.T, ruleID int64, orgID int64, expectedVersion int64) { + t.Helper() + dbrule := &models.AlertRule{} + err := sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error { + exist, err := sess.Table(models.AlertRule{}).ID(ruleID).Get(dbrule) + require.Truef(t, exist, fmt.Sprintf("rule with ID %d does not exist", ruleID)) + return err + }) + require.NoError(t, err) + require.Equal(t, expectedVersion, dbrule.Version) + } + + t.Run("should increase version for all rules", func(t *testing.T) { + _, err := store.IncreaseVersionForAllRulesInNamespaces(context.Background(), orgID, alertRuleNamespaceUIDs) + require.NoError(t, err) + + for _, rule := range alertRules { + requireAlertRuleVersion(t, rule.ID, orgID, rule.Version+1) + } + + // this rule's version should not be changed + requireAlertRuleVersion(t, alertRuleInAnotherNamespace.ID, orgID, alertRuleInAnotherNamespace.Version) + }) +} + // createAlertRule creates an alert rule in the database and returns it. // If a generator is not specified, uniqueness of primary key is not guaranteed. func createRule(t *testing.T, store *DBstore, generator *models.AlertRuleGenerator) *models.AlertRule { diff --git a/pkg/services/ngalert/tests/fakes/rules.go b/pkg/services/ngalert/tests/fakes/rules.go index 61eb036a1d4..014de832bfe 100644 --- a/pkg/services/ngalert/tests/fakes/rules.go +++ b/pkg/services/ngalert/tests/fakes/rules.go @@ -315,19 +315,24 @@ func (f *RuleStore) UpdateRuleGroup(ctx context.Context, orgID int64, namespaceU return nil } -func (f *RuleStore) IncreaseVersionForAllRulesInNamespace(_ context.Context, orgID int64, namespaceUID string) ([]models.AlertRuleKeyWithVersion, error) { +func (f *RuleStore) IncreaseVersionForAllRulesInNamespaces(_ context.Context, orgID int64, namespaceUIDs []string) ([]models.AlertRuleKeyWithVersion, error) { f.mtx.Lock() defer f.mtx.Unlock() f.RecordedOps = append(f.RecordedOps, GenericRecordedQuery{ - Name: "IncreaseVersionForAllRulesInNamespace", - Params: []any{orgID, namespaceUID}, + Name: "IncreaseVersionForAllRulesInNamespaces", + Params: []any{orgID, namespaceUIDs}, }) var result []models.AlertRuleKeyWithVersion + namespaceUIDsMap := make(map[string]struct{}, len(namespaceUIDs)) + for _, namespaceUID := range namespaceUIDs { + namespaceUIDsMap[namespaceUID] = struct{}{} + } + for _, rule := range f.Rules[orgID] { - if rule.NamespaceUID == namespaceUID && rule.OrgID == orgID { + if _, ok := namespaceUIDsMap[rule.NamespaceUID]; ok && rule.OrgID == orgID { rule.Version++ rule.Updated = time.Now() result = append(result, models.AlertRuleKeyWithVersion{ diff --git a/pkg/services/ngalert/testutil/testutil.go b/pkg/services/ngalert/testutil/testutil.go index 1f8e032617c..a1986de8734 100644 --- a/pkg/services/ngalert/testutil/testutil.go +++ b/pkg/services/ngalert/testutil/testutil.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/db" + "github.com/grafana/grafana/pkg/infra/tracing" "github.com/grafana/grafana/pkg/services/accesscontrol" acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/dashboards" @@ -26,7 +27,7 @@ import ( func SetupFolderService(tb testing.TB, cfg *setting.Cfg, db db.DB, dashboardStore dashboards.Store, folderStore *folderimpl.DashboardFolderStoreImpl, bus *bus.InProcBus, features featuremgmt.FeatureToggles, ac accesscontrol.AccessControl) folder.Service { tb.Helper() - return folderimpl.ProvideService(ac, bus, dashboardStore, folderStore, db, features, supportbundlestest.NewFakeBundleService(), nil) + return folderimpl.ProvideService(ac, bus, dashboardStore, folderStore, db, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) } func SetupDashboardService(tb testing.TB, sqlStore db.ReplDB, fs *folderimpl.DashboardFolderStoreImpl, cfg *setting.Cfg) (*dashboardservice.DashboardServiceImpl, dashboards.Store) { diff --git a/pkg/services/sqlstore/permissions/dashboard_test.go b/pkg/services/sqlstore/permissions/dashboard_test.go index a9c29d04e64..1acf17fce41 100644 --- a/pkg/services/sqlstore/permissions/dashboard_test.go +++ b/pkg/services/sqlstore/permissions/dashboard_test.go @@ -822,7 +822,7 @@ func setupNestedTest(t *testing.T, usr *user.SignedInUser, perms []accesscontrol dashStore, err := database.ProvideDashboardStore(db, cfg, features, tagimpl.ProvideService(db), quotatest.New(false, nil)) require.NoError(t, err) - folderSvc := folderimpl.ProvideService(actest.FakeAccessControl{ExpectedEvaluate: true}, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, folderimpl.ProvideDashboardFolderStore(db), db, features, supportbundlestest.NewFakeBundleService(), nil) + folderSvc := folderimpl.ProvideService(actest.FakeAccessControl{ExpectedEvaluate: true}, bus.ProvideBus(tracing.InitializeTracerForTest()), dashStore, folderimpl.ProvideDashboardFolderStore(db), db, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) // create parent folder parent, err := folderSvc.Create(context.Background(), &folder.CreateFolderCommand{ diff --git a/pkg/services/sqlstore/permissions/dashboards_bench_test.go b/pkg/services/sqlstore/permissions/dashboards_bench_test.go index 362905c447e..dfb2fe43b81 100644 --- a/pkg/services/sqlstore/permissions/dashboards_bench_test.go +++ b/pkg/services/sqlstore/permissions/dashboards_bench_test.go @@ -81,7 +81,7 @@ func setupBenchMark(b *testing.B, usr user.SignedInUser, features featuremgmt.Fe dashboardWriteStore, err := database.ProvideDashboardStore(store, cfg, features, tagimpl.ProvideService(store), quotaService) require.NoError(b, err) - folderSvc := folderimpl.ProvideService(mock.New(), bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardWriteStore, folderimpl.ProvideDashboardFolderStore(store), store, features, supportbundlestest.NewFakeBundleService(), nil) + folderSvc := folderimpl.ProvideService(mock.New(), bus.ProvideBus(tracing.InitializeTracerForTest()), dashboardWriteStore, folderimpl.ProvideDashboardFolderStore(store), store, features, supportbundlestest.NewFakeBundleService(), nil, tracing.InitializeTracerForTest()) origNewGuardian := guardian.New guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{CanViewValue: true, CanSaveValue: true})