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

Fix task manager query to return tasks to retry #63360

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Apr 13, 2020

This PR fixes task manager's InactiveTasks query to AND the filters in the must_not instead of OR which caused tasks with status running or claiming to never return.

Steps to reproduce issue:

  • Stop kibana while an alert executor is running (use timeouts and logs to tell you when to stop Kibana)
  • Restart kibana and lookup the alert task's retryAt
  • Wait for retryAt window to pass
  • Notice alert isn't executing anymore

@mikecote mikecote added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Task Manager v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8.0 labels Apr 13, 2020
@mikecote mikecote requested a review from a team as a code owner April 13, 2020 15:20
@mikecote mikecote self-assigned this Apr 13, 2020
@elasticmachine
Copy link
Contributor

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

@@ -54,15 +54,16 @@ export const IdleTaskWithExpiredRunAt: MustCondition<TermFilter | RangeFilter> =
},
};

export const InactiveTasks: MustNotCondition<TermFilter | RangeFilter> = {
// TODO: Fix query clauses to support this
export const InactiveTasks: BoolClauseWithAnyCondition<any> = {
Copy link
Contributor Author

@mikecote mikecote Apr 13, 2020

Choose a reason for hiding this comment

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

I did not manage to get query clause TS types working within a timebox, deferring to later.

@mikecote mikecote added this to In Progress in Make it Action Apr 13, 2020
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mikecote mikecote merged commit 0666dbd into elastic:master Apr 13, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Apr 13, 2020
* Fix task manager query to also return tasks to retry

* Fix failing jest tests
mikecote added a commit to mikecote/kibana that referenced this pull request Apr 13, 2020
* Fix task manager query to also return tasks to retry

* Fix failing jest tests
mikecote added a commit that referenced this pull request Apr 13, 2020
* Fix task manager query to also return tasks to retry

* Fix failing jest tests
mikecote added a commit that referenced this pull request Apr 13, 2020
* Fix task manager query to also return tasks to retry

* Fix failing jest tests
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 14, 2020
* master: (132 commits)
  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)
  Use MapInput type from Maps plugin (elastic#61539)
  Update to pagination for workpad and templates (elastic#62050)
  [SIEM] Fix AlertsTable id (elastic#63368)
  Consistent terminology around cypress test data (elastic#63279)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 14, 2020
* 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)
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

Given this a look over too and looks right.
I wish we could add a FT around this, but looking at the implementation it's actually quite hard because you can't have an FT run for over 5 minutes to get to the retry.

We might have to manually create a task who's retryAt is in the past through some hacky means 🤔
I'll give it some thought.

@mikecote mikecote moved this from In Progress to Done (Ordered by most recent) in Make it Action Apr 14, 2020
wayneseymour pushed a commit that referenced this pull request Apr 15, 2020
* Fix task manager query to also return tasks to retry

* Fix failing jest tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Task Manager release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.7.0 v7.8.0 v8.0.0
Projects
No open projects
Make it Action
  
Done (Ordered by most recent)
Development

Successfully merging this pull request may close these issues.

None yet

6 participants