[Response Ops] Keep task document when enabling/disabling rules #139826

ymao1 · 2022-08-31T13:09:54Z

🚨🚨🚨 A migration has been added to add enabled: true to existing task manager documents with status: 'claiming' | 'idle' | 'running'. It sets enabled: false to tasks with status: 'failed' | 'unrecognized' This might warrant extra review :) 🚨🚨🚨

Summary

This PR adds an enabled field to all task documents. This is a boolean indicating whether the task is currently enabled or disabled. The task claim query has been updated to only claim enabled tasks.

Task manager changes:

taskSchedule.schedule and taskSchedule.bulkSchedule have been updated to set enabled: true for newly scheduled tasks (unless enabled: false is explicitly set when scheduling.
new function taskSchedule.bulkEnableDisable to update this flag for specific task ids

Rules client changes:

updated disable function to stop deleting the underlying task manager document when a rule is disabled (with one exception, left comment on the PR). Instead, it updates the enabled flag on the task document to be false.
updated enable function to stop scheduling a new task when a rule is enabled (except when no task exists). Instead, it updates the enabled flag on the task document to be true.

To Verify

Run an older version of Kibana and create some rules with actions. Force a failure in an action in order to get a task doc with status failed. Create some rules and disable at least one of them.
Run this PR with the same data as step1. Verify that all task docs have a new enabled field. Verify that failed tasks have enabled: false but other tasks have enabled: true. Verify that the rules that were enabled before are still running.
Enable the disabled rules from before. Verify that a task doc gets created for these.
Create some new rules. Verify that enabling and disabling both old and new rules does not delete the task doc, it just flips the enabled flag on the task.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…nager/enabled

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

…rule id

…nager/enabled

ymao1 · 2022-09-01T17:46:09Z

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

          updatedBy: await this.getUserName(),
          updatedAt: new Date().toISOString(),
        }),
        { version }
      );
+


This update to the disable logic takes into account that there may be pre 8.0 rules running where the scheduled task ID does not equal the rule ID. In these cases, we still want to remove the task document so that a new one matching the rule id can be created on enable.

If the scheduledTaskId already matches the rule ID, this will set the task to disabled.

ymao1 · 2022-09-01T17:46:30Z

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

@@ -2035,6 +2035,23 @@ export class RulesClient {
      } catch (e) {
        throw e;
      }
+    }
+


This update to the enable logic takes into account the fact that there may be already disabled rules with no corresponding task.

If the rule is disabled with no corresponding task, it's scheduledTaskId will be undefined. In this case, we want to schedule a task on enable.

If a rule somehow has a scheduledTaskId defined but it doesn't actually exist, we want to schedule a task on enable.

Finally, if a rule has a scheduledTaskId defined and the task exists, we enable the task.

…nager/enabled

…tasks

ymao1 · 2022-09-01T20:54:50Z

@elasticmachine merge upstream

…into task-manager/enabled

elasticmachine · 2022-09-02T13:32:38Z

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

ymao1 · 2022-09-06T11:27:39Z

@elasticmachine merge upstream

ymao1 · 2022-09-06T12:36:59Z

@elasticmachine merge upstream

ymao1 · 2022-09-07T11:55:51Z

@elasticmachine merge upstream

ymao1 · 2022-09-08T11:47:54Z

@elasticmachine merge upstream

pmuellr · 2022-09-08T14:59:10Z

I'm not sure of the pressing need for this any more, but we have been using "disable then re-enable" as a way of "resetting" a rule's task state. Feels like we may still need this, probably just for problematic cases. Should we open a follow-on PR, or do we even need this, or are there alternatives?

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

ersin-erdal

LGTM. Tested locally, works as expected.

ymao1 · 2022-09-08T15:18:42Z

I'm not sure of the pressing need for this any more, but we have been using "disable then re-enable" as a way of "resetting" a rule's task state. Feels like we may still need this, probably just for problematic cases. Should we open a follow-on PR, or do we even need this, or are there alternatives?

@pmuellr We discussed this in the issue: #110096 (comment) and it seems like the consensus is that losing task state on disable is considered a bug that we're fixing. How often are we suggesting disable/reenable to clear task state?

ymao1 · 2022-09-08T18:14:16Z

I'm not sure of the pressing need for this any more, but we have been using "disable then re-enable" as a way of "resetting" a rule's task state. Feels like we may still need this, probably just for problematic cases. Should we open a follow-on PR, or do we even need this, or are there alternatives?

@pmuellr Re-read your question and I think you're asking if we should open a followup issue for being able to reset task state, which I definitely can :)

pmuellr · 2022-09-08T19:46:50Z

Re-read your question and I think you're asking if we should open a followup issue for being able to reset task state, which I definitely can :)

Yes, basically what I'm asking :-)

I know the typical use case WE use it for is probably just basic sync issues between the rule and task. I'm not sure folks have really used it JUST to reset the task state - for example, if there's some bad rule-specific state, or to "reset all the alerts" or something. I don't think we've really seen a need for that. Which is all a new "reset task" API would do, I would think.

And wasn't sure if there was already some other way of accomplishing that anyway.

Sounds like it's something we should consider though, so opening an issue to track seems appropriate.

ymao1 · 2022-09-09T13:38:52Z

@pmuellr issue created: #140402

ymao1 · 2022-09-09T13:39:01Z

@elasticmachine merge upstream

pmuellr

LGTM; left some questions ...

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

x-pack/plugins/task_manager/server/saved_objects/migrations.ts

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

x-pack/plugins/task_manager/server/task_running/task_runner.test.ts

x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts

x-pack/plugins/alerting/server/rules_client/tests/disable.test.ts

x-pack/plugins/alerting/server/rules_client_conflict_retries.test.ts

…nager/enabled

kibana-ci · 2022-09-12T13:30:38Z

💚 Build Succeeded

Buildkite Build
Commit: 3bd7a3a

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`taskManager`	82	83	+1

History

💚 Build #70842 succeeded bb04cff
💚 Build #70391 succeeded c9390eb
💛 Build #70039 was flaky 1123e3a
💛 Build #69665 was flaky 397495e
💔 Build #69635 failed 869be57
💚 Build #69251 succeeded 8dc3c62

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

cc @ymao1

ymao1 added 9 commits August 30, 2022 12:56

wip

ff61e14

Merge branch 'main' of https://github.com/elastic/kibana into task-ma…

1a56865

…nager/enabled

wip

b9c2d86

Fixing types and adding unit tests for task manager disable

531ceb6

Updating to enable/disable. Update rules client to use new fns

bdff214

Merge branch 'main' of https://github.com/elastic/kibana into task-ma…

feff3da

…nager/enabled

Updating unit tests. Fixing enable to still schedule task if necessary

69397ff

Adding functional test for task manager migration

f239fe9

Fixing query. Updating functional tests

2fa3a44

ymao1 commented Sep 1, 2022

View reviewed changes

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

ymao1 added 2 commits September 1, 2022 13:43

Setting scheduledTaskId to null on disable only if it does not match …

638852e

…rule id

Merge branch 'main' of https://github.com/elastic/kibana into task-ma…

0869306

…nager/enabled

ymao1 commented Sep 1, 2022

View reviewed changes

Updating README

46bfc04

ymao1 changed the title ~~Task manager/enabled~~ [Response Ops] Keep task document when enabling/disabling rules Sep 1, 2022

ymao1 self-assigned this Sep 1, 2022

ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Feature:Task Manager Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.5.0 labels Sep 1, 2022

ymao1 added 4 commits September 1, 2022 15:58

Fixing tests

0833943

Merge branch 'main' of https://github.com/elastic/kibana into task-ma…

fa4a907

…nager/enabled

Task manager runner doesn't overwrite enabled on update

a2db534

Updating migration to set enabled: false for failed and unrecognized …

4ee56b7

…tasks

kibanamachine and others added 3 commits September 1, 2022 16:54

Merge branch 'main' into task-manager/enabled

482d58d

Fixing tests

4dd9ade

Merge branch 'task-manager/enabled' of https://github.com/ymao1/kibana …

95b9e8c

…into task-manager/enabled

Merge branch 'main' into task-manager/enabled

869be57

Merge branch 'main' into task-manager/enabled

397495e

Merge branch 'main' into task-manager/enabled

1123e3a

Merge branch 'main' into task-manager/enabled

c9390eb

ersin-erdal reviewed Sep 8, 2022

View reviewed changes

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

ersin-erdal approved these changes Sep 8, 2022

View reviewed changes

ymao1 mentioned this pull request Sep 9, 2022

[Response Ops][Alerting] Do we need a way to clear task state? #140402

Open

Merge branch 'main' into task-manager/enabled

bb04cff

pmuellr approved these changes Sep 9, 2022

View reviewed changes

ymao1 added 2 commits September 12, 2022 06:57

Merge branch 'main' of https://github.com/elastic/kibana into task-ma…

2e25e39

…nager/enabled

PR feedback

3bd7a3a

This was referenced Sep 12, 2022

[Response Ops] [Alerting] Error when disabling rule will throw 500 #140499

Open

[Response Ops][Task Manager] Add lightweight function to check if task doc exists #140505

Open

ymao1 merged commit 0cf0e3d into elastic:main Sep 12, 2022

ymao1 deleted the task-manager/enabled branch September 12, 2022 13:35

kibanamachine added the backport:skip This commit does not require backporting label Sep 12, 2022

ymao1 mentioned this pull request Sep 13, 2022

[Response Ops] [Alerting] Disabling/enabling a rule no longer runs the rule #140632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Response Ops] Keep task document when enabling/disabling rules #139826

[Response Ops] Keep task document when enabling/disabling rules #139826

ymao1 commented Aug 31, 2022 •

edited

Loading

ymao1 Sep 1, 2022

ymao1 Sep 1, 2022

ymao1 commented Sep 1, 2022

elasticmachine commented Sep 2, 2022

ymao1 commented Sep 6, 2022

ymao1 commented Sep 6, 2022

ymao1 commented Sep 7, 2022

ymao1 commented Sep 8, 2022

pmuellr commented Sep 8, 2022

ersin-erdal left a comment

ymao1 commented Sep 8, 2022

ymao1 commented Sep 8, 2022

pmuellr commented Sep 8, 2022

ymao1 commented Sep 9, 2022

ymao1 commented Sep 9, 2022

pmuellr left a comment

kibana-ci commented Sep 12, 2022

API count

[Response Ops] Keep task document when enabling/disabling rules #139826

[Response Ops] Keep task document when enabling/disabling rules #139826

Conversation

ymao1 commented Aug 31, 2022 • edited Loading

Summary

To Verify

Checklist

ymao1 Sep 1, 2022

Choose a reason for hiding this comment

ymao1 Sep 1, 2022

Choose a reason for hiding this comment

ymao1 commented Sep 1, 2022

elasticmachine commented Sep 2, 2022

ymao1 commented Sep 6, 2022

ymao1 commented Sep 6, 2022

ymao1 commented Sep 7, 2022

ymao1 commented Sep 8, 2022

pmuellr commented Sep 8, 2022

ersin-erdal left a comment

Choose a reason for hiding this comment

ymao1 commented Sep 8, 2022

ymao1 commented Sep 8, 2022

pmuellr commented Sep 8, 2022

ymao1 commented Sep 9, 2022

ymao1 commented Sep 9, 2022

pmuellr left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 12, 2022

💚 Build Succeeded

Metrics [docs]

API count

History

ymao1 commented Aug 31, 2022 •

edited

Loading