[Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight #63093

gmmorris · 2020-04-09T09:43:58Z

Summary

Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings.

Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks).

Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted).

Closes #42477

Checklist

Delete any items that are not applicable to this PR.

~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

…t task

elasticmachine · 2020-04-09T09:44:00Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

YulNaumenko

LGTM

mikecote

LGTM! Just one comment

x-pack/plugins/alerting/server/task_runner/task_runner.ts

pmuellr

LGTM

x-pack/plugins/task_manager/server/task_pool.ts

x-pack/plugins/alerting/server/task_runner/task_runner.ts

mikecote · 2020-04-13T19:05:52Z

@elasticmachine merge upstream

* master: document code splitting for client code (elastic#62593) Escape single quotes surrounded by double quotes (elastic#63229) [Endpoint] Update cli mapping to match endpoint package (elastic#63372) update in-app links to metricbeat configuration docs (elastic#63295) investigation notes field (documentation / metadata) (elastic#63386) [Maps] fix bug where toggling Scaling type does not re-fetch data (elastic#63326) [Alerting] set correct parameter for unauthented email action (elastic#63086) [Telemetry] force staging urls in tests (elastic#63356) Migrate legacy maps service to NP & update refs (elastic#60942) Fix task manager query to return tasks to retry (elastic#63360) [Endpoint] Policy list support for URL pagination state (elastic#63291) [Canvas] Migrate saved object mappings and migrations to Kibana Platform (elastic#58891) [DOCS] Add ILM tutorial (elastic#59502) [Maps] Add SOURCE_TYPES enumeration (elastic#62975) [Maps] update geospatial filters to use geo_shape query for geo_point fields (elastic#62966) Move away from npStart for embeddables in canvas (elastic#62680)

* master: [Event Log] Adds namespace into save objects (elastic#62974)

mikecote

Code changes LGTM

pmuellr

still LGTM - made a note about trying to get that SO helper function enhanced so we don't have to grep through the error message

pmuellr · 2020-04-14T18:21:00Z

x-pack/plugins/alerting/server/lib/is_alert_not_found_error.ts

+import { SavedObjectsErrorHelpers } from '../../../../../src/core/server';
+
+export function isAlertSavedObjectNotFoundError(err: Error, alertId: string) {
+  return SavedObjectsErrorHelpers.isNotFoundError(err) && `${err}`.includes(alertId);


This feels yucky enough that I feel like we should ask for a better helper - like being able to pass alertId into that isNotFoundError() function, rather than text searching the error message ourselves. The alertId values are UUIDs today, which seems safe enough to test for, but who knows what tomorrow holds - action id's can now be user-specified via pre-configured actions ...

Yeah, that's fair, but as there seems to be pressure to get this backported to 7.7, I think we'll have to defer that.

x-pack/plugins/task_manager/server/task_pool.ts

* master: (29 commits) Add test:jest_integration npm script (elastic#62938) [data.search.aggs] Remove service getters from agg types (AggConfig part) (elastic#62548) [Discover] Fix broken setting of bucketInterval (elastic#62939) Disable adding conditions when in alert management context. (elastic#63514) [Alerting] fixes to allow pre-configured actions to be executed (elastic#63432) adding useMemo (elastic#63504) [Maps] fix double fetch when filter pill is added (elastic#63024) [Lens] Fix missing formatting bug in "break down by" (elastic#63288) [SIEM] [Cases] Removed double pasted line (elastic#63507) [Reporting] Improve functional test steps (elastic#63259) [SIEM][CASE] Tests for server's configuration API (elastic#63099) [SIEM] [Cases] Case container unit tests (elastic#63376) [ML] Improving parsing of large uploaded files (elastic#62970) [ML] Listing global calendars on the job management page (elastic#63124) [Ingest][Endpoint] Add Ingest rest api response types for use in Endpoint (elastic#63373) Add help text to form fields (elastic#63165) [ML] Converts utils Mocha tests to Jest (elastic#63132) [Metrics UI] Refactor With* containers to hooks (elastic#59503) [NP] Migrate logstash server side code to NP (elastic#63135) Clicking cancel in saved query save modal doesn't close it (elastic#62774) ...

kibanamachine · 2020-04-15T11:27:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c5cdea9

History

💔 Build #40885 failed 522bd13
💔 Build #40692 failed 435b487
💔 Build #40623 failed 6813fa9
💔 Build #40595 failed 2eb2639
💔 Build #40488 failed 9353f75

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

… being deleted mid flight (elastic#63093) Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings. Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks). Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted).

gmmorris · 2020-04-15T13:18:18Z

We didn't make it to 7.7, so this will be backported for the 7.7.1 patch.
The workaround for 7.7 would be to manually delete these tasks, so hopefully that'll be good enough for the duration.

… being deleted mid flight (#63093) (#63564) Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings. Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks). Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted). Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

… being deleted mid flight (#63093) Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings. Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks). Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted).

kibanamachine · 2020-04-16T13:18:45Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

… being deleted mid flight (#63093) (#63569) Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings. Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks). Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted). Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…t object being deleted mid flight (#63093) (#63569)" This reverts commit 3cdf259.

…t object being deleted mid flight (#63093) (#63569)" (#63820) This reverts commit 3cdf259.

… being deleted mid flight (elastic#63093) Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings. Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks). Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted). # Conflicts: # x-pack/plugins/alerting/server/task_runner/task_runner.test.ts # x-pack/plugins/alerting/server/task_runner/task_runner.ts

… being deleted mid flight (#63093) (#66570) Detects if a task run failed due to the task SO being deleted mid flight and if so writes debug logs instead of warnings. Detects if an Alerting task run failed due to the alert SO being deleted mid flight of the task and if so ensures the task doesn't reschedule itself (as it usually would with other types of tasks). Ensures that the operation of deleting or disabling an Alert won't fail if it fails to delete an already deleted task (a task might preemptively self delete if its underlying alert object was deleted, even if the overall delete operation wasn't deleted). # Conflicts: # x-pack/plugins/alerting/server/task_runner/task_runner.test.ts # x-pack/plugins/alerting/server/task_runner/task_runner.ts

gmmorris added 2 commits April 9, 2020 10:39

swallow 404 errors from Saved Object when they happen for an in fligh…

af4fa92

…t task

debug instead of swallow on 404s

2cf8744

gmmorris added Feature:Alerting v8.0.0 v7.7.0 Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.8.0 labels Apr 9, 2020

gmmorris requested a review from a team as a code owner April 9, 2020 09:43

gmmorris added the release_note:skip Skip the PR/issue when compiling release notes label Apr 9, 2020

gmmorris mentioned this pull request Apr 9, 2020

extraneous error/warning messages when deleting alerts #42477

Closed

YulNaumenko approved these changes Apr 9, 2020

View reviewed changes

avoid rescheduling alert task if it fails to find the alert

4d9dbba

mikecote approved these changes Apr 9, 2020

View reviewed changes

x-pack/plugins/alerting/server/task_runner/task_runner.ts Show resolved Hide resolved

pmuellr approved these changes Apr 10, 2020

View reviewed changes

x-pack/plugins/task_manager/server/task_pool.ts Outdated Show resolved Hide resolved

x-pack/plugins/alerting/server/task_runner/task_runner.ts Show resolved Hide resolved

elasticmachine and others added 6 commits April 13, 2020 15:05

Merge branch 'master' into alerting/deleted-alert-error

9353f75

avoid reliance on Boom and instead relly on SO helpers

876dea5

removed unused import

2eb2639

only log mesage when it isnt failure to find alert object

c50ec7d

Merge branch 'master' into alerting/deleted-alert-error

6813fa9

* master: [Event Log] Adds namespace into save objects (elastic#62974)

gmmorris changed the title ~~[Alerting] Avoid logging a warning when an in flight Alert task is deleted~~ [Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight Apr 14, 2020

handle 404s when removing a task behind an alert

435b487

mikecote approved these changes Apr 14, 2020

View reviewed changes

pmuellr approved these changes Apr 14, 2020

View reviewed changes

gmmorris added 3 commits April 15, 2020 09:48

fixed flaky date behaviour

522bd13

removed unused import

c5cdea9

gmmorris merged commit d1134c5 into elastic:master Apr 15, 2020

gmmorris mentioned this pull request Apr 15, 2020

[7.x] [Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight (#63093) #63564

Merged

gmmorris mentioned this pull request Apr 15, 2020

[7.7] [Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight (#63093) #63569

Merged

gmmorris added v7.7.1 and removed v7.7.0 labels Apr 15, 2020

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 16, 2020

mikecote added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 16, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 17, 2020

gmmorris added a commit that referenced this pull request Apr 17, 2020

Revert "[Alerting] Handle when an Alerting Task fails due to its Aler…

c670dab

…t object being deleted mid flight (#63093) (#63569)" This reverts commit 3cdf259.

gmmorris added a commit that referenced this pull request Apr 17, 2020

Revert "[Alerting] Handle when an Alerting Task fails due to its Aler…

86249f2

…t object being deleted mid flight (#63093) (#63569)" (#63820) This reverts commit 3cdf259.

gmmorris mentioned this pull request May 14, 2020

[7.7] [Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight (#63093) #66570

Merged

mikecote mentioned this pull request Oct 30, 2020

[Task Manager] Changed alerts schedule logic to use Task Manager internals #80149

Merged

3 tasks

pmuellr mentioned this pull request Jul 14, 2021

[Alerting] Added retry logic in the alerting task run during update rule that fails with Saved object not found error. #103925

Closed

[Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight #63093

[Alerting] Handle when an Alerting Task fails due to its Alert object being deleted mid flight #63093

Uh oh!

Conversation

gmmorris commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Apr 9, 2020

Uh oh!

YulNaumenko left a comment

Choose a reason for hiding this comment

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikecote commented Apr 13, 2020

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

gmmorris Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Apr 15, 2020

💚 Build Succeeded

History

Uh oh!

gmmorris commented Apr 15, 2020

Uh oh!

kibanamachine commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gmmorris commented Apr 9, 2020 •

edited

Loading