[Task Manager] Adding list of explicitly de-registered task types #123963

ymao1 · 2022-01-27T18:32:05Z

Summary

Adds a list of explicitly de-registered task types and updates claim script to only mark a task as unrecognized if the task type exists in that list.

In order to explicitly stop registering a task, plugin authors will need to remove the task registration code as well as add the task type to the REMOVED_TYPES list in x-pack/plugins/task_manager/server/task_type_dictionary.ts

If the task is not registered but not added to REMOVED_TYPES, it will not be marked as unrecognized. It will just not be claimed.
If a task is added to REMOVED_TYPES but still registered, an error will be thrown. These scenarios should be caught in development.

Todo

Add old siem rule type to list.

To Verify

Create a rule that runs frequently and make sure it runs (I used the People in Space rule). Query for the rule in the task manager index and see that status is either idle or running.
Update the code so that this rule type is no longer registered with alerting

--- a/x-pack/examples/alerting_example/server/plugin.ts
+++ b/x-pack/examples/alerting_example/server/plugin.ts
@@ -28,7 +28,7 @@ export interface AlertingExampleDeps {
 export class AlertingExamplePlugin implements Plugin<void, void, AlertingExampleDeps> {
   public setup(core: CoreSetup, { alerting, features }: AlertingExampleDeps) {
     alerting.registerType(alwaysFiringAlert);
-    alerting.registerType(peopleInSpaceAlert);
+    // alerting.registerType(peopleInSpaceAlert);

Verify that the rule no longer runs and the status stays idle
Revert the update made in step 2. Verify that the rule starts running again.
Update the code to add this rule type to the list of removed types

--- a/x-pack/plugins/task_manager/server/task_type_dictionary.ts
+++ b/x-pack/plugins/task_manager/server/task_type_dictionary.ts
@@ -14,6 +14,7 @@ import { Logger } from '../../../../src/core/server';
 export const REMOVED_TYPES: string[] = [
   // for testing
   'sampleTaskRemovedType',
+  'alerting:example.people-in-space',
 ];

Verify that the rule status is updated to unrecognized.

Checklist

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

…unrecognized

…nager/unregistered-task-types

ymao1 · 2022-02-02T23:25:34Z

@elasticmachine merge upstream

ymao1 · 2022-02-03T14:52:09Z

@elasticmachine merge upstream

elasticmachine · 2022-02-03T17:06:53Z

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

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

gmmorris · 2022-02-09T12:02:33Z

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

+ */
+export const REMOVED_TYPES: string[] = [
+  // for testing
+  'sampleTaskRemovedType',


Once this PR is merged will we not actually have any type in here?
It raises the question of whether we need the unusedTypes feature at all, no? 🤔

pmuellr · 2022-02-09T17:26:54Z

This reminds me of an issue I forgot to open up, when looking to disable apm telemetry. I believe when I looked into this, if apm was configured to disable telemetry, the existing telemetry task would eventually get marked as unrecognized, with no way to reverse that without explicitly deleting the task doc from the TM index. I think if we merge something like this, the existing code would then work fine.

Start here, where the apm plugin sets up telemetry stuff, including registering the telemetry task, in the plugin setup(), by calling createApmTelemetry() - but that call is made conditionally:

kibana/x-pack/plugins/apm/server/plugin.ts

Lines 67 to 94 in 8e8c4b8

    
           public setup( 
        
             core: CoreSetup<APMPluginStartDependencies>, 
        
             plugins: APMPluginSetupDependencies 
        
           ) { 
        
             this.logger = this.initContext.logger.get(); 
        
             const config$ = this.initContext.config.create<APMConfig>(); 
        
             core.savedObjects.registerType(apmIndices); 
        
             core.savedObjects.registerType(apmTelemetry); 
        
             core.savedObjects.registerType(apmServerSettings); 
        
             const currentConfig = this.initContext.config.get<APMConfig>(); 
        
             this.currentConfig = currentConfig; 
        
             if ( 
        
               plugins.taskManager && 
        
               plugins.usageCollection && 
        
               currentConfig.telemetryCollectionEnabled 
        
             ) { 
        
               createApmTelemetry({ 
        
                 core, 
        
                 config$, 
        
                 usageCollector: plugins.usageCollection, 
        
                 taskManager: plugins.taskManager, 
        
                 logger: this.logger, 
        
                 kibanaVersion: this.initContext.env.packageInfo.version, 
        
               }); 
        
             }

The actual task registration is in that createApmTelemetry() function:

kibana/x-pack/plugins/apm/server/lib/apm_telemetry/index.ts

Lines 37 to 64 in 8e8c4b8

    
           export async function createApmTelemetry({ 
        
             core, 
        
             config$, 
        
             usageCollector, 
        
             taskManager, 
        
             logger, 
        
             kibanaVersion, 
        
           }: { 
        
             core: CoreSetup; 
        
             config$: Observable<APMConfig>; 
        
             usageCollector: UsageCollectionSetup; 
        
             taskManager: TaskManagerSetupContract; 
        
             logger: Logger; 
        
             kibanaVersion: string; 
        
           }) { 
        
             taskManager.registerTaskDefinitions({ 
        
               [APM_TELEMETRY_TASK_NAME]: { 
        
                 title: 'Collect APM usage', 
        
                 createTaskRunner: () => { 
        
                   return { 
        
                     run: async () => { 
        
                       await collectAndStore(); 
        
                     }, 
        
                     cancel: async () => {}, 
        
                   }; 
        
                 }, 
        
               }, 
        
             });

It wouldn't surprise me to find other task registrations that are conditional like this. Again, I >think< with the change in this PR, such tasks would be ignored, until Kibana was later restarted and when down a conditional path to register the task again. And wouldn't end up in the unrecognized task black hole. Am I thinking about that right?

….com/ymao1/kibana into task-manager/unregistered-task-types

ymao1 · 2022-02-10T13:22:26Z

It wouldn't surprise me to find other task registrations that are conditional like this. Again, I >think< with the change in this PR, such tasks would be ignored, until Kibana was later restarted and when down a conditional path to register the task again. And wouldn't end up in the unrecognized task black hole. Am I thinking about that right?

@pmuellr Yes, that's correct. With the change in this PR, those tasks would not get marked as unrecognized unless they are specifically added to the UNUSED_TYPES list. Instead task manager would just ignore them so they stay idle until a Kibana that does register the task type tries to claim them.

ersin-erdal · 2022-02-10T18:00:02Z

x-pack/plugins/task_manager/server/queries/mark_available_tasks_as_claimed.test.ts

@@ -126,7 +127,7 @@ if (doc['task.runAt'].size()!=0) {
      ctx._source.task.status = "claiming"; ${Object.keys(fieldUpdates)
        .map((field) => `ctx._source.task.${field}=params.fieldUpdates.${field};`)
        .join(' ')}
-    } else if (!params.skippedTaskTypes.contains(ctx._source.task.taskType)) {


Don't we need to test the skipped tasks?

Previously we maintained a list of claimableTaskTypes and skippedTaskTypes. These are all the types that this task manager knows about so if a task is not claimable or skipped, we mark is as unrecognized. Now we are just using claimableTaskTypes to claim and unusedTaskTypes to determine what to mark as unrecognized. Skipped task types will go into the else bucket and get a no-op.

mikecote

Changes LGTM, tested locally the following scenarios:

Alerting rule with a task in "unrecognized" status returning to idle after upgrade
Alerting rule with a task definition missing did not make the task have an unrecognized status
Alerting rule with a task definition missing and part of REMOVED_TYPES caused the task status to become unrecognized

👍

ymao1 · 2022-02-14T13:06:31Z

@elasticmachine merge upstream

ersin-erdal · 2022-02-15T13:09:12Z

x-pack/plugins/task_manager/server/queries/mark_available_tasks_as_claimed.test.ts

-        updateFieldsAndMarkAsFailed(fieldUpdates, claimTasksById, ['foo', 'bar'], [], {
-          foo: 5,
-          bar: 2,
+        updateFieldsAndMarkAsFailed({


Clean Code, group the arguments, love it :)

ersin-erdal

LGTM!

ymao1 · 2022-02-15T13:24:46Z

@elasticmachine merge upstream

ymao1 · 2022-02-15T14:00:38Z

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

+  'sampleTaskRemovedType',
+
+  // deprecated in https://github.com/elastic/kibana/pull/121442
+  'alerting:siem.signals',


@madirey @marshallmain

Tagging you on this PR for awareness. We adding an explicit registry of removed task types that will be marked as unrecognized. Adding alerting:siem.signals due to this PR that removed that rule type. There should be no change to the outcome of that PR (meaning old siem.signals rule types will still be marked as unrecognized)

kibana-ci · 2022-02-15T15:25:55Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #23863 succeeded 09d30cd
💚 Build #22979 succeeded bda2fdd
💚 Build #21622 succeeded 0c40a92
💔 Build #21487 failed ed31d92
💔 Build #20759 failed 821bc7d
💚 Build #20717 succeeded 1a0453b

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

cc @ymao1

tsullivan · 2022-02-17T22:18:49Z

I just found out about this PR by investigating #120995 and finding I could no longer reproduce the Reporting issue. Thank you @ymao1!

ymao1 added 8 commits January 27, 2022 13:31

Adding REMOVED_TYPES to task manager and only marking those types as …

1ef085d

…unrecognized

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

cf0726d

…nager/unregistered-task-types

Adding unit tests

7dbccbc

Fixing functional test

976a14f

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

1a0453b

…nager/unregistered-task-types

Throwing error when registering a removed task type

7a5cf7b

Adding migration

e126374

Adding functional tests

821bc7d

Merge branch 'main' into task-manager/unregistered-task-types

ed31d92

Merge branch 'main' into task-manager/unregistered-task-types

0c40a92

ymao1 changed the title ~~Adding REMOVED_TYPES to task manager and only marking those types as …~~ [Task Manager] Adding list of explicitly de-registered task types Feb 3, 2022

ymao1 self-assigned this Feb 3, 2022

ymao1 added backport:skip This commit does not require backporting 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) v8.2.0 labels Feb 3, 2022

ymao1 marked this pull request as ready for review February 3, 2022 17:06

ymao1 requested a review from a team as a code owner February 3, 2022 17:06

ymao1 commented Feb 4, 2022

View reviewed changes

x-pack/plugins/task_manager/server/saved_objects/migrations.ts Show resolved Hide resolved

gmmorris reviewed Feb 9, 2022

View reviewed changes

ymao1 added 3 commits February 9, 2022 16:27

Merging in main

249a912

Merge branch 'task-manager/unregistered-task-types' of https://github…

770cb4a

….com/ymao1/kibana into task-manager/unregistered-task-types

Cleanup

bda2fdd

ymao1 requested review from mikecote and pmuellr February 10, 2022 13:19

ymao1 requested a review from ersin-erdal February 10, 2022 13:19

ersin-erdal reviewed Feb 10, 2022

View reviewed changes

mikecote approved these changes Feb 11, 2022

View reviewed changes

Merge branch 'main' into task-manager/unregistered-task-types

09d30cd

ymao1 requested a review from ersin-erdal February 14, 2022 13:06

ersin-erdal reviewed Feb 15, 2022

View reviewed changes

ersin-erdal approved these changes Feb 15, 2022

View reviewed changes

kibanamachine and others added 2 commits February 15, 2022 08:24

Merge branch 'main' into task-manager/unregistered-task-types

2fa9e16

Adding disabled siem signals rule type

2dee057

ymao1 commented Feb 15, 2022

View reviewed changes

ymao1 merged commit c2a0103 into elastic:main Feb 15, 2022

tsullivan mentioned this pull request Feb 17, 2022

[Reporting] Problems with non-uniform enablement #120995

Closed

mikecote mentioned this pull request Mar 29, 2023

Remove task to cleanup action_task_params of failed tasks #151873

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Task Manager] Adding list of explicitly de-registered task types #123963

[Task Manager] Adding list of explicitly de-registered task types #123963

ymao1 commented Jan 27, 2022 •

edited

Loading

ymao1 commented Feb 2, 2022

ymao1 commented Feb 3, 2022

elasticmachine commented Feb 3, 2022

gmmorris Feb 9, 2022

pmuellr commented Feb 9, 2022

ymao1 commented Feb 10, 2022

ersin-erdal Feb 10, 2022

ymao1 Feb 10, 2022

mikecote left a comment

ymao1 commented Feb 14, 2022

ersin-erdal Feb 15, 2022

ersin-erdal left a comment

ymao1 commented Feb 15, 2022

ymao1 Feb 15, 2022

kibana-ci commented Feb 15, 2022

tsullivan commented Feb 17, 2022

[Task Manager] Adding list of explicitly de-registered task types #123963

[Task Manager] Adding list of explicitly de-registered task types #123963

Conversation

ymao1 commented Jan 27, 2022 • edited Loading

Summary

Todo

To Verify

Checklist

ymao1 commented Feb 2, 2022

ymao1 commented Feb 3, 2022

elasticmachine commented Feb 3, 2022

gmmorris Feb 9, 2022

Choose a reason for hiding this comment

pmuellr commented Feb 9, 2022

ymao1 commented Feb 10, 2022

ersin-erdal Feb 10, 2022

Choose a reason for hiding this comment

ymao1 Feb 10, 2022

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

ymao1 commented Feb 14, 2022

ersin-erdal Feb 15, 2022

Choose a reason for hiding this comment

ersin-erdal left a comment

Choose a reason for hiding this comment

ymao1 commented Feb 15, 2022

ymao1 Feb 15, 2022

Choose a reason for hiding this comment

kibana-ci commented Feb 15, 2022

💚 Build Succeeded

Metrics [docs]

History

tsullivan commented Feb 17, 2022

ymao1 commented Jan 27, 2022 •

edited

Loading