From af3c0fe602ed9142a259a6c14c1250c94b876989 Mon Sep 17 00:00:00 2001 From: Dafydd <72009875+dafydd-t@users.noreply.github.com> Date: Tue, 8 Dec 2020 18:41:35 +0000 Subject: [PATCH] Bugfix 29848: Remove annotation_tag entries as part of annotations cleanup (#29534) closes https://github.com/grafana/grafana/issues/29484 --- pkg/services/annotations/annotations.go | 2 +- pkg/services/cleanup/cleanup.go | 4 +- pkg/services/sqlstore/annotation_cleanup.go | 63 +++++++++++++------ .../sqlstore/annotation_cleanup_test.go | 38 ++++++++++- 4 files changed, 85 insertions(+), 22 deletions(-) diff --git a/pkg/services/annotations/annotations.go b/pkg/services/annotations/annotations.go index cc25567e53e..838eca98d3f 100644 --- a/pkg/services/annotations/annotations.go +++ b/pkg/services/annotations/annotations.go @@ -16,7 +16,7 @@ type Repository interface { // AnnotationCleaner is responsible for cleaning up old annotations type AnnotationCleaner interface { - CleanAnnotations(ctx context.Context, cfg *setting.Cfg) error + CleanAnnotations(ctx context.Context, cfg *setting.Cfg) (int64, int64, error) } type ItemQuery struct { diff --git a/pkg/services/cleanup/cleanup.go b/pkg/services/cleanup/cleanup.go index 442180bf367..8254a4facd7 100644 --- a/pkg/services/cleanup/cleanup.go +++ b/pkg/services/cleanup/cleanup.go @@ -65,9 +65,11 @@ func (srv *CleanUpService) Run(ctx context.Context) error { func (srv *CleanUpService) cleanUpOldAnnotations(ctx context.Context) { cleaner := annotations.GetAnnotationCleaner() - err := cleaner.CleanAnnotations(ctx, srv.Cfg) + affected, affectedTags, err := cleaner.CleanAnnotations(ctx, srv.Cfg) if err != nil { srv.log.Error("failed to clean up old annotations", "error", err) + } else { + srv.log.Debug("Deleted excess annotations", "annotations affected", affected, "annotation tags affected", affectedTags) } } diff --git a/pkg/services/sqlstore/annotation_cleanup.go b/pkg/services/sqlstore/annotation_cleanup.go index ad1ba15ece5..a878b0879d2 100644 --- a/pkg/services/sqlstore/annotation_cleanup.go +++ b/pkg/services/sqlstore/annotation_cleanup.go @@ -9,7 +9,7 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -// AnnotationCleanupService is responseible for cleaning old annotations. +// AnnotationCleanupService is responsible for cleaning old annotations. type AnnotationCleanupService struct { batchSize int64 log log.Logger @@ -21,48 +21,74 @@ const ( apiAnnotationType = "alert_id = 0 AND dashboard_id = 0" ) -// CleanAnnotations deletes old annotations created by -// alert rules, API requests and human made in the UI. -func (acs *AnnotationCleanupService) CleanAnnotations(ctx context.Context, cfg *setting.Cfg) error { - err := acs.cleanAnnotations(ctx, cfg.AlertingAnnotationCleanupSetting, alertAnnotationType) +// CleanAnnotations deletes old annotations created by alert rules, API +// requests and human made in the UI. It subsequently deletes orphaned rows +// from the annotation_tag table. Cleanup actions are performed in batches +// so that no query takes too long to complete. +// +// Returns the number of annotation and annotation_tag rows deleted. If an +// error occurs, it returns the number of rows affected so far. +func (acs *AnnotationCleanupService) CleanAnnotations(ctx context.Context, cfg *setting.Cfg) (int64, int64, error) { + var totalCleanedAnnotations int64 + affected, err := acs.cleanAnnotations(ctx, cfg.AlertingAnnotationCleanupSetting, alertAnnotationType) + totalCleanedAnnotations += affected if err != nil { - return err + return totalCleanedAnnotations, 0, err } - err = acs.cleanAnnotations(ctx, cfg.APIAnnotationCleanupSettings, apiAnnotationType) + affected, err = acs.cleanAnnotations(ctx, cfg.APIAnnotationCleanupSettings, apiAnnotationType) + totalCleanedAnnotations += affected if err != nil { - return err + return totalCleanedAnnotations, 0, err } - return acs.cleanAnnotations(ctx, cfg.DashboardAnnotationCleanupSettings, dashboardAnnotationType) + affected, err = acs.cleanAnnotations(ctx, cfg.DashboardAnnotationCleanupSettings, dashboardAnnotationType) + totalCleanedAnnotations += affected + if err != nil { + return totalCleanedAnnotations, 0, err + } + + affected, err = acs.cleanOrphanedAnnotationTags(ctx) + return totalCleanedAnnotations, affected, err } -func (acs *AnnotationCleanupService) cleanAnnotations(ctx context.Context, cfg setting.AnnotationCleanupSettings, annotationType string) error { +func (acs *AnnotationCleanupService) cleanAnnotations(ctx context.Context, cfg setting.AnnotationCleanupSettings, annotationType string) (int64, error) { + var totalAffected int64 if cfg.MaxAge > 0 { cutoffDate := time.Now().Add(-cfg.MaxAge).UnixNano() / int64(time.Millisecond) deleteQuery := `DELETE FROM annotation WHERE id IN (SELECT id FROM (SELECT id FROM annotation WHERE %s AND created < %v ORDER BY id DESC %s) a)` sql := fmt.Sprintf(deleteQuery, annotationType, cutoffDate, dialect.Limit(acs.batchSize)) - err := acs.executeUntilDoneOrCancelled(ctx, sql) + affected, err := acs.executeUntilDoneOrCancelled(ctx, sql) + totalAffected += affected if err != nil { - return err + return totalAffected, err } } if cfg.MaxCount > 0 { deleteQuery := `DELETE FROM annotation WHERE id IN (SELECT id FROM (SELECT id FROM annotation WHERE %s ORDER BY id DESC %s) a)` sql := fmt.Sprintf(deleteQuery, annotationType, dialect.LimitOffset(acs.batchSize, cfg.MaxCount)) - return acs.executeUntilDoneOrCancelled(ctx, sql) + affected, err := acs.executeUntilDoneOrCancelled(ctx, sql) + totalAffected += affected + return totalAffected, err } - return nil + return totalAffected, nil } -func (acs *AnnotationCleanupService) executeUntilDoneOrCancelled(ctx context.Context, sql string) error { +func (acs *AnnotationCleanupService) cleanOrphanedAnnotationTags(ctx context.Context) (int64, error) { + deleteQuery := `DELETE FROM annotation_tag WHERE id IN ( SELECT id FROM (SELECT id FROM annotation_tag WHERE NOT EXISTS (SELECT 1 FROM annotation a WHERE annotation_id = a.id) %s) a)` + sql := fmt.Sprintf(deleteQuery, dialect.Limit(acs.batchSize)) + return acs.executeUntilDoneOrCancelled(ctx, sql) +} + +func (acs *AnnotationCleanupService) executeUntilDoneOrCancelled(ctx context.Context, sql string) (int64, error) { + var totalAffected int64 for { select { case <-ctx.Done(): - return ctx.Err() + return totalAffected, ctx.Err() default: var affected int64 err := withDbSession(ctx, func(session *DBSession) error { @@ -72,15 +98,16 @@ func (acs *AnnotationCleanupService) executeUntilDoneOrCancelled(ctx context.Con } affected, err = res.RowsAffected() + totalAffected += affected return err }) if err != nil { - return err + return totalAffected, err } if affected == 0 { - return nil + return totalAffected, nil } } } diff --git a/pkg/services/sqlstore/annotation_cleanup_test.go b/pkg/services/sqlstore/annotation_cleanup_test.go index 75daf6b9055..e74d2ef1a3a 100644 --- a/pkg/services/sqlstore/annotation_cleanup_test.go +++ b/pkg/services/sqlstore/annotation_cleanup_test.go @@ -25,6 +25,7 @@ func TestAnnotationCleanUp(t *testing.T) { createTestAnnotations(t, fakeSQL, 21, 6) assertAnnotationCount(t, fakeSQL, "", 21) + assertAnnotationTagCount(t, fakeSQL, 42) tests := []struct { name string @@ -32,6 +33,7 @@ func TestAnnotationCleanUp(t *testing.T) { alertAnnotationCount int64 dashboardAnnotationCount int64 APIAnnotationCount int64 + affectedAnnotations int64 }{ { name: "default settings should not delete any annotations", @@ -43,6 +45,7 @@ func TestAnnotationCleanUp(t *testing.T) { alertAnnotationCount: 7, dashboardAnnotationCount: 7, APIAnnotationCount: 7, + affectedAnnotations: 0, }, { name: "should remove annotations created before cut off point", @@ -54,6 +57,7 @@ func TestAnnotationCleanUp(t *testing.T) { alertAnnotationCount: 5, dashboardAnnotationCount: 5, APIAnnotationCount: 5, + affectedAnnotations: 6, }, { name: "should only keep three annotations", @@ -65,6 +69,7 @@ func TestAnnotationCleanUp(t *testing.T) { alertAnnotationCount: 3, dashboardAnnotationCount: 3, APIAnnotationCount: 3, + affectedAnnotations: 6, }, { name: "running the max count delete again should not remove any annotations", @@ -76,18 +81,28 @@ func TestAnnotationCleanUp(t *testing.T) { alertAnnotationCount: 3, dashboardAnnotationCount: 3, APIAnnotationCount: 3, + affectedAnnotations: 0, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { cleaner := &AnnotationCleanupService{batchSize: 1, log: log.New("test-logger")} - err := cleaner.CleanAnnotations(context.Background(), test.cfg) + affectedAnnotations, affectedAnnotationTags, err := cleaner.CleanAnnotations(context.Background(), test.cfg) require.NoError(t, err) + assert.Equal(t, test.affectedAnnotations, affectedAnnotations) + assert.Equal(t, test.affectedAnnotations*2, affectedAnnotationTags) + assertAnnotationCount(t, fakeSQL, alertAnnotationType, test.alertAnnotationCount) assertAnnotationCount(t, fakeSQL, dashboardAnnotationType, test.dashboardAnnotationCount) assertAnnotationCount(t, fakeSQL, apiAnnotationType, test.APIAnnotationCount) + + // we create two records in annotation_tag for each sample annotation + expectedAnnotationTagCount := (test.alertAnnotationCount + + test.dashboardAnnotationCount + + test.APIAnnotationCount) * 2 + assertAnnotationTagCount(t, fakeSQL, expectedAnnotationTagCount) }) } } @@ -128,7 +143,7 @@ func TestOldAnnotationsAreDeletedFirst(t *testing.T) { // run the clean up task to keep one annotation. cleaner := &AnnotationCleanupService{batchSize: 1, log: log.New("test-logger")} - err = cleaner.cleanAnnotations(context.Background(), setting.AnnotationCleanupSettings{MaxCount: 1}, alertAnnotationType) + _, err = cleaner.cleanAnnotations(context.Background(), setting.AnnotationCleanupSettings{MaxCount: 1}, alertAnnotationType) require.NoError(t, err) // assert that the last annotations were kept @@ -151,6 +166,17 @@ func assertAnnotationCount(t *testing.T, fakeSQL *SQLStore, sql string, expected require.Equal(t, expectedCount, count) } +func assertAnnotationTagCount(t *testing.T, fakeSQL *SQLStore, expectedCount int64) { + t.Helper() + + session := fakeSQL.NewSession() + defer session.Close() + + count, err := session.SQL("select count(*) from annotation_tag").Count() + require.NoError(t, err) + require.Equal(t, expectedCount, count) +} + func createTestAnnotations(t *testing.T, sqlstore *SQLStore, expectedCount int, oldAnnotations int) { t.Helper() @@ -187,6 +213,14 @@ func createTestAnnotations(t *testing.T, sqlstore *SQLStore, expectedCount int, _, err := sqlstore.NewSession().Insert(a) require.NoError(t, err, "should be able to save annotation", err) + + // mimick the SQL annotation Save logic by writing records to the annotation_tag table + // we need to ensure they get deleted when we clean up annotations + sess := sqlstore.NewSession() + for tagID := range []int{1, 2} { + _, err = sess.Exec("INSERT INTO annotation_tag (annotation_id, tag_id) VALUES(?,?)", a.Id, tagID) + require.NoError(t, err, "should be able to save annotation tag ID", err) + } } }