Manual cherry-pick of (37969b1) #34155 into release 10.11. (#34334)

Automatic Merge

server/config/database.go

@@ -418,9 +418,24 @@ func (ds *DatabaseStore) Close() error {
 	return ds.db.Close()
 }
 
-// removes configurations from database if they are older than threshold.
+// removes configurations from database if they are older than threshold,
-func (ds *DatabaseStore) cleanUp(thresholdCreatAt int) error {
+// keeping the active configuration and the last 5 most recent ones.
-	if _, err := ds.db.NamedExec("DELETE FROM Configurations Where CreateAt < :timestamp", map[string]any{"timestamp": thresholdCreatAt}); err != nil {
+func (ds *DatabaseStore) cleanUp(thresholdCreateAt int64) error {
+	query := `
+		DELETE FROM Configurations
+		WHERE CreateAt < :timestamp
+			AND (Active IS NULL OR Active = false)
+			AND ID NOT IN (
+				SELECT ID FROM (
+					SELECT ID
+					FROM Configurations
+					ORDER BY CreateAt DESC
+					LIMIT 5
+				) AS recent
+			);
+	`
+
+	if _, err := ds.db.NamedExec(query, map[string]any{"timestamp": thresholdCreateAt}); err != nil {
 		return errors.Wrap(err, "unable to clean Configurations table")
 	}
 

server/config/database_test.go

@@ -1122,40 +1122,79 @@ func TestCleanUp(t *testing.T) {
 	require.NotNil(t, ds)
 	defer ds.Close()
 
-	dbs, ok := ds.backingStore.(*DatabaseStore)
+	t.Run("should keep last 5 configurations regardless", func(t *testing.T) {
-	require.True(t, ok, "should be a DatabaseStore instance")
+		dbs, ok := ds.backingStore.(*DatabaseStore)
+		require.True(t, ok, "should be a DatabaseStore instance")
 
-	b, err := marshalConfig(ds.config)
+		b, err := marshalConfig(ds.config)
-	require.NoError(t, err)
+		require.NoError(t, err)
 
-	ds.config.JobSettings.CleanupConfigThresholdDays = model.NewPointer(30) // we set 30 days as threshold
+		ds.config.JobSettings.CleanupConfigThresholdDays = model.NewPointer(30) // we set 30 days as threshold
+
+		var initialCount int
+		row := dbs.db.QueryRow("SELECT COUNT(*) FROM Configurations")
+		err = row.Scan(&initialCount)
+		require.NoError(t, err)
+		require.Less(t, initialCount, 5, "should have less than 5 configurations before test")
+
+		now := time.Now()
+		for i := range 10 {
+			// we are simulating that each config was created 40 days apart
+			// so all but last 5 should be deleted
+			m := -1 * i * 24 * 40
+			params := map[string]any{
+				"id":        model.NewId(),
+				"value":     string(b),
+				"create_at": model.GetMillisForTime(now.Add(time.Duration(m) * time.Hour)),
+			}
+
+			_, err = dbs.db.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt) VALUES (:id, :value, :create_at)", params)
+			require.NoError(t, err)
+		}
+		var beforeCleanup int
+		row = dbs.db.QueryRow("SELECT COUNT(*) FROM Configurations")
+		err = row.Scan(&beforeCleanup)
+		require.NoError(t, err)
+		require.Equal(t, 10+initialCount, beforeCleanup, "should have more than 10 configurations before cleanup")
+
+		err = ds.CleanUp()
+		require.NoError(t, err)
+
+		var count int
+		row = dbs.db.QueryRow("SELECT COUNT(*) FROM Configurations")
+		err = row.Scan(&count)
+		require.NoError(t, err)
+		require.Equal(t, 5, count, "should have only 5 configurations left")
+	})
+
+	t.Run("should keep the active configuration regardless", func(t *testing.T) {
+		b, err := marshalConfig(ds.config)
+		require.NoError(t, err)
+
+		dbs, ok := ds.backingStore.(*DatabaseStore)
+		require.True(t, ok, "should be a DatabaseStore instance")
+
+		// remove all other configurations
+		_, err = dbs.db.Exec("DELETE FROM Configurations")
+		require.NoError(t, err)
 
-	now := time.Now()
-	for i := 0; i < 5; i++ {
-		// 20 days, we expect to remove at least 3 configuration values from the store
-		// first 2 (0 and 1) will be within a month constraint, others will be older than
-		// a month hence we expect 3 configurations to be removed from the database.
-		m := -1 * i * 24 * 20
 		params := map[string]any{
-			"id":        model.NewId(),
+			"id":    model.NewId(),
-			"value":     string(b),
+			"value": string(b),
-			"create_at": model.GetMillisForTime(now.Add(time.Duration(m) * time.Hour)),
+			// we set the create_at to 100 days ago so it wouldn't be deleted if it is active
+			"create_at": model.GetMillisForTime(time.Now().Add(time.Duration(-1*100*24) * time.Hour)),
 		}
 
 		_, err = dbs.db.NamedExec("INSERT INTO Configurations (Id, Value, CreateAt) VALUES (:id, :value, :create_at)", params)
 		require.NoError(t, err)
-	}
-	var initialCount int
-	row := dbs.db.QueryRow("SELECT COUNT(*) FROM Configurations")
-	err = row.Scan(&initialCount)
-	require.NoError(t, err)
 
-	err = ds.CleanUp()
+		err = ds.CleanUp()
-	require.NoError(t, err)
+		require.NoError(t, err)
 
-	var count int
+		var count int
-	row = dbs.db.QueryRow("SELECT COUNT(*) FROM Configurations")
+		row := dbs.db.QueryRow("SELECT COUNT(*) FROM Configurations")
-	err = row.Scan(&count)
+		err = row.Scan(&count)
-	require.NoError(t, err)
+		require.NoError(t, err)
-	require.True(t, count+3 == initialCount)
+		require.Equal(t, 1, count, "should have only 1 configuration left")
+	})
 }

server/config/store.go

@@ -406,7 +406,7 @@ func (s *Store) CleanUp() error {
 	case *DatabaseStore:
 		dur := time.Duration(*s.config.JobSettings.CleanupConfigThresholdDays) * time.Hour * 24
 		expiry := model.GetMillisForTime(time.Now().Add(-dur))
-		return bs.cleanUp(int(expiry))
+		return bs.cleanUp(expiry)
 	default:
 		return nil
 	}