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

@@ -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 {

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)
 	}
 }
 

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
+// CleanAnnotations deletes old annotations created by alert rules, API
-// alert rules, API requests and human made in the UI.
+// requests and human made in the UI. It subsequently deletes orphaned rows
-func (acs *AnnotationCleanupService) CleanAnnotations(ctx context.Context, cfg *setting.Cfg) error {
+// from the annotation_tag table. Cleanup actions are performed in batches
-	err := acs.cleanAnnotations(ctx, cfg.AlertingAnnotationCleanupSetting, alertAnnotationType)
+// 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
 			}
 		}
 	}

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)
+		}
 	}
 }