public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Alerting: Update scheduler to receive rule updates only from database (#64662)

Browse Source 
Alerting: Update scheduler to get updates only from database (#64635)

* stop using the scheduler's Update and Delete methods all communication must be via the database
* update scheduler's registry to calculate diff before re-setting the cache
* update fetcher to return the diff generated by registry
* update processTick to update rule eval routine if the rule was updated and it is not going to be evaluated at this tick.
* remove references to the scheduler from api package
* remove unused methods in the scheduler

(cherry picked from commit 85a954cd81)

# Conflicts:
#	pkg/services/ngalert/api/api_ruler.go
#	pkg/services/ngalert/ngalert.go
#	pkg/services/ngalert/ngalert_test.go
#	pkg/services/ngalert/schedule/schedule.go
#	pkg/services/ngalert/schedule/schedule_mock.go
#	pkg/services/ngalert/schedule/schedule_unit_test.go
This commit is contained in:
Yuri Tseretyan
2023-03-14 18:51:26 -04:00
committed by GitHub
parent f854430ca6
commit 2b00a15709
11 changed files with 191 additions and 290 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/ngalert/api/api_ruler_test.go
+24 -74
View File
@@ -22,7 +22,6 @@ import (
apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/ngalert/provisioning"
"github.com/grafana/grafana/pkg/services/ngalert/schedule"
"github.com/grafana/grafana/pkg/services/ngalert/store"
"github.com/grafana/grafana/pkg/services/ngalert/tests/fakes"
"github.com/grafana/grafana/pkg/services/org"
@@ -47,7 +46,7 @@ func TestRouteDeleteAlertRules(t *testing.T) {
return result
}
assertRulesDeleted := func(t *testing.T, expectedRules []*models.AlertRule, ruleStore *fakes.RuleStore, scheduler *schedule.FakeScheduleService) {
assertRulesDeleted := func(t *testing.T, expectedRules []*models.AlertRule, ruleStore *fakes.RuleStore) {
deleteCommands := getRecordedCommand(ruleStore)
require.Len(t, deleteCommands, 1)
cmd := deleteCommands[0]
@@ -56,20 +55,6 @@ func TestRouteDeleteAlertRules(t *testing.T) {
for _, rule := range expectedRules {
require.Containsf(t, actualUIDs, rule.UID, "Rule %s was expected to be deleted but it wasn't", rule.UID)
}
notDeletedRules := make(map[models.AlertRuleKey]struct{}, len(expectedRules))
for _, rule := range expectedRules {
notDeletedRules[rule.GetKey()] = struct{}{}
}
for _, call := range scheduler.Calls {
require.Equal(t, "DeleteAlertRule", call.Method)
keys, ok := call.Arguments.Get(0).([]models.AlertRuleKey)
require.Truef(t, ok, "Expected AlertRuleKey but got something else")
for _, key := range keys {
delete(notDeletedRules, key)
}
}
require.Emptyf(t, notDeletedRules, "Not all rules were deleted")
}
orgID := rand.Int63()
@@ -89,14 +74,10 @@ func TestRouteDeleteAlertRules(t *testing.T) {
ruleStore := initFakeRuleStore(t)
ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
request := createRequestContext(orgID, org.RoleViewer, nil)
response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "")
response := createService(ac, ruleStore).RouteDeleteAlertRules(request, folder.Title, "")
require.Equalf(t, 401, response.Status(), "Expected 401 but got %d: %v", response.Status(), string(response.Body()))
scheduler.AssertNotCalled(t, "DeleteAlertRule")
require.Empty(t, getRecordedCommand(ruleStore))
})
t.Run("editor should be able to delete all non-provisioned rules in folder", func(t *testing.T) {
@@ -104,14 +85,10 @@ func TestRouteDeleteAlertRules(t *testing.T) {
rulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))
ruleStore.PutRule(context.Background(), rulesInFolder...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
request := createRequestContext(orgID, org.RoleEditor, nil)
response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "")
response := createService(ac, ruleStore).RouteDeleteAlertRules(request, folder.Title, "")
require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body()))
assertRulesDeleted(t, rulesInFolder, ruleStore, scheduler)
})
t.Run("editor should be able to delete rules group if it is not provisioned", func(t *testing.T) {
groupName := util.GenerateShortUID()
@@ -124,26 +101,19 @@ func TestRouteDeleteAlertRules(t *testing.T) {
// rules in the same group but different folder
ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withGroup(groupName)))...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything).Return()
request := createRequestContext(orgID, org.RoleEditor, nil)
response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, groupName)
response := createService(ac, ruleStore).RouteDeleteAlertRules(request, folder.Title, groupName)
require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body()))
assertRulesDeleted(t, rulesInFolderInGroup, ruleStore, scheduler)
assertRulesDeleted(t, rulesInFolderInGroup, ruleStore)
})
t.Run("should return 202 if folder is empty", func(t *testing.T) {
ruleStore := initFakeRuleStore(t)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
requestCtx := createRequestContext(orgID, org.RoleEditor, nil)
response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(requestCtx, folder.Title, "")
response := createService(ac, ruleStore).RouteDeleteAlertRules(requestCtx, folder.Title, "")
require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body()))
scheduler.AssertNotCalled(t, "DeleteAlertRule")
require.Empty(t, getRecordedCommand(ruleStore))
})
})
@@ -155,25 +125,18 @@ func TestRouteDeleteAlertRules(t *testing.T) {
ruleStore := initFakeRuleStore(t)
ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder)))...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything).Panic("should not be called")
ac := acMock.New()
request := createRequestContext(orgID, "None", nil)
response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(request, folder.Title, "")
response := createService(ac, ruleStore).RouteDeleteAlertRules(request, folder.Title, "")
require.Equalf(t, 401, response.Status(), "Expected 401 but got %d: %v", response.Status(), string(response.Body()))
scheduler.AssertNotCalled(t, "DeleteAlertRule")
require.Empty(t, getRecordedCommand(ruleStore))
})
t.Run("delete only non-provisioned groups that user is authorized", func(t *testing.T) {
ruleStore := initFakeRuleStore(t)
provisioningStore := provisioning.NewFakeProvisioningStore()
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
authorizedRulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup("authz_"+util.GenerateShortUID())))
provisionedRulesInFolder := models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup("provisioned_"+util.GenerateShortUID())))
@@ -187,10 +150,10 @@ func TestRouteDeleteAlertRules(t *testing.T) {
ac := acMock.New().WithPermissions(createPermissionsForRules(append(authorizedRulesInFolder, provisionedRulesInFolder...)))
response := createServiceWithProvenanceStore(ac, ruleStore, scheduler, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, "")
response := createServiceWithProvenanceStore(ac, ruleStore, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, "")
require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body()))
assertRulesDeleted(t, authorizedRulesInFolder, ruleStore, scheduler)
assertRulesDeleted(t, authorizedRulesInFolder, ruleStore)
})
t.Run("return 400 if all rules user can access are provisioned", func(t *testing.T) {
ruleStore := initFakeRuleStore(t)
@@ -204,15 +167,11 @@ func TestRouteDeleteAlertRules(t *testing.T) {
// more rules in the same namespace but user does not have access to them
ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(util.GenerateShortUID())))...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
ac := acMock.New().WithPermissions(createPermissionsForRules(provisionedRulesInFolder))
response := createServiceWithProvenanceStore(ac, ruleStore, scheduler, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, "")
response := createServiceWithProvenanceStore(ac, ruleStore, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, "")
require.Equalf(t, 400, response.Status(), "Expected 400 but got %d: %v", response.Status(), string(response.Body()))
scheduler.AssertNotCalled(t, "DeleteAlertRule")
require.Empty(t, getRecordedCommand(ruleStore))
})
})
@@ -226,15 +185,11 @@ func TestRouteDeleteAlertRules(t *testing.T) {
// more rules in the same group but user is not authorized to access them
ruleStore.PutRule(context.Background(), models.GenerateAlertRulesSmallNonEmpty(models.AlertRuleGen(withOrgID(orgID), withNamespace(folder), withGroup(groupName)))...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
ac := acMock.New().WithPermissions(createPermissionsForRules(authorizedRulesInGroup))
response := createService(ac, ruleStore, scheduler).RouteDeleteAlertRules(requestCtx, folder.Title, groupName)
response := createService(ac, ruleStore).RouteDeleteAlertRules(requestCtx, folder.Title, groupName)
require.Equalf(t, 401, response.Status(), "Expected 401 but got %d: %v", response.Status(), string(response.Body()))
scheduler.AssertNotCalled(t, "DeleteAlertRule", mock.Anything)
deleteCommands := getRecordedCommand(ruleStore)
require.Empty(t, deleteCommands)
})
@@ -248,15 +203,11 @@ func TestRouteDeleteAlertRules(t *testing.T) {
ruleStore.PutRule(context.Background(), provisionedRulesInFolder...)
scheduler := &schedule.FakeScheduleService{}
scheduler.On("DeleteAlertRule", mock.Anything)
ac := acMock.New().WithPermissions(createPermissionsForRules(provisionedRulesInFolder))
response := createServiceWithProvenanceStore(ac, ruleStore, scheduler, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, groupName)
response := createServiceWithProvenanceStore(ac, ruleStore, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.Title, groupName)
require.Equalf(t, 400, response.Status(), "Expected 400 but got %d: %v", response.Status(), string(response.Body()))
scheduler.AssertNotCalled(t, "DeleteAlertRule", mock.Anything)
deleteCommands := getRecordedCommand(ruleStore)
require.Empty(t, deleteCommands)
})
@@ -277,7 +228,7 @@ func TestRouteGetNamespaceRulesConfig(t *testing.T) {
ac := acMock.New().WithPermissions(createPermissionsForRules(expectedRules))
req := createRequestContext(orgID, "", nil)
response := createService(ac, ruleStore, nil).RouteGetNamespaceRulesConfig(req, folder.Title)
response := createService(ac, ruleStore).RouteGetNamespaceRulesConfig(req, folder.Title)
require.Equal(t, http.StatusAccepted, response.Status())
result := &apimodels.NamespaceConfigResponse{}
@@ -312,7 +263,7 @@ func TestRouteGetNamespaceRulesConfig(t *testing.T) {
ac := acMock.New().WithDisabled()
req := createRequestContext(orgID, org.RoleViewer, nil)
response := createService(ac, ruleStore, nil).RouteGetNamespaceRulesConfig(req, folder.Title)
response := createService(ac, ruleStore).RouteGetNamespaceRulesConfig(req, folder.Title)
require.Equal(t, http.StatusAccepted, response.Status())
result := &apimodels.NamespaceConfigResponse{}
@@ -345,7 +296,7 @@ func TestRouteGetNamespaceRulesConfig(t *testing.T) {
ruleStore.PutRule(context.Background(), expectedRules...)
ac := acMock.New().WithDisabled()
svc := createService(ac, ruleStore, nil)
svc := createService(ac, ruleStore)
// add provenance to the first generated rule
rule := &models.AlertRule{
@@ -389,7 +340,7 @@ func TestRouteGetNamespaceRulesConfig(t *testing.T) {
ruleStore.PutRule(context.Background(), expectedRules...)
ac := acMock.New().WithDisabled()
response := createService(ac, ruleStore, nil).RouteGetNamespaceRulesConfig(createRequestContext(orgID, org.RoleViewer, nil), folder.Title)
response := createService(ac, ruleStore).RouteGetNamespaceRulesConfig(createRequestContext(orgID, org.RoleViewer, nil), folder.Title)
require.Equal(t, http.StatusAccepted, response.Status())
result := &apimodels.NamespaceConfigResponse{}
@@ -441,7 +392,7 @@ func TestRouteGetRulesConfig(t *testing.T) {
request := createRequestContext(orgID, "", nil)
t.Run("and do not return group if user does not have access to one of rules", func(t *testing.T) {
ac := acMock.New().WithPermissions(createPermissionsForRules(append(group1, group2[1:]...)))
response := createService(ac, ruleStore, nil).RouteGetRulesConfig(request)
response := createService(ac, ruleStore).RouteGetRulesConfig(request)
require.Equal(t, http.StatusOK, response.Status())
result := &apimodels.NamespaceConfigResponse{}
@@ -471,7 +422,7 @@ func TestRouteGetRulesConfig(t *testing.T) {
ruleStore.PutRule(context.Background(), expectedRules...)
ac := acMock.New().WithDisabled()
response := createService(ac, ruleStore, nil).RouteGetRulesConfig(createRequestContext(orgID, org.RoleViewer, nil))
response := createService(ac, ruleStore).RouteGetRulesConfig(createRequestContext(orgID, org.RoleViewer, nil))
require.Equal(t, http.StatusOK, response.Status())
result := &apimodels.NamespaceConfigResponse{}
@@ -522,13 +473,13 @@ func TestRouteGetRulesGroupConfig(t *testing.T) {
t.Run("and return 401 if user does not have access one of rules", func(t *testing.T) {
ac := acMock.New().WithPermissions(createPermissionsForRules(expectedRules[1:]))
response := createService(ac, ruleStore, nil).RouteGetRulesGroupConfig(request, folder.Title, groupKey.RuleGroup)
response := createService(ac, ruleStore).RouteGetRulesGroupConfig(request, folder.Title, groupKey.RuleGroup)
require.Equal(t, http.StatusUnauthorized, response.Status())
})
t.Run("and return rules if user has access to all of them", func(t *testing.T) {
ac := acMock.New().WithPermissions(createPermissionsForRules(expectedRules))
response := createService(ac, ruleStore, nil).RouteGetRulesGroupConfig(request, folder.Title, groupKey.RuleGroup)
response := createService(ac, ruleStore).RouteGetRulesGroupConfig(request, folder.Title, groupKey.RuleGroup)
require.Equal(t, http.StatusAccepted, response.Status())
result := &apimodels.RuleGroupConfigResponse{}
@@ -551,7 +502,7 @@ func TestRouteGetRulesGroupConfig(t *testing.T) {
ruleStore.PutRule(context.Background(), expectedRules...)
ac := acMock.New().WithDisabled()
response := createService(ac, ruleStore, nil).RouteGetRulesGroupConfig(createRequestContext(orgID, org.RoleViewer, nil), folder.Title, groupKey.RuleGroup)
response := createService(ac, ruleStore).RouteGetRulesGroupConfig(createRequestContext(orgID, org.RoleViewer, nil), folder.Title, groupKey.RuleGroup)
require.Equal(t, http.StatusAccepted, response.Status())
result := &apimodels.RuleGroupConfigResponse{}
@@ -638,19 +589,18 @@ func TestVerifyProvisionedRulesNotAffected(t *testing.T) {
})
}
func createServiceWithProvenanceStore(ac *acMock.Mock, store *fakes.RuleStore, scheduler schedule.ScheduleService, provenanceStore provisioning.ProvisioningStore) *RulerSrv {
svc := createService(ac, store, scheduler)
func createServiceWithProvenanceStore(ac *acMock.Mock, store *fakes.RuleStore, provenanceStore provisioning.ProvisioningStore) *RulerSrv {
svc := createService(ac, store)
svc.provenanceStore = provenanceStore
return svc
}
func createService(ac *acMock.Mock, store *fakes.RuleStore, scheduler schedule.ScheduleService) *RulerSrv {
func createService(ac *acMock.Mock, store *fakes.RuleStore) *RulerSrv {
return &RulerSrv{
xactManager: store,
store: store,
QuotaService: nil,
provenanceStore: provisioning.NewFakeProvisioningStore(),
scheduleService: scheduler,
log: log.New("test"),
cfg: nil,
ac: ac,