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.

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

[ResponseOps][Alerting] Unable to upgrade to 8.4.0 when I have snoozed rules #139427

Merged
merged 6 commits into from Aug 25, 2022

Conversation

doakalexi
Copy link
Contributor

@doakalexi doakalexi commented Aug 24, 2022

Resolves #139412

Summary

Added migration for 8.4.1 to remove isSnoozedUntil

Checklist

@doakalexi doakalexi changed the title Adding migration [ResponseOps][Alerting] Unable to upgrade to 8.4.0 when I have snoozed rules Aug 24, 2022
@doakalexi doakalexi added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:fix labels Aug 24, 2022
@doakalexi doakalexi marked this pull request as ready for review August 24, 2022 19:57
@doakalexi doakalexi requested a review from a team as a code owner August 24, 2022 19:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM! Left one comment about another unit test but otherwise looks great.

I verified this locally by:

  • Running 8.2 and creating some rules:
    • 8.2 rule never snoozed - didn't touch after creating
    • 8.2 rule snoozed indefinitely in 8.2 - snoozed indefinitely after creating
    • 8.2 rule snoozed for 1 day in 8.2 - snoozed for 1 day after creating
    • 8.2 rule snoozed then unsnoozed in 8.2 - snoozed then unsnoozed after creating
    • 8.2 rule snoozed indefinitely in 8.3 - didn't touch after creating
    • 8.2 rule snoozed for 1 day in 8.3 - didn't touch after creating
    • 8.2 rule snoozed then unsnoozed in 8.3 - didn't touch after creating
  • Upgraded to 8.3, verified the rules ran successfully then:
    • snoozed the 8.2 rule snoozed indefinitely in 8.3 rule indefinitely
    • snoozed the 8.2 rule snoozed for 1 day in 8.3 rule for 1 day
    • snoozed then unsnoozed the 8.2 rule snoozed then unsnoozed in 8.3 rule
    • then created:
      • 8.3 rule never snoozed - didn't touch after creating
      • 8.3 rule snoozed indefinitely in 8.3 - snoozed indefinitely after creating
      • 8.3 rule snoozed for 1 day in 8.3 - snoozed for 1 day after creating
      • 8.3 rule snoozed then unsnoozed in 8.3 - snoozed then unsnoozed after creating
  • Upgraded to 8.4 and saw the migration fail due to these rules:
    • 8.2 rule snoozed for 1 day in 8.3
    • 8.2 rule snoozed for 1 day in 8.2
    • 8.3 rule snoozed for 1 day in 8.3
  • Ran this PR and saw the migration succeed. Snooze end date showed up correctly in the UI and saw the rules all run successfully.

return {
...doc,
attributes: {
...(omit(doc.attributes, ['isSnoozedUntil']) as RawRule),
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the result of this fix is that a rule that was snoozed in 8.3 will no longer be snoozed in 8.4.1.

Given the urgency I'm 👍 on that, but we should probably make that clear in the 8.4.1 release notes.

@gmmorris
Copy link
Contributor

@Zacqary as you know the snooze functionality better than anyone - can you help us verify this?

I'm wondering what will happen to a rule that removed the isSnoozedUntil field but still retains a snoozeSchedule field. 🤔
Will the snooze still work under the hood but the UX will no longer reflect that it's snoozed?

@ymao1
Copy link
Contributor

ymao1 commented Aug 25, 2022

I believe the result of this fix is that a rule that was snoozed in 8.3 will no longer be snoozed in 8.4.1.

@gmmorris When I tested this PR locally, I could still see the snooze in the UI after successful upgrade to 8.4. The PR that removed the mapping also calculates it on the fly, so I believe the snooze functionality should still work. Hopefully @Zacqary can confirm :)

@mikecote mikecote self-requested a review August 25, 2022 13:48
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM! I can confirm the failed migraiton scenario no longer happens with this fix.

@mikecote mikecote added auto-backport Deprecated: Automatically backport this PR after it's merged v8.4.1 labels Aug 25, 2022
@doakalexi doakalexi merged commit 8991e04 into elastic:main Aug 25, 2022
@Zacqary
Copy link
Contributor

Zacqary commented Aug 25, 2022

Will the snooze still work under the hood but the UX will no longer reflect that it's snoozed?

isSnoozedUntil should now be a field that's only added by the API and calculated when the user requests it, it's no longer present in the saved object at all. So no, there shouldn't be any UX issues.

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.4 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 139427

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@doakalexi
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

doakalexi added a commit to doakalexi/kibana that referenced this pull request Aug 25, 2022
…d rules (elastic#139427)

* Adding migration

* Removing timer

* Adding functional test

* Adding another test

(cherry picked from commit 8991e04)

# Conflicts:
#	x-pack/plugins/alerting/server/saved_objects/migrations.ts
#	x-pack/test/alerting_api_integration/spaces_only/tests/alerting/migrations.ts
#	x-pack/test/functional/es_archives/alerts/data.json
doakalexi added a commit that referenced this pull request Aug 25, 2022
…d rules (#139427) (#139486)

* Adding migration

* Removing timer

* Adding functional test

* Adding another test

(cherry picked from commit 8991e04)

# Conflicts:
#	x-pack/plugins/alerting/server/saved_objects/migrations.ts
#	x-pack/test/alerting_api_integration/spaces_only/tests/alerting/migrations.ts
#	x-pack/test/functional/es_archives/alerts/data.json
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
…d rules (elastic#139427)

* Adding migration

* Removing timer

* Adding functional test

* Adding another test
@doakalexi doakalexi deleted the alerting/8.4.1-migration branch December 6, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.4.1
Development

Successfully merging this pull request may close these issues.

Unable to upgrade to 8.4.0 from 8.3.* when I have snoozed rules
8 participants