From 288e8eeb1563f8eb35430f7fdcd4ba4e9ed648cb Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Fri, 4 Mar 2022 16:16:33 -0500 Subject: [PATCH] Alerting: Do not update rule in database if it was not changed (#45980) * do not include update if no diff * refactor calculate changes to include diff (and log) Co-authored-by: George Robinson --- pkg/services/ngalert/api/api_ruler.go | 113 ++++++++++++------ pkg/services/ngalert/api/api_ruler_test.go | 84 ++++++++----- .../api/alerting/api_alertmanager_test.go | 6 +- 3 files changed, 130 insertions(+), 73 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 23bef6da32b..babcd656ad8 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/util/cmputil" "github.com/prometheus/common/model" @@ -261,27 +262,48 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *models.ReqContext, ruleGroupConf func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *models.Folder, groupName string, rules []*ngmodels.AlertRule) response.Response { // TODO add create rules authz logic - var changes *RuleChanges = nil + var groupChanges *changes = nil err := srv.store.InTransaction(c.Req.Context(), func(tranCtx context.Context) error { var err error - changes, err = calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules) + groupChanges, err = calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules) if err != nil { return err } - // TODO add update/delete authz logic - err = srv.store.UpsertAlertRules(tranCtx, changes.Upsert) - if err != nil { - return fmt.Errorf("failed to add or update rules: %w", err) + if groupChanges.isEmpty() { + srv.log.Info("no changes detected in the request. Do nothing") + return nil } - for _, rule := range changes.Delete { + if len(groupChanges.Update) > 0 || len(groupChanges.New) > 0 { + upsert := make([]store.UpsertRule, 0, len(groupChanges.Update)+len(groupChanges.New)) + for _, update := range groupChanges.Update { + srv.log.Debug("updating rule", "uid", update.New.UID, "diff", update.Diff.String()) + upsert = append(upsert, store.UpsertRule{ + Existing: update.Existing, + New: *update.New, + }) + } + for _, rule := range groupChanges.New { + upsert = append(upsert, store.UpsertRule{ + Existing: nil, + New: *rule, + }) + } + // TODO add update/delete authz logic + err = srv.store.UpsertAlertRules(tranCtx, upsert) + if err != nil { + return fmt.Errorf("failed to add or update rules: %w", err) + } + } + + for _, rule := range groupChanges.Delete { if err = srv.store.DeleteAlertRuleByUID(tranCtx, c.SignedInUser.OrgId, rule.UID); err != nil { return fmt.Errorf("failed to delete rule %d with UID %s: %w", rule.ID, rule.UID, err) } } - if changes.newRules > 0 { + if len(groupChanges.New) > 0 { limitReached, err := srv.QuotaService.CheckQuotaReached(tranCtx, "alert_rule", "a.ScopeParameters{ OrgId: c.OrgId, UserId: c.UserId, @@ -307,23 +329,24 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *mod return ErrResp(http.StatusInternalServerError, err, "failed to update rule group") } - // TODO uncomment when rules that are not changed will be filter out from the upsert list. - // for _, rule := range changes.Upsert { - // if rule.Existing != nil { - // srv.scheduleService.UpdateAlertRule(ngmodels.AlertRuleKey{ - // OrgID: c.SignedInUser.OrgId, - // UID: rule.Existing.UID, - // }) - // } - // } + for _, rule := range groupChanges.Update { + srv.scheduleService.UpdateAlertRule(ngmodels.AlertRuleKey{ + OrgID: c.SignedInUser.OrgId, + UID: rule.Existing.UID, + }) + } - for _, rule := range changes.Delete { + for _, rule := range groupChanges.Delete { srv.scheduleService.DeleteAlertRule(ngmodels.AlertRuleKey{ OrgID: c.SignedInUser.OrgId, UID: rule.UID, }) } + if groupChanges.isEmpty() { + return response.JSON(http.StatusAccepted, util.DynMap{"message": "no changes detected in the rule group"}) + } + return response.JSON(http.StatusAccepted, util.DynMap{"message": "rule group updated successfully"}) } @@ -364,15 +387,25 @@ func toNamespaceErrorResponse(err error) response.Response { return apierrors.ToFolderErrorResponse(err) } -type RuleChanges struct { - newRules int - Upsert []store.UpsertRule - Delete []*ngmodels.AlertRule +type ruleUpdate struct { + Existing *ngmodels.AlertRule + New *ngmodels.AlertRule + Diff cmputil.DiffReport +} + +type changes struct { + New []*ngmodels.AlertRule + Update []ruleUpdate + Delete []*ngmodels.AlertRule +} + +func (c *changes) isEmpty() bool { + return len(c.Update)+len(c.New)+len(c.Delete) == 0 } // calculateChanges calculates the difference between rules in the group in the database and the submitted rules. If a submitted rule has UID it tries to find it in the database (in other groups). // returns a list of rules that need to be added, updated and deleted. Deleted considered rules in the database that belong to the group but do not exist in the list of submitted rules. -func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int64, namespace *models.Folder, ruleGroupName string, submittedRules []*ngmodels.AlertRule) (*RuleChanges, error) { +func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int64, namespace *models.Folder, ruleGroupName string, submittedRules []*ngmodels.AlertRule) (*changes, error) { q := &ngmodels.ListRuleGroupAlertRulesQuery{ OrgID: orgId, NamespaceUID: namespace.Uid, @@ -388,9 +421,8 @@ func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int6 existingGroupRulesUIDs[r.UID] = r } - upsert := make([]store.UpsertRule, 0, len(submittedRules)) - toDelete := make([]*ngmodels.AlertRule, 0, len(submittedRules)) - newRules := 0 + var toAdd, toDelete []*ngmodels.AlertRule + var toUpdate []ruleUpdate for _, r := range submittedRules { var existing *ngmodels.AlertRule = nil @@ -414,19 +446,21 @@ func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int6 } if existing == nil { - upsert = append(upsert, store.UpsertRule{ - Existing: nil, - New: *r, - }) - newRules++ + toAdd = append(toAdd, r) continue } ngmodels.PatchPartialAlertRule(existing, r) - // TODO diff between patched and existing, as well as between submitted - upsert = append(upsert, store.UpsertRule{ + + diff := existing.Diff(r, alertRuleFieldsToIgnoreInDiff...) + if len(diff) == 0 { + continue + } + + toUpdate = append(toUpdate, ruleUpdate{ Existing: existing, - New: *r, + New: r, + Diff: diff, }) continue } @@ -435,9 +469,12 @@ func calculateChanges(ctx context.Context, ruleStore store.RuleStore, orgId int6 toDelete = append(toDelete, rule) } - return &RuleChanges{ - Upsert: upsert, - Delete: toDelete, - newRules: newRules, + return &changes{ + New: toAdd, + Delete: toDelete, + Update: toUpdate, }, nil } + +// alertRuleFieldsToIgnoreInDiff contains fields that the AlertRule.Diff should ignore +var alertRuleFieldsToIgnoreInDiff = []string{"ID", "Version", "Updated"} diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index b26a799d8a6..3b5eb14425a 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" models2 "github.com/grafana/grafana/pkg/models" @@ -29,23 +28,14 @@ func TestCalculateChanges(t *testing.T) { changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted) require.NoError(t, err) - require.Equal(t, changes.newRules, len(submitted)) + require.Len(t, changes.New, len(submitted)) require.Empty(t, changes.Delete) - require.Len(t, changes.Upsert, len(submitted)) - for _, rule := range changes.Upsert { - require.Nil(t, rule.Existing) - } - - opts := []cmp.Option{ - cmp.FilterPath(func(path cmp.Path) bool { - return path.String() == "Data.modelProps" - }, cmp.Ignore()), - } + require.Empty(t, changes.Update) outerloop: for _, expected := range submitted { - for _, rule := range changes.Upsert { - if cmp.Equal(*expected, rule.New, opts...) { + for _, rule := range changes.New { + if len(expected.Diff(rule)) == 0 { continue outerloop } } @@ -64,8 +54,8 @@ func TestCalculateChanges(t *testing.T) { changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, make([]*models.AlertRule, 0)) require.NoError(t, err) - require.Equal(t, 0, changes.newRules) - require.Len(t, changes.Upsert, 0) + require.Empty(t, changes.New) + require.Empty(t, changes.Update) require.Len(t, changes.Delete, len(inDatabaseMap)) for _, toDelete := range changes.Delete { require.Contains(t, inDatabaseMap, toDelete.UID) @@ -86,15 +76,44 @@ func TestCalculateChanges(t *testing.T) { changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted) require.NoError(t, err) - require.Len(t, changes.Upsert, len(inDatabase)) - for _, upsert := range changes.Upsert { + require.Len(t, changes.Update, len(inDatabase)) + for _, upsert := range changes.Update { require.NotNil(t, upsert.Existing) require.Equal(t, upsert.Existing.UID, upsert.New.UID) require.Equal(t, inDatabaseMap[upsert.Existing.UID], upsert.Existing) - require.Equal(t, *submittedMap[upsert.Existing.UID], upsert.New) + require.Equal(t, submittedMap[upsert.Existing.UID], upsert.New) + require.NotEmpty(t, upsert.Diff) + } + require.Empty(t, changes.Delete) + require.Empty(t, changes.New) + }) + + t.Run("should include only if there are changes ignoring specific fields", func(t *testing.T) { + namespace := randFolder() + groupName := util.GenerateShortUID() + _, inDatabase := models.GenerateUniqueAlertRules(rand.Intn(5)+1, models.AlertRuleGen(withOrgID(orgId), withGroup(groupName), withNamespace(namespace))) + + submitted := make([]*models.AlertRule, 0, len(inDatabase)) + for _, rule := range inDatabase { + r := models.CopyRule(rule) + + // Ignore difference in the following fields as submitted models do not have them set + r.ID = rand.Int63() + r.Version = rand.Int63() + r.Updated = r.Updated.Add(1 * time.Minute) + + submitted = append(submitted, r) } - require.Len(t, changes.Delete, 0) - require.Equal(t, 0, changes.newRules) + + fakeStore := store.NewFakeRuleStore(t) + fakeStore.PutRule(context.Background(), inDatabase...) + + changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted) + require.NoError(t, err) + + require.Empty(t, changes.Update) + require.Empty(t, changes.Delete) + require.Empty(t, changes.New) }) t.Run("should patch rule with UID specified by existing rule", func(t *testing.T) { @@ -150,12 +169,12 @@ func TestCalculateChanges(t *testing.T) { submitted := *expected changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, []*models.AlertRule{&submitted}) require.NoError(t, err) - require.Len(t, changes.Upsert, 1) - ch := changes.Upsert[0] + require.Len(t, changes.Update, 1) + ch := changes.Update[0] require.Equal(t, ch.Existing, dbRule) fixed := *expected models.PatchPartialAlertRule(dbRule, &fixed) - require.Equal(t, fixed, ch.New) + require.Equal(t, fixed, *ch.New) }) } }) @@ -173,14 +192,15 @@ func TestCalculateChanges(t *testing.T) { changes, err := calculateChanges(context.Background(), fakeStore, orgId, namespace, groupName, submitted) require.NoError(t, err) - require.Len(t, changes.Delete, 0) - require.Equal(t, 0, changes.newRules) - require.Len(t, changes.Upsert, len(submitted)) - for _, upsert := range changes.Upsert { - require.NotNil(t, upsert.Existing) - require.Equal(t, upsert.Existing.UID, upsert.New.UID) - require.Equal(t, inDatabaseMap[upsert.Existing.UID], upsert.Existing) - require.Equal(t, *submittedMap[upsert.Existing.UID], upsert.New) + require.Empty(t, changes.Delete) + require.Empty(t, changes.New) + require.Len(t, changes.Update, len(submitted)) + for _, update := range changes.Update { + require.NotNil(t, update.Existing) + require.Equal(t, update.Existing.UID, update.New.UID) + require.Equal(t, inDatabaseMap[update.Existing.UID], update.Existing) + require.Equal(t, submittedMap[update.Existing.UID], update.New) + require.NotEmpty(t, update.Diff) } }) diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index ff38d29ebdf..06f4682720d 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -1787,7 +1787,7 @@ func TestAlertRuleCRUD(t *testing.T) { }`, body) } - // update the rule; keep title, condition, no data state, error state, queries and expressions if not provided + // update the rule; keep title, condition, no data state, error state, queries and expressions if not provided. should be noop { rules := apimodels.PostableRuleGroupConfig{ Name: "arulegroup", @@ -1817,7 +1817,7 @@ func TestAlertRuleCRUD(t *testing.T) { require.NoError(t, err) assert.Equal(t, resp.StatusCode, 202) - require.JSONEq(t, `{"message":"rule group updated successfully"}`, string(b)) + require.JSONEq(t, `{"message":"no changes detected in the rule group"}`, string(b)) // let's make sure that rule definitions are updated correctly. u = fmt.Sprintf("http://grafana:password@%s/api/ruler/grafana/api/v1/rules/default", grafanaListedAddr) @@ -1872,7 +1872,7 @@ func TestAlertRuleCRUD(t *testing.T) { ], "updated":"2021-02-21T01:10:30Z", "intervalSeconds":60, - "version":4, + "version":3, "uid":"uid", "namespace_uid":"nsuid", "namespace_id":1,