Skip rule execution when it has a newer typeVersion than Kibana instance #172161

ersin-erdal · 2023-11-29T12:49:52Z

This PR adds a new logic to skip task execution by using the modelVersions of saved objects.

schemas.create in modelVersions are used by Kibana to validate the SO that they are attached before saving them.
So it will force developers to bump the versions whenever there is a change in the SO (add, remove etc.)

We have already saved the latestRuleVersion in ruleTypeRegistry with the following PR #171927

And with this PR, we also save the typeVersion in a rule on its create, update and execute actions, so we can pass the rule type version to TaskManager with loadIndirectParams.
Then we can compare the latestRuleVersion with typeVersion in TaskManager and skip the execution if typeVersion of a rule is greater than latestRuleVersion. This means the rule has already been updated by a newer Kibana version.

But before passing typeVersion to TM, we recursively call the isCompatibleWithPreviousVersion method of the rule versions until one of them return false (or up to version 1) then return that version to TM.

isCompatibleWithPreviousVersion takes the rawRule as param, so a developer can decide if the new version is compatible with the previous one on run time.

For instance: The new field muted is added to the rule SO. And a developer wants to if the fetched rule SO is compatible with the previous version, so they can do:

isCompatibleWithPreviousVersion: (rawRule) => rawRule.muted ? true : false,

Another change that is introduced by tis PR:
As we are replacing the indirectParams validation with version check in TM, we decided to validate only the rule.params rather than the whole rawRule. Therefore we return rule.params as indirectParams and ruleType.validate.params as indirectParamsSchema

Note: we do indirectParamsSchema.validate(indirectParams) in TM.

To verify:

Enable skipping feature in config:

xpack:
 task_manager:
   requeue_invalid_tasks:
     enabled: true
     delay: 1000
     max_attempts: 10

Create a rule and check if it has the typeVersion:1 field,

replace typeVersion with typeVersion: 2 (current latest rule type version is 1) in here

TM should be skipping the rule execution and print 2 warnings:
Task (xxx/xxx) has a newer version(2) than expected(1)
Task Manager has skipped executing the Task (xxx/xxx) x times as it has invalid params.

after 10 skips, the should be executed.

typeVersion field should be addedd to the rules from an older Kibana version (after upgrade) on the first execution.

ersin-erdal · 2023-11-29T12:52:38Z

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

@@ -941,7 +943,6 @@ export class TaskRunner<

        return { interval: retryInterval };
      }),
-      monitoring: this.ruleMonitoring.getMonitoring(),


This field is redundant and not used by TaskManager.

ersin-erdal · 2023-11-29T13:04:43Z

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

-      const { data } = await this.task.loadIndirectParams();
-      if (data) {
+      // validate indirect params e.g. connector or rule.params
+      if (indirectParamsSchema && indirectParams) {


Patrick left a note about this check in this PR's POC.
When there is a version mismatch the task is skipped 100 times (default number) then doesn't return a skipError in order to let the rule run.
So it comes to this line and does indirectParamsSchema validation as well. As it has already reached the maxAttempts just logs a warning and doesn't return any skipError either.

We may skip this validation when there is already an error (hasValidationError). WDYT?

elasticmachine · 2023-11-29T17:33:22Z

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

ersin-erdal · 2023-11-29T23:48:49Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group2/execution_status.ts

@@ -212,7 +212,7 @@ export default function executionStatusAlertTests({ getService }: FtrProviderCon
        })
        .expect(200);

-      executionStatus = await waitForStatus(alertId, new Set(['error']));
+      executionStatus = await waitForStatus(alertId, new Set(['error']), 50000);


As this waits for the task execution to be skipped 2 times and then run the rule, increased the delay to make it long enough for 2 skips.

ymao1 · 2023-11-30T15:54:14Z

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

@@ -315,7 +314,7 @@ export class RuleTypeRegistry {
          spaceId: schema.string(),
          consumer: schema.maybe(schema.string()),
        }),
-        indirectParamsSchema: rawRuleSchemaV1,
+        indirectParamsSchema: ruleType.validate.params,


Why are we no longer validating the rule schema and just the rule parameters? Seems like changes to the rule schema could also potentially be backwards incompatible

Actually version check replaces the schema check, but i wanted to use it for params validation rather than removing the whole feature. Maybe removing is just better as we already have another issue for params, config and secrets validation.

And the rule schema is just used for modelVersions which validates the rule just before creating the SO in ES.
This is to force developers to update the schema and bump the version.

ymao1 · 2023-11-30T16:16:21Z

Can you provide instructions for verifying this PR? Under what conditions should it succeed? Under what conditions should it fail? I am familiar at a high level but it would be helpful to document these things as well.

ersin-erdal · 2023-12-01T11:55:20Z

Can you provide instructions for verifying this PR? Under what conditions should it succeed? Under what conditions should it fail? I am familiar at a high level but it would be helpful to document these things as well.

Added how-to-verify section to the description

pmuellr · 2023-12-04T16:27:13Z

I'm not seeing the expected results from To verify: - the rule seems. to continue running without extra logging.

Perhaps because I had a 1s interval? Will try again with a 1m. Also I figured I'd disable the rule before making the code change, make the code change, let Kibana restart and spam the console, wait for it to finish, then re-enable the rule in hopes of more clearly seeing the messages. Should work, right?

ersin-erdal · 2023-12-04T17:35:14Z

I'm not seeing the expected results from To verify: - the rule seems. to continue running without extra logging.

Perhaps because I had a 1s interval? Will try again with a 1m. Also I figured I'd disable the rule before making the code change, make the code change, let Kibana restart and spam the console, wait for it to finish, then re-enable the rule in hopes of more clearly seeing the messages. Should work, right?

Opss, I was confused...
You need to replace typeVersion not rule.version, fixed the verification steps as well. Sorry :)

edit: There was one more missing thing, you need to pull the PR again.

…sion' into 166967-skip-tasks-with-wrong-version

# Conflicts: # x-pack/plugins/task_manager/server/task_running/task_runner.ts

kibana-ci · 2023-12-26T17:22:37Z

💚 Build Succeeded

Buildkite Build
Commit: 7edebad

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`alerting`	794	795	+1
`taskManager`	62	63	+1
total			+2

Unknown metric groups

API count

id	before	after	diff
`alerting`	825	826	+1
`taskManager`	105	106	+1
total			+2

History

💛 Build #184123 was flaky e0dc0c1
💛 Build #183928 was flaky 7b26948
💔 Build #183891 failed 39c0bc9
💚 Build #181542 succeeded 5893b6d
💛 Build #180775 was flaky 1e1bcea
💔 Build #180754 failed 08777fd

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

cc @ersin-erdal

ersin-erdal · 2024-03-25T13:01:14Z

The feature has been reverted.

Skip rule execution when it has a newer typeVersion than Kibana instance

34fc4f9

ersin-erdal added 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.12.0 labels Nov 29, 2023

ersin-erdal self-assigned this Nov 29, 2023

ersin-erdal commented Nov 29, 2023

View reviewed changes

ersin-erdal added 2 commits November 29, 2023 14:15

fix type check

69b279c

fix indirect params schema

da79ebe

ersin-erdal marked this pull request as ready for review November 29, 2023 17:33

ersin-erdal requested a review from a team as a code owner November 29, 2023 17:33

add the removed test back

c1748c2

ersin-erdal commented Nov 29, 2023

View reviewed changes

add skipping task functional test

ae89376

ymao1 reviewed Nov 30, 2023

View reviewed changes

ersin-erdal added 2 commits November 30, 2023 23:34

add sampleTaskWithInvalidVersion to registered tsk types

c4077df

remove params

08777fd

ersin-erdal force-pushed the 166967-skip-tasks-with-wrong-version branch from e441fb8 to 08777fd Compare December 1, 2023 01:18

Merge branch 'main' into 166967-skip-tasks-with-wrong-version

1e1bcea

ersin-erdal mentioned this pull request Dec 1, 2023

Alerting rules are not skipped if the params aren't compatible #172351

Closed

ersin-erdal and others added 5 commits December 4, 2023 20:56

Merge branch 'main' into 166967-skip-tasks-with-wrong-version

ee4419e

add missing latestTypeVersion to registerTaskDefinitions

966a841

Merge remote-tracking branch 'origin/166967-skip-tasks-with-wrong-ver…

5893b6d

…sion' into 166967-skip-tasks-with-wrong-version

Merge branch 'main' into 166967-skip-tasks-with-wrong-version

0b7c14d

# Conflicts: # x-pack/plugins/task_manager/server/task_running/task_runner.ts

fix validation in task runner

39c0bc9

fix validation in task runner

7b26948

ersin-erdal added v8.13.0 and removed v8.12.0 labels Dec 18, 2023

ersin-erdal and others added 2 commits December 18, 2023 23:45

merge main

e0dc0c1

Merge branch 'main' into 166967-skip-tasks-with-wrong-version

7edebad

ersin-erdal closed this Mar 25, 2024

ersin-erdal deleted the 166967-skip-tasks-with-wrong-version branch March 25, 2024 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip rule execution when it has a newer typeVersion than Kibana instance #172161

Skip rule execution when it has a newer typeVersion than Kibana instance #172161

ersin-erdal commented Nov 29, 2023 •

edited

Loading

ersin-erdal Nov 29, 2023

ersin-erdal Nov 29, 2023

elasticmachine commented Nov 29, 2023

ersin-erdal Nov 29, 2023

ymao1 Nov 30, 2023

ersin-erdal Nov 30, 2023

ersin-erdal Nov 30, 2023

ymao1 commented Nov 30, 2023

ersin-erdal commented Dec 1, 2023

pmuellr commented Dec 4, 2023

ersin-erdal commented Dec 4, 2023 •

edited

Loading

kibana-ci commented Dec 26, 2023

API count

ersin-erdal commented Mar 25, 2024

Skip rule execution when it has a newer typeVersion than Kibana instance #172161

Skip rule execution when it has a newer typeVersion than Kibana instance #172161

Conversation

ersin-erdal commented Nov 29, 2023 • edited Loading

To verify:

ersin-erdal Nov 29, 2023

Choose a reason for hiding this comment

ersin-erdal Nov 29, 2023

Choose a reason for hiding this comment

elasticmachine commented Nov 29, 2023

ersin-erdal Nov 29, 2023

Choose a reason for hiding this comment

ymao1 Nov 30, 2023

Choose a reason for hiding this comment

ersin-erdal Nov 30, 2023

Choose a reason for hiding this comment

ersin-erdal Nov 30, 2023

Choose a reason for hiding this comment

ymao1 commented Nov 30, 2023

ersin-erdal commented Dec 1, 2023

pmuellr commented Dec 4, 2023

ersin-erdal commented Dec 4, 2023 • edited Loading

kibana-ci commented Dec 26, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

ersin-erdal commented Mar 25, 2024

ersin-erdal commented Nov 29, 2023 •

edited

Loading

ersin-erdal commented Dec 4, 2023 •

edited

Loading