SQLStore: Enable migration locking by default (#84983)

* Introduce new configuration for migration locking

* Remove feature toggle

* Fix test and turn it into an integration

* Fix docs

conf/defaults.ini

@@ -160,7 +160,10 @@ cache_mode = private
 # For "sqlite3" only. Enable/disable Write-Ahead Logging, https://sqlite.org/wal.html. Default is false.
 wal = false
 
-# For "mysql" only if migrationLocking feature toggle is set. How many seconds to wait before failing to lock the database for the migrations, default is 0.
+# For "mysql" and "postgres". Lock the database for the migrations, default is true.
+migration_locking = true
+
+# For "mysql" and "postgres" only if migrationLocking is set. How many seconds to wait before failing to lock the database for the migrations, default is 0.
 locking_attempt_timeout_sec = 0
 
 # For "sqlite" only. How many times to retry query in case of database is locked failures. Default is 0 (disabled).

conf/sample.ini

@@ -158,7 +158,10 @@
 # For "sqlite3" only. Enable/disable Write-Ahead Logging, https://sqlite.org/wal.html. Default is false.
 ;wal = false
 
-# For "mysql" only if migrationLocking feature toggle is set. How many seconds to wait before failing to lock the database for the migrations, default is 0.
+# For "mysql" and "postgres" only. Lock the database for the migrations, default is true.
+;migration_locking = true
+
+# For "mysql" and "postgres" only if migrationLocking is set. How many seconds to wait before failing to lock the database for the migrations, default is 0.
 ;locking_attempt_timeout_sec = 0
 
 # For "sqlite" only. How many times to retry query in case of database is locked failures. Default is 0 (disabled).

docs/sources/setup-grafana/configure-grafana/_index.md

@@ -372,6 +372,10 @@ The maximum number of open connections to the database. For MYSQL, configure thi
 
 Sets the maximum amount of time a connection may be reused. The default is 14400 (which means 14400 seconds or 4 hours). For MySQL, this setting should be shorter than the [`wait_timeout`](https://dev.mysql.com/doc/refman/5.7/en/server-system-variables.html#sysvar_wait_timeout) variable.
 
+### migration_locking
+
+Set to `false` to disable database locking during the migrations. Default is true.
+
 ### locking_attempt_timeout_sec
 
 For "mysql", if the `migrationLocking` feature toggle is set, specify the time (in seconds) to wait before failing to lock the database for the migrations. Default is 0.

docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md

@@ -66,7 +66,6 @@ Some features are enabled by default. You can disable these feature by setting t
 | Feature toggle name                    | Description                                                                                                                                                                                  |
 | -------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | `panelTitleSearch`                     | Search for dashboards using panel title                                                                                                                                                      |
-| `migrationLocking`                     | Lock database during migrations                                                                                                                                                              |
 | `autoMigrateOldPanels`                 | Migrate old angular panels to supported versions (graph, table-old, worldmap, etc)                                                                                                           |
 | `autoMigrateGraphPanel`                | Migrate old graph panel to supported time series panel - broken out from autoMigrateOldPanels to enable granular tracking                                                                    |
 | `autoMigrateTablePanel`                | Migrate old table panel to supported table panel - broken out from autoMigrateOldPanels to enable granular tracking                                                                          |

packages/grafana-data/src/types/featureToggles.gen.ts

@@ -27,7 +27,6 @@ export interface FeatureToggles {
   publicDashboardsScene?: boolean;
   lokiExperimentalStreaming?: boolean;
   featureHighlights?: boolean;
-  migrationLocking?: boolean;
   storage?: boolean;
   correlations?: boolean;
   exploreContentOutline?: boolean;

pkg/services/featuremgmt/registry.go

@@ -83,12 +83,6 @@ var (
 			Owner:          grafanaAsCodeSquad,
 			AllowSelfServe: true,
 		},
-		{
-			Name:        "migrationLocking",
-			Description: "Lock database during migrations",
-			Stage:       FeatureStagePublicPreview,
-			Owner:       grafanaBackendPlatformSquad,
-		},
 		{
 			Name:        "storage",
 			Description: "Configurable storage for dashboards, datasources, and resources",

pkg/services/featuremgmt/toggles_gen.csv

@@ -8,7 +8,6 @@ publicDashboardsEmailSharing,preview,@grafana/sharing-squad,false,false,false
 publicDashboardsScene,experimental,@grafana/sharing-squad,false,false,true
 lokiExperimentalStreaming,experimental,@grafana/observability-logs,false,false,false
 featureHighlights,GA,@grafana/grafana-as-code,false,false,false
-migrationLocking,preview,@grafana/backend-platform,false,false,false
 storage,experimental,@grafana/grafana-app-platform-squad,false,false,false
 correlations,GA,@grafana/explore-squad,false,false,false
 exploreContentOutline,GA,@grafana/explore-squad,false,false,true

pkg/services/featuremgmt/toggles_gen.go

@@ -43,10 +43,6 @@ const (
 	// Highlight Grafana Enterprise features
 	FlagFeatureHighlights = "featureHighlights"
 
-	// FlagMigrationLocking
-	// Lock database during migrations
-	FlagMigrationLocking = "migrationLocking"
-
 	// FlagStorage
 	// Configurable storage for dashboards, datasources, and resources
 	FlagStorage = "storage"

pkg/services/featuremgmt/toggles_gen.json

@@ -1428,7 +1428,8 @@
       "metadata": {
         "name": "migrationLocking",
         "resourceVersion": "1709648236447",
-        "creationTimestamp": "2024-03-05T14:17:16Z"
+        "creationTimestamp": "2024-03-05T14:17:16Z",
+        "deletionTimestamp": "2024-03-22T10:42:09Z"
       },
       "spec": {
         "description": "Lock database during migrations",

pkg/services/sqlstore/database_config.go

@@ -39,6 +39,7 @@ type DatabaseConfig struct {
 	WALEnabled                  bool
 	UrlQueryParams              map[string][]string
 	SkipMigrations              bool
+	MigrationLock               bool
 	MigrationLockAttemptTimeout int
 	LogQueries                  bool
 	// SQLite only
@@ -113,6 +114,7 @@ func (dbCfg *DatabaseConfig) readConfig(cfg *setting.Cfg) error {
 	dbCfg.CacheMode = sec.Key("cache_mode").MustString("private")
 	dbCfg.WALEnabled = sec.Key("wal").MustBool(false)
 	dbCfg.SkipMigrations = sec.Key("skip_migrations").MustBool()
+	dbCfg.MigrationLock = sec.Key("migration_locking").MustBool(true)
 	dbCfg.MigrationLockAttemptTimeout = sec.Key("locking_attempt_timeout_sec").MustInt()
 
 	dbCfg.QueryRetries = sec.Key("query_retries").MustInt()

pkg/services/sqlstore/migrations/migrations_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/go-sql-driver/mysql"
+	"github.com/golang-migrate/migrate/v4/database"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/ini.v1"
@@ -69,7 +70,11 @@ func TestMigrations(t *testing.T) {
 	checkStepsAndDatabaseMatch(t, mg, expectedMigrations)
 }
 
-func TestMigrationLock(t *testing.T) {
+func TestIntegrationMigrationLock(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}
+
 	dbType := sqlutil.GetTestDBType()
 	if dbType == SQLite {
 		t.Skip()
@@ -96,9 +101,12 @@ func TestMigrationLock(t *testing.T) {
 		sess.Close()
 	})
 
+	key, err := database.GenerateAdvisoryLockId("test")
+	require.NoError(t, err)
+
 	cfg := LockCfg{
 		Session: sess,
-		Key:     "test",
+		Key:     key,
 	}
 
 	t.Run("obtaining lock should succeed", func(t *testing.T) {
@@ -143,7 +151,7 @@ func TestMigrationLock(t *testing.T) {
 		err = dialect.Lock(cfg)
 		require.NoError(t, err)
 
-		err = d2.Lock(LockCfg{Session: sess2})
+		err = d2.Lock(LockCfg{Session: sess2, Key: key})
 		require.Error(t, err)
 		assert.ErrorIs(t, err, ErrLockDB)
 

pkg/services/sqlstore/sqlstore.go

@@ -67,7 +67,7 @@ func ProvideService(cfg *setting.Cfg,
 	}
 	s.features = features
 
-	if err := s.Migrate(features.IsEnabledGlobally(featuremgmt.FlagMigrationLocking)); err != nil {
+	if err := s.Migrate(s.dbCfg.MigrationLock); err != nil {
 		return nil, err
 	}
 

pkg/services/store/entity/db/migrations/migrator.go

@@ -70,7 +70,6 @@ func MigrateEntityStore(db db.EntityDBInterface, features featuremgmt.FeatureTog
 		}
 	}
 
-	return mg.Start(
-		features.IsEnabledGlobally(featuremgmt.FlagMigrationLocking),
-		0)
+	// since it's a new feature enable migration locking by default
+	return mg.Start(true, 0)
 }