diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 52a331415fc..3e60ddff3c2 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -1,8 +1,6 @@ package migrations import ( - "os" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/sqlstore/migrations/accesscontrol" "github.com/grafana/grafana/pkg/services/sqlstore/migrations/ualert" @@ -52,11 +50,6 @@ func (*OSSMigrations) AddMigration(mg *Migrator) { addUserAuthTokenMigrations(mg) addCacheMigration(mg) addShortURLMigrations(mg) - // TODO Delete when unified alerting is enabled by default unconditionally (Grafana v9) - if err := ualert.CheckUnifiedAlertingEnabledByDefault(mg); err != nil { // this should always go before any other ualert migration - mg.Logger.Error("failed to determine the status of alerting engine. Enable either legacy or unified alerting explicitly and try again", "err", err) - os.Exit(1) - } ualert.AddTablesMigrations(mg) ualert.AddDashAlertMigration(mg) addLibraryElementsMigrations(mg) diff --git a/pkg/services/sqlstore/migrations/ualert/ualert.go b/pkg/services/sqlstore/migrations/ualert/ualert.go index 7d118270d9b..5bd0bf5cacc 100644 --- a/pkg/services/sqlstore/migrations/ualert/ualert.go +++ b/pkg/services/sqlstore/migrations/ualert/ualert.go @@ -706,48 +706,6 @@ func (u *upgradeNgAlerting) SQL(migrator.Dialect) string { return "code migration" } -// CheckUnifiedAlertingEnabledByDefault determines the final status of unified alerting, if it is not enabled explicitly. -// Checks table `alert` and if it is empty, then it changes UnifiedAlerting.Enabled to true. Otherwise, it sets the flag to false. -// After this method is executed the status of alerting should be determined, i.e. both flags will not be nil. -// Note: this is not a real migration but a step that other migrations depend on. -// TODO Delete when unified alerting is enabled by default unconditionally (Grafana v9) -func CheckUnifiedAlertingEnabledByDefault(migrator *migrator.Migrator) error { - // if [unified_alerting][enabled] is explicitly set, we've got nothing to do here. - if migrator.Cfg.UnifiedAlerting.Enabled != nil { - return nil - } - var ualertEnabled bool - // this duplicates the logic in setting.ReadUnifiedAlertingSettings, and is put here just for logical completeness. - if setting.AlertingEnabled != nil && !*setting.AlertingEnabled { - ualertEnabled = true - migrator.Cfg.UnifiedAlerting.Enabled = &ualertEnabled - migrator.Logger.Debug("Unified alerting is enabled because the legacy is disabled explicitly") - return nil - } - - resp := &struct { - Count int64 - }{} - exist, err := migrator.DBEngine.IsTableExist("alert") - if err != nil { - return fmt.Errorf("failed to verify if the 'alert' table exists: %w", err) - } - if exist { - if _, err := migrator.DBEngine.SQL("SELECT COUNT(1) as count FROM alert").Get(resp); err != nil { - return fmt.Errorf("failed to read 'alert' table: %w", err) - } - } - // if table does not exist then we treat it as absence of legacy alerting and therefore enable unified alerting. - - ualertEnabled = resp.Count == 0 - legacyEnabled := !ualertEnabled - migrator.Cfg.UnifiedAlerting.Enabled = &ualertEnabled - setting.AlertingEnabled = &legacyEnabled - - migrator.Logger.Debug(fmt.Sprintf("Found %d legacy alerts in the database. Unified alerting enabled is %v", resp.Count, ualertEnabled)) - return nil -} - // getAlertFolderNameFromDashboard generates a folder name for alerts that belong to a dashboard. Formats the string according to DASHBOARD_FOLDER format. // If the resulting string exceeds the migrations.MaxTitleLength, the dashboard title is stripped to be at the maximum length func getAlertFolderNameFromDashboard(dash *dashboard) string { diff --git a/pkg/services/sqlstore/migrations/ualert/ualert_test.go b/pkg/services/sqlstore/migrations/ualert/ualert_test.go index 75abe7369fa..38650fc1ed8 100644 --- a/pkg/services/sqlstore/migrations/ualert/ualert_test.go +++ b/pkg/services/sqlstore/migrations/ualert/ualert_test.go @@ -7,15 +7,10 @@ import ( "sort" "testing" + "github.com/grafana/grafana/pkg/components/simplejson" + "github.com/grafana/grafana/pkg/util" "github.com/prometheus/alertmanager/pkg/labels" "github.com/stretchr/testify/require" - "xorm.io/xorm" - - "github.com/grafana/grafana/pkg/components/simplejson" - "github.com/grafana/grafana/pkg/services/sqlstore/migrator" - "github.com/grafana/grafana/pkg/services/sqlstore/sqlutil" - "github.com/grafana/grafana/pkg/setting" - "github.com/grafana/grafana/pkg/util" ) var MigTitle = migTitle @@ -103,99 +98,6 @@ func Test_validateAlertmanagerConfig(t *testing.T) { } } -func TestCheckUnifiedAlertingEnabledByDefault(t *testing.T) { - testDB := sqlutil.SQLite3TestDB() - x, err := xorm.NewEngine(testDB.DriverName, testDB.ConnStr) - require.NoError(t, err) - _, err = x.Exec("CREATE TABLE alert ( id bigint )") - require.NoError(t, err) - t.Cleanup(func() { - _, err = x.Exec("DROP TABLE alert") - require.NoError(t, err) - }) - - tests := []struct { - title string - legacyAlertExists bool - legacyIsDefined bool - legacyValue bool - expectedUnifiedAlerting bool - }{ - { - title: "enable unified alerting when there are no legacy alerts", - legacyIsDefined: false, - legacyAlertExists: false, - expectedUnifiedAlerting: true, - }, - { - title: "enable unified alerting when there are no legacy alerts and legacy enabled", - legacyIsDefined: true, - legacyValue: true, - legacyAlertExists: false, - expectedUnifiedAlerting: true, - }, - { - title: "enable unified alerting when there are no legacy alerts and legacy disabled", - legacyIsDefined: true, - legacyValue: false, - legacyAlertExists: false, - expectedUnifiedAlerting: true, - }, - { - title: "enable unified alerting when there are legacy alerts but legacy disabled", - legacyIsDefined: true, - legacyValue: false, - legacyAlertExists: true, - expectedUnifiedAlerting: true, - }, - { - title: "disable unified alerting when there are legacy alerts", - legacyIsDefined: false, - legacyAlertExists: true, - expectedUnifiedAlerting: false, - }, - { - title: "disable unified alerting when there are legacy alerts and it is enabled", - legacyIsDefined: true, - legacyValue: true, - legacyAlertExists: true, - expectedUnifiedAlerting: false, - }, - } - - for _, test := range tests { - t.Run(test.title, func(t *testing.T) { - setting.AlertingEnabled = nil - if test.legacyIsDefined { - value := test.legacyValue - setting.AlertingEnabled = &value - } - - if test.legacyAlertExists { - _, err := x.Exec("INSERT INTO alert VALUES (1)") - require.NoError(t, err) - } else { - _, err := x.Exec("DELETE FROM alert") - require.NoError(t, err) - } - - cfg := setting.Cfg{ - UnifiedAlerting: setting.UnifiedAlertingSettings{ - Enabled: nil, - }, - } - mg := migrator.NewMigrator(x, &cfg) - - err := CheckUnifiedAlertingEnabledByDefault(mg) - require.NoError(t, err) - require.NotNil(t, setting.AlertingEnabled) - require.NotNil(t, cfg.UnifiedAlerting.Enabled) - require.Equal(t, *cfg.UnifiedAlerting.Enabled, test.expectedUnifiedAlerting) - require.Equal(t, *setting.AlertingEnabled, !test.expectedUnifiedAlerting) - }) - } -} - func configFromReceivers(t *testing.T, receivers []*PostableGrafanaReceiver) *PostableUserConfig { t.Helper() diff --git a/pkg/setting/setting_test.go b/pkg/setting/setting_test.go index 8eac5e99be5..1018cd65951 100644 --- a/pkg/setting/setting_test.go +++ b/pkg/setting/setting_test.go @@ -539,9 +539,10 @@ func TestAlertingEnabled(t *testing.T) { require.NoError(t, err) err = cfg.ReadUnifiedAlertingSettings(f) require.NoError(t, err) - assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, cfg.UnifiedAlerting.Enabled) + assert.Equal(t, true, *cfg.UnifiedAlerting.Enabled) assert.NotNil(t, AlertingEnabled) - assert.Equal(t, *AlertingEnabled, true) + assert.Equal(t, false, *AlertingEnabled) }, }, { @@ -557,9 +558,9 @@ func TestAlertingEnabled(t *testing.T) { err = cfg.ReadUnifiedAlertingSettings(f) require.NoError(t, err) assert.NotNil(t, cfg.UnifiedAlerting.Enabled) - assert.Equal(t, *cfg.UnifiedAlerting.Enabled, false) + assert.Equal(t, true, *cfg.UnifiedAlerting.Enabled) assert.NotNil(t, AlertingEnabled) - assert.Equal(t, *AlertingEnabled, true) + assert.Equal(t, false, *AlertingEnabled) }, }, { @@ -593,7 +594,7 @@ func TestAlertingEnabled(t *testing.T) { err = cfg.ReadUnifiedAlertingSettings(f) require.NoError(t, err) assert.NotNil(t, cfg.UnifiedAlerting.Enabled) - assert.Equal(t, *cfg.UnifiedAlerting.Enabled, false) + assert.Equal(t, *cfg.UnifiedAlerting.Enabled, true) assert.NotNil(t, AlertingEnabled) assert.Equal(t, *AlertingEnabled, false) }, @@ -610,7 +611,8 @@ func TestAlertingEnabled(t *testing.T) { require.NoError(t, err) err = cfg.ReadUnifiedAlertingSettings(f) require.NoError(t, err) - assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, cfg.UnifiedAlerting.Enabled) + assert.True(t, *cfg.UnifiedAlerting.Enabled) assert.Nil(t, AlertingEnabled) }, }, @@ -627,9 +629,8 @@ func TestAlertingEnabled(t *testing.T) { err = cfg.ReadUnifiedAlertingSettings(f) require.NoError(t, err) assert.NotNil(t, cfg.UnifiedAlerting.Enabled) - assert.Equal(t, *cfg.UnifiedAlerting.Enabled, false) - assert.NotNil(t, AlertingEnabled) - assert.Equal(t, *AlertingEnabled, true) + assert.True(t, *cfg.UnifiedAlerting.Enabled) + assert.Nil(t, AlertingEnabled) }, }, { diff --git a/pkg/setting/setting_unified_alerting.go b/pkg/setting/setting_unified_alerting.go index 72cbc6eeb09..96bdf6bdc0c 100644 --- a/pkg/setting/setting_unified_alerting.go +++ b/pkg/setting/setting_unified_alerting.go @@ -95,47 +95,50 @@ func (u *UnifiedAlertingSettings) IsEnabled() bool { return u.Enabled == nil || *u.Enabled } +// readUnifiedAlertingEnabledSettings reads the settings for unified alerting. +// It returns a non-nil bool and a nil error when unified alerting is enabled either +// because it has been enabled in the settings or by default. It returns nil and +// a non-nil error both unified alerting and legacy alerting are enabled at the same time. func (cfg *Cfg) readUnifiedAlertingEnabledSetting(section *ini.Section) (*bool, error) { - enabled, err := section.Key("enabled").Bool() - // the unified alerting is not enabled by default. First, check the feature flag + // At present an invalid value is considered the same as no value. This means that a + // spelling mistake in the string "false" could enable unified alerting rather + // than disable it. This issue can be found here + unifiedAlerting, err := section.Key("enabled").Bool() if err != nil { // TODO: Remove in Grafana v9 if cfg.IsFeatureToggleEnabled("ngalert") { cfg.Logger.Warn("ngalert feature flag is deprecated: use unified alerting enabled setting instead") - enabled = true - // feature flag overrides the legacy alerting setting. + // feature flag overrides the legacy alerting setting legacyAlerting := false AlertingEnabled = &legacyAlerting - return &enabled, nil + unifiedAlerting = true + return &unifiedAlerting, nil } - if IsEnterprise { - enabled = false - if AlertingEnabled == nil { - legacyEnabled := true - AlertingEnabled = &legacyEnabled - } - return &enabled, nil + + // if legacy alerting has not been configured then enable unified alerting + if AlertingEnabled == nil { + unifiedAlerting = true + return &unifiedAlerting, nil } - // next, check whether legacy flag is set - if AlertingEnabled != nil && !*AlertingEnabled { - enabled = true - return &enabled, nil // if legacy alerting is explicitly disabled, enable the unified alerting by default. - } - // NOTE: If the enabled flag is still not defined, the final decision is made during migration (see sqlstore.migrations.ualert.CheckUnifiedAlertingEnabledByDefault). - cfg.Logger.Info("The state of unified alerting is still not defined. The decision will be made during as we run the database migrations") - return nil, nil // the flag is not defined + + // enable unified alerting and disable legacy alerting + legacyAlerting := false + AlertingEnabled = &legacyAlerting + unifiedAlerting = true + return &unifiedAlerting, nil } - // If unified alerting is defined explicitly as well as legacy alerting and both are enabled, return error. - if enabled && AlertingEnabled != nil && *AlertingEnabled { - return nil, errors.New("both legacy and Grafana 8 Alerts are enabled. Disable one of them and restart") + // If both legacy and unified alerting are enabled then return an error + if AlertingEnabled != nil && *AlertingEnabled && unifiedAlerting { + return nil, errors.New("legacy and unified alerting cannot both be enabled at the same time, please disable one of them and restart Grafana") } - // if legacy alerting is not defined but unified is determined then update the legacy with inverted value + if AlertingEnabled == nil { - legacyEnabled := !enabled - AlertingEnabled = &legacyEnabled + legacyAlerting := !unifiedAlerting + AlertingEnabled = &legacyAlerting } - return &enabled, nil + + return &unifiedAlerting, nil } // ReadUnifiedAlertingSettings reads both the `unified_alerting` and `alerting` sections of the configuration while preferring configuration the `alerting` section.