Alerting: Move legacy alert migration from sqlstore migration to service #72702

JacobsonMT · 2023-08-01T22:17:16Z

This PR moves the legacy alert migration out of the sqlstore db migrations and into ngalert.

There are multiple reasons behind this change, I'll do my best to explain them here:

Legacy alert migration doesn't fit the definition of a sqlstore migration

Initially, this was a natural place for the migration code to live. It does, in fact, migrate database schema and contents. However, it breaks two key requirements of sqlstore migrations:

It is not guaranteed to run at a specific point in time relative to other migrations.
It changes over time.

These two differences are caused by the fact that the legacy migration is currently optional. Users may decide when to migrate, or even rollback the migration and try again later. This optional status also means we were able to improve the migration over time, thus causing the migration to change.

This has contributed to numerous bugs in the past, either directly and indirectly. To name a few recent ones:

Legacy provisioned alerts cannot currently be migrated in stateless grafana deployments

Since sqlstore migrations run before any provisioning, this means that the migration code does not see that legacy provisioned alerts exist.

By moving this into ngalert.Run() we allow a followup PR to run dashboard provisioning before we start the legacy migration.

The migration code is saving vendored models to the database

This makes some sense if the migration were a true sqlstore migration where followup migrations would update the vendored models to more current ngalert versions. This is not currently what is happening. Instead, we periodically update the vendored models in the main migration in an attempt to keep the vendor in sync with ngalert models.

This is prone to drift and is not obvious in the context of sqlstore migrations.

With the migration code part of the ngalert package, a followup PR can ensure it uses the real ngalert models and drift should be caught alongside any other ngalert code changes we do.

This unlocks features that were not easily done before

If the migration code is in ngalert as a service, then we are free to use it in new/existing endpoints. This opens up interesting feature possibilities. For example (not saying we will do all of these and some are mutually exclusive):

No longer migrate legacy alerts on startup, instead make it an endpoint with UI.
- This button could show you the yaml of all the UA resources that would be created after the migration. The yaml could then be imported into another instances for testing in dev, for example.
- Or maybe, the UI will let you live toggle between legacy and UA as we migrate/revert over and again as you check out the differences.
Create an endpoint that can take legacy dashboard yaml containing alerts and migrate them ad-hoc directly into UA (or maybe convert to UA yaml)
Migrate provisioned legacy alerts to provisioned UA alerts with a special provenance (provenance legacy_migration or something). As opposed to most alerts, these legacy_migration provenance alerts can be overwritten on a subsequent migration run. This could allow us to let users keep their as-code provisioned alerts in legacy format for a while if desired.

Edit:

For the purposes of keeping the change self-contained in a single commit, this PR has been combined with a 2nd PR to replace the vendored models in the migration with their equivalent ngalert models. It also replaces the raw SQL selects and inserts with service calls:

Dashboard Alerts: 9f5b6b2
- alertRule -> ngmodels.AlertRule
Alertmanager Configuration: 9f5b6b2
- PostableUserConfig, PostableApiAlertingConfig, PostableApiReceiver, Route, ObjectMatchers, PostableGrafanaReceiver to their apimodels equivalents of the same name.
Secure settings encryption: 149ec38
- util.Encrypt -> secrets.Service.Encrypt
Legacy notification channels: 42dcb80
- notificationChannel -> legacymodels.AlertNotification
Dashboards:
- dashboard -> dashboards.Dashboard d023023
- Raw SQL -> services a41f32d
Datasources: 53b9e47
dsUIDLookup -> datasources.CacheService

Some gaps in the testing suite are filled:

Migration of alert rules: verifying that the actual data model (queries, conditions) are correct 9a7cfa9
Secure settings migration: verifying that secure fields remain encrypted for all available notifiers and certain fields migrate from plain text to encrypted secure settings correctly e7d3993

Replacing the checks for custom dashboard ACLs will be done in a separate targeted PR as it adds functionality instead of moving it around.

pkg/services/ngalert/migration/service.go

pkg/services/ngalert/migration/testing.go

pkg/services/ngalert/migration/service.go

github-actions · 2023-09-03T01:47:26Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

JacobsonMT

Some review notes

JacobsonMT · 2023-09-06T22:27:32Z

pkg/services/ngalert/migration/alert_rule.go

-	NamespaceUID    string `xorm:"namespace_uid"`
+	UID             string  `xorm:"uid"`
+	NamespaceUID    string  `xorm:"namespace_uid"`
+	DashboardUID    *string `xorm:"dashboard_uid"`


Added because AddDashboardUIDPanelIDMigration used to be conditional on migration (runs and reverts with migration). Since we cna't rely on it anymore the migration service needs to populate the fields. See 2fdf1d0

JacobsonMT · 2023-09-06T22:32:21Z

pkg/services/sqlstore/migrations/ualert/ualert.go

-
+// FixEarlyMigration fixes UA configs created before 8.2 with org_id=0 and moves some files like __default__.tmpl.
+// The only use of this migration is when a user enabled ng-alerting before 8.2.
+func FixEarlyMigration(mg *migrator.Migrator) {


This used to be called RerunDashAlertMigration which was wholly confusing as it didn't actually rerun the migration. Renamed to better represent what it was doing, and removed the condition on migration.

The code is not added to the migration service as it's no longer relevant. See 60d35fb

JacobsonMT · 2023-09-06T22:35:06Z

pkg/services/sqlstore/migrations/ualert/tables.go

@@ -492,9 +492,6 @@ func addAlertImageMigrations(mg *migrator.Migrator) {
 }

 func extractAlertmanagerConfigurationHistoryMigration(mg *migrator.Migrator) {
-	if !mg.Cfg.UnifiedAlerting.IsEnabled() {


This always ran before the migration, so it never did anything to new
legacy migrations anyways.

JacobsonMT · 2023-09-06T22:42:12Z

pkg/services/sqlstore/migrations/ualert/rule_group_index_migration.go

+
+// UpdateRuleGroupIndexMigration updates a new field rule_group_index for alert rules that belong to a group with more than 1 alert.
+func UpdateRuleGroupIndexMigration(mg *migrator.Migrator) {
+	mg.AddMigration("update group index for alert rules", &updateRulesOrderInGroup{})


Moved as-is from ualert along with the vendored structs it needed from elsewhere in the migration model.

The only change is here is removing the condition to only add if UA is enabled.

JacobsonMT · 2023-09-06T22:45:59Z

pkg/services/sqlstore/migrations/ualert/ualert.go

-}
-
-// CreateDefaultFoldersForAlertingMigration creates a folder dedicated for alerting if no folders exist
-func CreateDefaultFoldersForAlertingMigration(mg *migrator.Migrator) {


This migration was removed entirely as it's no longer necessary.

The migration service will create the general alerting folder when ran.

Creating a new first folder via the rule creation page is simpler than when this migration was created.

JacobsonMT · 2023-09-06T22:46:40Z

pkg/services/ngalert/tests/fakes/kvstore.go

@@ -0,0 +1,102 @@
+package fakes


Moved as-is from notifier.testing so it can be used for migration tests.

JacobsonMT · 2023-09-06T22:48:21Z

pkg/services/sqlstore/migrations/ualert/migration_service_migration.go

+var migratedKey = "migrated"
+
+// MigrationServiceMigration moves the legacy alert migration status from the migration log to kvstore.
+func MigrationServiceMigration(mg *migrator.Migrator) {


This is necessary so that we retain the current migration state between when sqlstore was in charge of UA migration and now. See 79781ae

JacobsonMT

Review notes part 2

JacobsonMT · 2023-09-06T23:02:38Z

pkg/services/ngalert/migration/permissions.go

@@ -0,0 +1,300 @@
+package migration


This is moved nearly 100% as-is from sqstore, not sure why it's not detected as a move. The only changes are to use the session through the context instead of through folderHelper. Browser diff: https://www.diffchecker.com/ldxQCY9E/

JacobsonMT · 2023-09-06T23:05:18Z

pkg/services/sqlstore/migrations/external_alertmanagers.go

+type secureJsonData map[string][]byte
+
+// getEncryptedJsonData returns map where all keys are encrypted.
+func getEncryptedJsonData(sjd map[string]string, log log.Logger) secureJsonData {


Leftover from securejsondata.go

Leaves custom ACL logic for followup as it needs to be completely rewritten for rbac.

This is a needed for upcoming work, so we do it separately for clarity.

Setting dashboard.created_by to -8 as a way to track which folders were created by the migration is no longer possible. Instead, we store the newly created folders uids in the kvstore and use it on revert.

If migrationLock feature flag was enabled and the max db connections was 2 or less, a deadlock would occur. This is because the GetMigrationLog() method spawns a new session and when migrationLock is used, the migration is executed in a nested transaction. Together, these deplete the connection pool.

JacobsonMT · 2023-10-12T00:01:19Z

/deploy-to-hg

ephemeral-instances-bot · 2023-10-12T00:01:56Z

Preparing your instance. A comment containing your instance's url will be added to this PR when the instance is ready.
Your instance will be ready in ~10 minutes.
Check the GitHub actions tab to follow the workflow progress
Slack channel: #proj-ephemeral-hg-instances
Building instance with jacobsonmt/migration_as_service oss branch and main enterprise branch. How to choose a branch

ephemeral-instances-bot · 2023-10-12T00:13:43Z

Your instance can be accessed at: https://ephemeral1511182172702jacobso.grafana-dev.net
The instance is not using the CDN assets.
How to access / How to update instance config / How to build a specific branch

yuri-tceretian

LGTM

JacobsonMT added area/alerting Grafana Alerting add to changelog area/backend/db/migration no-backport Skip backport of PR labels Aug 1, 2023

JacobsonMT added this to the 10.2.x milestone Aug 1, 2023

grafana-pr-automation bot added the area/backend label Aug 1, 2023

yuri-tceretian reviewed Aug 3, 2023

View reviewed changes

pkg/services/ngalert/migration/service.go Outdated Show resolved Hide resolved

yuri-tceretian reviewed Aug 3, 2023

View reviewed changes

pkg/services/ngalert/migration/service.go Outdated Show resolved Hide resolved

yuri-tceretian reviewed Aug 3, 2023

View reviewed changes

pkg/services/ngalert/migration/testing.go Outdated Show resolved Hide resolved

yuri-tceretian reviewed Aug 3, 2023

View reviewed changes

pkg/services/ngalert/migration/service.go Show resolved Hide resolved

github-actions bot added the stale Issue with no recent activity label Sep 3, 2023

JacobsonMT force-pushed the jacobsonmt/migration_as_service branch from d36812c to 73a06ff Compare September 6, 2023 05:27

JacobsonMT commented Sep 6, 2023

View reviewed changes

JacobsonMT marked this pull request as ready for review September 6, 2023 22:50

JacobsonMT requested a review from a team as a code owner September 6, 2023 22:50

JacobsonMT requested a review from a team September 6, 2023 22:50

JacobsonMT requested review from a team as code owners September 6, 2023 22:50

JacobsonMT requested review from rwwiv, yuri-tceretian, grobinson-grafana, papagian, zserge and suntala and removed request for a team September 6, 2023 22:50

JacobsonMT commented Sep 6, 2023

View reviewed changes

github-actions bot removed the stale Issue with no recent activity label Sep 7, 2023

JacobsonMT mentioned this pull request Sep 7, 2023

Alerting: Remove vendored models in migration service #74503

Merged

JacobsonMT added 18 commits October 12, 2023 00:49

Replace vendored alertRule and PostableUserConfig with ngalert versions

bc94bff

Improve testing for migration of channel secure settings

64cc071

Replace vendored notificationChannel with AlertNotification

942c227

Encrypt migrated secureSettings with secrets.Service

fd293f2

Replace vendored dashboard with dashboards.Dashboard

894d821

Improve testing for creating new folders when migrating

ad9f4d3

Replace vendored dashboard & sql with dashboards.Dashboard and services

0d80709

Leaves custom ACL logic for followup as it needs to be completely rewritten for rbac.

Replace raw sql datasource lookup with datasources.CacheService

4adb107

Linting

fa488e2

Revert accidental change

91f65f1

Assert accesscontrol.BackgroundUser back to user.SignedInUser

4d9a5e0

Fix regression: unknown datasource id errors instead of uses empty uid

5b29ac1

Remove vendored dashAlert

94ddfcc

Noop refactor - combine most external store access into migration/store

351d4ed

Refactor migration to be per org

45f2843

This is a needed for upcoming work, so we do it separately for clarity.

Keep track of new folders using kvstore instead of dashboard.created_by

2682382

Setting dashboard.created_by to -8 as a way to track which folders were created by the migration is no longer possible. Instead, we store the newly created folders uids in the kvstore and use it on revert.

Fix linting

c3cfcd6

JacobsonMT force-pushed the jacobsonmt/migration_as_service branch from 117aa81 to 137e6bd Compare October 11, 2023 23:54

grafana deleted a comment from ephemeral-instances-bot bot Oct 12, 2023

yuri-tceretian approved these changes Oct 12, 2023

View reviewed changes

JacobsonMT merged commit 82f3127 into main Oct 12, 2023
14 checks passed

JacobsonMT deleted the jacobsonmt/migration_as_service branch October 12, 2023 12:43

zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023

dnhn mentioned this pull request Oct 24, 2023

grafana 10.2.0 Homebrew/homebrew-core#152264

Closed

BrewTestBot mentioned this pull request Oct 25, 2023

grafana 10.2.0 Homebrew/homebrew-core#152321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alerting: Move legacy alert migration from sqlstore migration to service #72702

Alerting: Move legacy alert migration from sqlstore migration to service #72702

JacobsonMT commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Sep 3, 2023

JacobsonMT left a comment

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT left a comment

JacobsonMT Sep 6, 2023

JacobsonMT Sep 6, 2023

JacobsonMT commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

yuri-tceretian left a comment

Alerting: Move legacy alert migration from sqlstore migration to service #72702

Alerting: Move legacy alert migration from sqlstore migration to service #72702

Conversation

JacobsonMT commented Aug 1, 2023 • edited Loading

Legacy alert migration doesn't fit the definition of a sqlstore migration

Legacy provisioned alerts cannot currently be migrated in stateless grafana deployments

The migration code is saving vendored models to the database

This unlocks features that were not easily done before

github-actions bot commented Sep 3, 2023

JacobsonMT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobsonMT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobsonMT commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

ephemeral-instances-bot bot commented Oct 12, 2023

yuri-tceretian left a comment

Choose a reason for hiding this comment

JacobsonMT commented Aug 1, 2023 •

edited

Loading