Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Alerting: Add clean_upgrade config and deprecate force_migration #78324

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Alerting: Add clean_upgrade config and deprecate force_migration #78324

merged 5 commits into from
Nov 30, 2023

Conversation

JacobsonMT
Copy link
Member

@JacobsonMT JacobsonMT commented Nov 17, 2023 •
edited

Note: #78369 is a prerequisite for this PR.

What is this feature?

This change has two main components:

  • Upgrading to UA and rolling back will no longer delete any data by default. Instead, each set of tables will remain unchanged when switching between legacy and UA. As such, the force_migration config has been deprecated and no extra configuration is required to roll back to legacy anymore.
  • If you want to re-perform the upgrade from scratch, you should no longer set force_migration when rolling back. Instead, you set the new clean_upgrade when upgrading (though force_migration will continue to work as-is for now):
[unified_alerting.upgrade]
clean_upgrade=true

This config takes effect when upgrading from legacy->UA and, if enabled, will first revert all UA data before performing the upgrade from a clean slate.

Note: Similar to force_migration, clean_upgrade should be kept false when not needed as it may cause unintended data-loss if left enabled.

Why do we need this feature?

This is a general improvement and precursor to upcoming migration dry-run functionality.

Who is this feature for?

Users migrating from legacy alerting to Grafana Alerting (UA).

Please check that:

@JacobsonMT JacobsonMT requested review from rwwiv, yuri-tceretian, grobinson-grafana, zserge, nikimanoledaki and yangkb09 and removed request for a team November 17, 2023 12:17
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Nov 17, 2023
chri2547
chri2547 reviewed Nov 17, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! I have reviewed the configuration docs updates and will let @brendamuir take a look at the alerting docs updates.

I made some suggestions. Please review before you accept to ensure I have not changed the technical meaning. Thanks!

@JacobsonMT JacobsonMT mentioned this pull request Nov 20, 2023
Merged
1 task
@JacobsonMT JacobsonMT changed the base branch from main to jacobsonmt/org-level-migration-status November 20, 2023 04:40
@JacobsonMT
Copy link
Member Author

Moved some of the prerequisite but out-of-scope code changes into #78369

brendamuir
brendamuir approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@brendamuir brendamuir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

chri2547
chri2547 approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc changes look fine, thank you!

@JacobsonMT JacobsonMT changed the title Alerting: Replace force_migration config with clean_upgrade, don't delete data on UA rollback Alerting: Replace force_migration config with clean_upgrade Nov 20, 2023
gotjosh
gotjosh reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the old flag around and introduce the new one. When Grafana 11 comes, then we can remove the old one - otherwise LGTM.

@JacobsonMT
Copy link
Member Author

I think we should keep the old flag around and introduce the new one. When Grafana 11 comes, then we can remove the old one - otherwise LGTM.

@gotjosh Added force_migration back in a deprecated state: b7a95ab

@brendamuir Added the docs section back in setup-grafana/configure-grafana/ with clearer descriptions but didn't add it back to the alerting/set-up/migrating-alerts/ as it's deprecated.

image

@JacobsonMT JacobsonMT changed the title Alerting: Replace force_migration config with clean_upgrade Alerting: Add clean_upgrade config and deprecate force_migration Nov 23, 2023
gotjosh
gotjosh approved these changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed through it and overall LGTM, I'm not sure I follow all the pieces of the logic in migration/service.go as I'm not very familiar with the current state of the migration.

I'd advise you seek a second approver to make sure the logic is sound.

@JacobsonMT JacobsonMT added the area/alerting/migration Issues relating to legacy alerting migration label Nov 24, 2023
yuri-tceretian
yuri-tceretian approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. just a few minor comments.

Re deprecation vs removal: Personally, I am in favor of removing it at all.

  1. It is a transactional flag and is not supposed to be other than default. This flag is only used during upgrade, which is not something that happens regularly.
  2. Having the deprecated flag means that you have to support it anyway, and therefore there is a possibility of unexpected combinations (both true, one is true etc), which creates multiple upgrade paths and additional complexity in troubleshooting of potential problems.
  3. Deprecated is also strange because, afaiu, the entire migration package will be gone in G11... i.e. clean_upgrade and the upgrade section is also kinda "deprecated"

pkg/services/ngalert/migration/store/database.go Outdated Show resolved Hide resolved
pkg/services/ngalert/migration/migration_test.go Outdated
},
isMigrationRun: true,
expected: true,
expectedErr: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we still want to mark it as deprecated, then clean_upgrade needs its own set of tests when force is true and false. Same in service_test.go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force_migration and clean_upgrade have their own set of tests

pkg/services/ngalert/migration/service.go Outdated Show resolved Hide resolved
pkg/services/ngalert/migration/service_test.go
@@ -279,7 +281,7 @@ func TestServiceRevert(t *testing.T) {
}
})

t.Run("ForceMigration story", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep the "force" flag, then I would recommend adding a new set of tests and keeping existing ones untouched.

@yuri-tceretian
Copy link
Contributor

I think we can improve readability by branching Run by either UA.enabled or migrated flags, and extract logic into separate methods for both cases. This way the code flow will be much clearer to understand.

@JacobsonMT
Copy link
Member Author

JacobsonMT commented Nov 29, 2023 •
edited

I think we can improve readability by branching Run by either UA.enabled or migrated flags, and extract logic into separate methods for both cases. This way the code flow will be much clearer to understand.

Agreed, improved the readability of the migration logic in base branch here: #78369 (comment). The change in this PR is now extremely simple and clear.

Base automatically changed from jacobsonmt/org-level-migration-status to main November 30, 2023 15:26
JacobsonMT and others added 4 commits November 30, 2023 10:32
@JacobsonMT
Alerting: Introduce clean_upgrade config, don't delete data on rollback
ccf30e7 
Upgrading to UA and rolling back no longer deletes any data. Instead, we remove
 the force_migration in favour of a new config called clean_upgrade.

If clean_upgrade is set to true when upgrading from legacy alerting to Unified
Alerting, grafana will first delete all existing Unified Alerting resources,
thus re-upgrading all organizations from scratch. If false or unset,
organizations that have previously upgraded will not lose their existing Unified
 Alerting data when switching between legacy and Unified Alerting.

 Similar to force_migration, it should be kept false when not needed as it may
 cause unintended data-loss if left enabled.
@JacobsonMT
Fix docs linting
7a86c07
@JacobsonMT @chri2547
Apply suggestions from code review
8914e12 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@JacobsonMT
Add force_migration back in a deprecated state
c2e6d17 
To be removed in v11
@JacobsonMT JacobsonMT merged commit 5a80962 into main Nov 30, 2023
13 checks passed
@JacobsonMT JacobsonMT deleted the jacobsonmt/clean-upgrade branch November 30, 2023 16:01
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuri-tceretian yuri-tceretian yuri-tceretian approved these changes

@gotjosh gotjosh gotjosh approved these changes

@chri2547 chri2547 chri2547 approved these changes

@brendamuir brendamuir brendamuir approved these changes

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

@rwwiv rwwiv Awaiting requested review from rwwiv rwwiv is a code owner automatically assigned from grafana/alerting-backend-product

@grobinson-grafana grobinson-grafana Awaiting requested review from grobinson-grafana grobinson-grafana is a code owner automatically assigned from grafana/alerting-backend-product

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@nikimanoledaki nikimanoledaki Awaiting requested review from nikimanoledaki nikimanoledaki was automatically assigned from grafana/backend-platform

@yangkb09 yangkb09 Awaiting requested review from yangkb09 yangkb09 was automatically assigned from grafana/backend-platform

Assignees
No one assigned
Labels
add to changelog area/alerting/migration Issues relating to legacy alerting migration area/alerting Grafana Alerting area/backend/db/migration area/backend no-backport Skip backport of PR type/docs
Projects
Alerting
Archived in project
Milestone
10.2.3
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JacobsonMT @yuri-tceretian @gotjosh @chri2547 @brendamuir @aangelisc