From d17ab82b98ab30a6c0203db15984591fb9df9493 Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Tue, 27 Sep 2022 08:56:30 -0500 Subject: [PATCH] Alerting: Break up store.RuleStore interface, delete dead code (#55776) * Refactor state manager to not depend on rule store interface * Refactor grafana and proxied ruler APIs to not depend on store.RuleStore * Refactor folder subscription logic to not use store.RuleStore * Delete dead code * Delete store.RuleStore --- pkg/services/ngalert/api/api.go | 2 +- pkg/services/ngalert/api/api_prometheus.go | 3 +- pkg/services/ngalert/api/api_ruler.go | 2 +- pkg/services/ngalert/api/persist.go | 27 +++++++++++++ pkg/services/ngalert/models/alert_rule.go | 6 --- pkg/services/ngalert/ngalert.go | 2 +- pkg/services/ngalert/state/manager.go | 5 +-- pkg/services/ngalert/state/persist.go | 6 +++ pkg/services/ngalert/store/alert_rule.go | 44 ---------------------- pkg/services/ngalert/store/testing.go | 22 ----------- 10 files changed, 39 insertions(+), 80 deletions(-) create mode 100644 pkg/services/ngalert/api/persist.go diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 5dacbc8af36..eb16162cc57 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -68,7 +68,7 @@ type API struct { Schedule schedule.ScheduleService TransactionManager provisioning.TransactionManager ProvenanceStore provisioning.ProvisioningStore - RuleStore store.RuleStore + RuleStore RuleStore AlertingStore AlertingStore AdminConfigStore store.AdminConfigurationStore DataProxy *datasourceproxy.DataSourceProxyService diff --git a/pkg/services/ngalert/api/api_prometheus.go b/pkg/services/ngalert/api/api_prometheus.go index ea52787d8d3..e1c4f9c1ebe 100644 --- a/pkg/services/ngalert/api/api_prometheus.go +++ b/pkg/services/ngalert/api/api_prometheus.go @@ -18,7 +18,6 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/eval" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/state" - "github.com/grafana/grafana/pkg/services/ngalert/store" apiv1 "github.com/prometheus/client_golang/api/prometheus/v1" ) @@ -26,7 +25,7 @@ import ( type PrometheusSrv struct { log log.Logger manager state.AlertInstanceManager - store store.RuleStore + store RuleStore ac accesscontrol.AccessControl } diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 5562f13eff0..83b87cec1ef 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -36,7 +36,7 @@ type ConditionValidator interface { type RulerSrv struct { xactManager provisioning.TransactionManager provenanceStore provisioning.ProvisioningStore - store store.RuleStore + store RuleStore QuotaService quota.Service scheduleService schedule.ScheduleService log log.Logger diff --git a/pkg/services/ngalert/api/persist.go b/pkg/services/ngalert/api/persist.go new file mode 100644 index 00000000000..e143d705195 --- /dev/null +++ b/pkg/services/ngalert/api/persist.go @@ -0,0 +1,27 @@ +package api + +import ( + "context" + + "github.com/grafana/grafana/pkg/models" + ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/store" + "github.com/grafana/grafana/pkg/services/user" +) + +// RuleStore is the interface for persisting alert rules and instances +type RuleStore interface { + GetUserVisibleNamespaces(context.Context, int64, *user.SignedInUser) (map[string]*models.Folder, error) + GetNamespaceByTitle(context.Context, string, int64, *user.SignedInUser, bool) (*models.Folder, error) + GetAlertRulesGroupByRuleUID(ctx context.Context, query *ngmodels.GetAlertRulesGroupByRuleUIDQuery) error + ListAlertRules(ctx context.Context, query *ngmodels.ListAlertRulesQuery) error + + // InsertAlertRules will insert all alert rules passed into the function + // and return the map of uuid to id. + InsertAlertRules(ctx context.Context, rule []ngmodels.AlertRule) (map[string]int64, error) + UpdateAlertRules(ctx context.Context, rule []store.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) +} diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 163503cc048..d1be2d9aee0 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -365,12 +365,6 @@ type ListNamespaceAlertRulesQuery struct { Result []*AlertRule } -// ListRuleGroupsQuery is the query for listing unique rule groups -// across all organizations -type ListRuleGroupsQuery struct { - Result []string -} - // ListOrgRuleGroupsQuery is the query for listing unique rule groups // for an organization type ListOrgRuleGroupsQuery struct { diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index 53ad837a85f..c045f441322 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -216,7 +216,7 @@ func (ng *AlertNG) init() error { return DeclareFixedRoles(ng.accesscontrolService) } -func subscribeToFolderChanges(logger log.Logger, bus bus.Bus, dbStore store.RuleStore, scheduler schedule.ScheduleService) { +func subscribeToFolderChanges(logger log.Logger, bus bus.Bus, dbStore api.RuleStore, scheduler schedule.ScheduleService) { // 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 // clean up the current state bus.AddEventListener(func(ctx context.Context, e *events.FolderTitleUpdated) error { diff --git a/pkg/services/ngalert/state/manager.go b/pkg/services/ngalert/state/manager.go index 6d44fd35857..02857ecbc41 100644 --- a/pkg/services/ngalert/state/manager.go +++ b/pkg/services/ngalert/state/manager.go @@ -20,7 +20,6 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/image" "github.com/grafana/grafana/pkg/services/ngalert/metrics" ngModels "github.com/grafana/grafana/pkg/services/ngalert/models" - "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/screenshot" ) @@ -42,7 +41,7 @@ type Manager struct { quit chan struct{} ResendDelay time.Duration - ruleStore store.RuleStore + ruleStore RuleReader instanceStore InstanceStore dashboardService dashboards.DashboardService imageService image.ImageService @@ -50,7 +49,7 @@ type Manager struct { } func NewManager(logger log.Logger, metrics *metrics.State, externalURL *url.URL, - ruleStore store.RuleStore, instanceStore InstanceStore, + ruleStore RuleReader, instanceStore InstanceStore, dashboardService dashboards.DashboardService, imageService image.ImageService, clock clock.Clock, annotationsRepo annotations.Repository) *Manager { manager := &Manager{ cache: newCache(logger, metrics, externalURL), diff --git a/pkg/services/ngalert/state/persist.go b/pkg/services/ngalert/state/persist.go index f1c89baa977..b433aef29f8 100644 --- a/pkg/services/ngalert/state/persist.go +++ b/pkg/services/ngalert/state/persist.go @@ -6,6 +6,7 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/models" ) +// InstanceStore represents the ability to fetch and write alert instances. type InstanceStore interface { FetchOrgIds(ctx context.Context) ([]int64, error) ListAlertInstances(ctx context.Context, cmd *models.ListAlertInstancesQuery) error @@ -13,3 +14,8 @@ type InstanceStore interface { DeleteAlertInstance(ctx context.Context, orgID int64, ruleUID, labelsHash string) error DeleteAlertInstancesByRule(ctx context.Context, key models.AlertRuleKey) error } + +// RuleReader represents the ability to fetch alert rules. +type RuleReader interface { + ListAlertRules(ctx context.Context, query *models.ListAlertRulesQuery) error +} diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index f0fa1df4027..1743e0eb56b 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -38,28 +38,6 @@ var ( ErrOptimisticLock = errors.New("version conflict while updating a record in the database with optimistic locking") ) -// RuleStore is the interface for persisting alert rules and instances -type RuleStore interface { - DeleteAlertRulesByUID(ctx context.Context, orgID int64, ruleUID ...string) error - DeleteAlertInstancesByRuleUID(ctx context.Context, orgID int64, ruleUID string) error - GetAlertRuleByUID(ctx context.Context, query *ngmodels.GetAlertRuleByUIDQuery) error - GetAlertRulesGroupByRuleUID(ctx context.Context, query *ngmodels.GetAlertRulesGroupByRuleUIDQuery) error - ListAlertRules(ctx context.Context, query *ngmodels.ListAlertRulesQuery) error - // GetRuleGroups returns the unique rule groups across all organizations. - GetRuleGroups(ctx context.Context, query *ngmodels.ListRuleGroupsQuery) error - GetRuleGroupInterval(ctx context.Context, orgID int64, namespaceUID string, ruleGroup string) (int64, error) - GetUserVisibleNamespaces(context.Context, int64, *user.SignedInUser) (map[string]*models.Folder, error) - GetNamespaceByTitle(context.Context, string, int64, *user.SignedInUser, bool) (*models.Folder, error) - GetNamespaceByUID(context.Context, string, int64, *user.SignedInUser) (*models.Folder, error) - // InsertAlertRules will insert all alert rules passed into the function - // and return the map of uuid to id. - InsertAlertRules(ctx context.Context, rule []ngmodels.AlertRule) (map[string]int64, error) - UpdateAlertRules(ctx context.Context, rule []UpdateRule) 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) -} - func getAlertRuleByUID(sess *sqlstore.DBSession, alertRuleUID string, orgID int64) (*ngmodels.AlertRule, error) { // we consider optionally enabling some caching alertRule := ngmodels.AlertRule{OrgID: orgID, UID: alertRuleUID} @@ -112,17 +90,6 @@ func (st DBstore) IncreaseVersionForAllRulesInNamespace(ctx context.Context, org return keys, err } -// DeleteAlertInstancesByRuleUID is a handler for deleting alert instances by alert rule UID when a rule has been updated -func (st DBstore) DeleteAlertInstancesByRuleUID(ctx context.Context, orgID int64, ruleUID string) error { - return st.SQLStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { - _, err := sess.Exec("DELETE FROM alert_instance WHERE rule_org_id = ? AND rule_uid = ?", orgID, ruleUID) - if err != nil { - return err - } - return nil - }) -} - // GetAlertRuleByUID is a handler for retrieving an alert rule from that database by its UID and organisation ID. // It returns ngmodels.ErrAlertRuleNotFound if no alert rule is found for the provided ID. func (st DBstore) GetAlertRuleByUID(ctx context.Context, query *ngmodels.GetAlertRuleByUIDQuery) error { @@ -313,17 +280,6 @@ func (st DBstore) ListAlertRules(ctx context.Context, query *ngmodels.ListAlertR }) } -func (st DBstore) GetRuleGroups(ctx context.Context, query *ngmodels.ListRuleGroupsQuery) error { - return st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { - ruleGroups := make([]string, 0) - if err := sess.Table("alert_rule").Distinct("rule_group").Find(&ruleGroups); err != nil { - return err - } - query.Result = ruleGroups - return nil - }) -} - func (st DBstore) GetRuleGroupInterval(ctx context.Context, orgID int64, namespaceUID string, ruleGroup string) (int64, error) { var interval int64 = 0 return interval, st.SQLStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error { diff --git a/pkg/services/ngalert/store/testing.go b/pkg/services/ngalert/store/testing.go index e715c20e624..6b9bcea9650 100644 --- a/pkg/services/ngalert/store/testing.go +++ b/pkg/services/ngalert/store/testing.go @@ -171,9 +171,6 @@ func (f *FakeRuleStore) DeleteAlertRulesByUID(_ context.Context, orgID int64, UI return nil } -func (f *FakeRuleStore) DeleteAlertInstancesByRuleUID(_ context.Context, _ int64, _ string) error { - return nil -} func (f *FakeRuleStore) GetAlertRuleByUID(_ context.Context, q *models.GetAlertRuleByUIDQuery) error { f.mtx.Lock() defer f.mtx.Unlock() @@ -328,25 +325,6 @@ func (f *FakeRuleStore) ListAlertRules(_ context.Context, q *models.ListAlertRul return nil } -func (f *FakeRuleStore) GetRuleGroups(_ context.Context, q *models.ListRuleGroupsQuery) error { - f.mtx.Lock() - defer f.mtx.Unlock() - f.RecordedOps = append(f.RecordedOps, *q) - - m := make(map[string]struct{}) - for _, rules := range f.Rules { - for _, rule := range rules { - m[rule.RuleGroup] = struct{}{} - } - } - - for s := range m { - q.Result = append(q.Result, s) - } - - return nil -} - func (f *FakeRuleStore) GetUserVisibleNamespaces(_ context.Context, orgID int64, _ *user.SignedInUser) (map[string]*models2.Folder, error) { f.mtx.Lock() defer f.mtx.Unlock()