From 92716cb60297e11be5a089f38dbc486f960fdd25 Mon Sep 17 00:00:00 2001 From: Alexander Weaver Date: Tue, 15 Mar 2022 11:48:42 -0500 Subject: [PATCH] Alerting: Create abstraction for launching transactions and refactor existing transaction management to use it (#46216) * Remove InTransaction from RuleStore and make it its own interface * Ensure that ctx-based is clear from name * Resolve merge conflicts * Refactor tests to work in terms of the introduced abstraction rather than concrete dbstore --- pkg/services/ngalert/api/api.go | 9 ++- pkg/services/ngalert/api/api_ruler.go | 3 +- pkg/services/ngalert/ngalert.go | 1 + pkg/services/ngalert/store/alert_rule.go | 5 -- .../ngalert/store/provisioning_store_test.go | 60 +++++++++++++++---- pkg/services/ngalert/store/transactions.go | 12 ++++ 6 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 pkg/services/ngalert/store/transactions.go diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index b80f4f469b9..8910250bf9c 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -63,6 +63,7 @@ type API struct { ExpressionService *expr.Service QuotaService *quota.QuotaService Schedule schedule.ScheduleService + TransactionManager store.TransactionManager RuleStore store.RuleStore InstanceStore store.InstanceStore AlertingStore AlertingStore @@ -97,7 +98,13 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) { api.RegisterRulerApiEndpoints(NewForkedRuler( api.DatasourceCache, NewLotexRuler(proxy, logger), - &RulerSrv{DatasourceCache: api.DatasourceCache, QuotaService: api.QuotaService, scheduleService: api.Schedule, store: api.RuleStore, log: logger, cfg: &api.Cfg.UnifiedAlerting}, + &RulerSrv{ + DatasourceCache: api.DatasourceCache, + QuotaService: api.QuotaService, + scheduleService: api.Schedule, + store: api.RuleStore, + xactManager: api.TransactionManager, + log: logger, cfg: &api.Cfg.UnifiedAlerting}, ), m) api.RegisterTestingApiEndpoints(NewForkedTestingApi( &TestingApiSrv{ diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index babcd656ad8..b449e729480 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -27,6 +27,7 @@ import ( ) type RulerSrv struct { + xactManager store.TransactionManager store store.RuleStore DatasourceCache datasources.CacheService QuotaService *quota.QuotaService @@ -263,7 +264,7 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *mod // TODO add create rules authz logic var groupChanges *changes = nil - err := srv.store.InTransaction(c.Req.Context(), func(tranCtx context.Context) error { + err := srv.xactManager.InTransaction(c.Req.Context(), func(tranCtx context.Context) error { var err error groupChanges, err = calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules) if err != nil { diff --git a/pkg/services/ngalert/ngalert.go b/pkg/services/ngalert/ngalert.go index 156ab04e41d..237e51d8cee 100644 --- a/pkg/services/ngalert/ngalert.go +++ b/pkg/services/ngalert/ngalert.go @@ -144,6 +144,7 @@ func (ng *AlertNG) init() error { DataProxy: ng.DataProxy, QuotaService: ng.QuotaService, SecretsService: ng.SecretsService, + TransactionManager: store, InstanceStore: store, RuleStore: store, AlertingStore: store, diff --git a/pkg/services/ngalert/store/alert_rule.go b/pkg/services/ngalert/store/alert_rule.go index c3d2fe61cff..fdfeac0d80e 100644 --- a/pkg/services/ngalert/store/alert_rule.go +++ b/pkg/services/ngalert/store/alert_rule.go @@ -48,7 +48,6 @@ type RuleStore interface { GetNamespaceByTitle(context.Context, string, int64, *models.SignedInUser, bool) (*models.Folder, error) GetOrgRuleGroups(ctx context.Context, query *ngmodels.ListOrgRuleGroupsQuery) error UpsertAlertRules(ctx context.Context, rule []UpsertRule) error - InTransaction(ctx context.Context, f func(ctx context.Context) error) error } func getAlertRuleByUID(sess *sqlstore.DBSession, alertRuleUID string, orgID int64) (*ngmodels.AlertRule, error) { @@ -502,7 +501,3 @@ WHERE org_id = ?` return nil }) } - -func (st *DBstore) InTransaction(ctx context.Context, f func(ctx context.Context) error) error { - return st.SQLStore.InTransaction(ctx, f) -} diff --git a/pkg/services/ngalert/store/provisioning_store_test.go b/pkg/services/ngalert/store/provisioning_store_test.go index 668a4b60630..4e9770c2d0b 100644 --- a/pkg/services/ngalert/store/provisioning_store_test.go +++ b/pkg/services/ngalert/store/provisioning_store_test.go @@ -2,9 +2,12 @@ package store_test import ( "context" + "fmt" "testing" + "github.com/grafana/grafana/pkg/services/ngalert" "github.com/grafana/grafana/pkg/services/ngalert/models" + "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/ngalert/tests" "github.com/stretchr/testify/require" ) @@ -12,14 +15,14 @@ import ( const testAlertingIntervalSeconds = 10 func TestProvisioningStore(t *testing.T) { - _, dbstore := tests.SetupTestEnv(t, testAlertingIntervalSeconds) + store, xact := createSut(tests.SetupTestEnv(t, testAlertingIntervalSeconds)) t.Run("Default provenance of a known type is None", func(t *testing.T) { rule := models.AlertRule{ UID: "asdf", } - provenance, err := dbstore.GetProvenance(context.Background(), &rule) + provenance, err := store.GetProvenance(context.Background(), &rule) require.NoError(t, err) require.Equal(t, models.ProvenanceNone, provenance) @@ -29,10 +32,10 @@ func TestProvisioningStore(t *testing.T) { rule := models.AlertRule{ UID: "123", } - err := dbstore.SetProvenance(context.Background(), &rule, models.ProvenanceFile) + err := store.SetProvenance(context.Background(), &rule, models.ProvenanceFile) require.NoError(t, err) - p, err := dbstore.GetProvenance(context.Background(), &rule) + p, err := store.GetProvenance(context.Background(), &rule) require.NoError(t, err) require.Equal(t, models.ProvenanceFile, p) @@ -47,10 +50,10 @@ func TestProvisioningStore(t *testing.T) { UID: "456", OrgID: 3, } - err := dbstore.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceFile) + err := store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceFile) require.NoError(t, err) - p, err := dbstore.GetProvenance(context.Background(), &ruleOrg3) + p, err := store.GetProvenance(context.Background(), &ruleOrg3) require.NoError(t, err) require.Equal(t, models.ProvenanceNone, p) @@ -65,19 +68,54 @@ func TestProvisioningStore(t *testing.T) { UID: "789", OrgID: 3, } - err := dbstore.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceFile) + err := store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceFile) require.NoError(t, err) - err = dbstore.SetProvenance(context.Background(), &ruleOrg3, models.ProvenanceFile) + err = store.SetProvenance(context.Background(), &ruleOrg3, models.ProvenanceFile) require.NoError(t, err) - err = dbstore.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceApi) + err = store.SetProvenance(context.Background(), &ruleOrg2, models.ProvenanceApi) require.NoError(t, err) - p, err := dbstore.GetProvenance(context.Background(), &ruleOrg2) + p, err := store.GetProvenance(context.Background(), &ruleOrg2) require.NoError(t, err) require.Equal(t, models.ProvenanceApi, p) - p, err = dbstore.GetProvenance(context.Background(), &ruleOrg3) + p, err = store.GetProvenance(context.Background(), &ruleOrg3) require.NoError(t, err) require.Equal(t, models.ProvenanceFile, p) }) + + t.Run("Store saves provenance type when contextual transaction is applied", func(t *testing.T) { + rule := models.AlertRule{ + UID: "456", + } + + err := xact.InTransaction(context.Background(), func(ctx context.Context) error { + return store.SetProvenance(ctx, &rule, models.ProvenanceFile) + }) + require.NoError(t, err) + + provenance, err := store.GetProvenance(context.Background(), &rule) + require.NoError(t, err) + require.Equal(t, models.ProvenanceFile, provenance) + }) + + t.Run("Contextual transaction which errors before saving rolls back type update", func(t *testing.T) { + rule := models.AlertRule{ + UID: "789", + } + + _ = xact.InTransaction(context.Background(), func(ctx context.Context) error { + err := store.SetProvenance(ctx, &rule, models.ProvenanceFile) + require.NoError(t, err) + return fmt.Errorf("something happened!") + }) + + provenance, err := store.GetProvenance(context.Background(), &rule) + require.NoError(t, err) + require.Equal(t, models.ProvenanceNone, provenance) + }) +} + +func createSut(_ *ngalert.AlertNG, db *store.DBstore) (store.ProvisioningStore, store.TransactionManager) { + return db, db } diff --git a/pkg/services/ngalert/store/transactions.go b/pkg/services/ngalert/store/transactions.go new file mode 100644 index 00000000000..a29223727c1 --- /dev/null +++ b/pkg/services/ngalert/store/transactions.go @@ -0,0 +1,12 @@ +package store + +import "context" + +// TransactionManager represents the ability to issue and close transactions through contexts. +type TransactionManager interface { + InTransaction(ctx context.Context, work func(ctx context.Context) error) error +} + +func (st *DBstore) InTransaction(ctx context.Context, f func(ctx context.Context) error) error { + return st.SQLStore.InTransaction(ctx, f) +}