[Response Ops][Task Manager] Adding integration test to ensure no `WorkloadAggregator` errors when there are unrecognized task types. by ymao1 · Pull Request #193479 · elastic/kibana

ymao1 · 2024-09-19T20:05:13Z

Fixes https://github.com/elastic/kibana-team/issues/1036

Summary

Adding integration test as RCA action for incident where unrecognized task types was causing issues generating the workload portion of the task manager health report.

To verify

Add this line to your code to that will throw an error when there are unrecognized task types when generating the health report

--- a/x-pack/plugins/task_manager/server/task_type_dictionary.ts
+++ b/x-pack/plugins/task_manager/server/task_type_dictionary.ts
@@ -128,6 +128,7 @@ export class TaskTypeDictionary {
   }

   public get(type: string): TaskDefinition | undefined {
+    this.ensureHas(type);
     return this.definitions.get(type);
   }

Run the integration test node scripts/jest_integration.js x-pack/plugins/task_manager/server/integration_tests/removed_types.test.ts and see that it fails because a WorkloadAggregator error is logged.

kibanamachine · 2024-09-19T21:31:41Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6983

[❌] x-pack/test/plugin_api_integration/config.ts: 13/100 tests passed.

see run history

kibanamachine · 2024-09-20T14:58:21Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6989

[❌] x-pack/test/plugin_api_integration/config.ts: 12/100 tests passed.

see run history

kibanamachine · 2024-09-20T16:39:00Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6991

[❌] x-pack/test/plugin_api_integration/config.ts: 14/100 tests passed.

see run history

elasticmachine · 2024-09-23T19:07:23Z

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

pmuellr · 2024-09-24T21:07:26Z

Should we run the flaky tests again? Didn't see any running right now - or maybe you were using those for some research?

pmuellr

LGTM, left a note about changing the task claimer :-)

pmuellr · 2024-09-24T22:04:20Z

+    const setupResult = await setupTestServers({
+      xpack: {
+        task_manager: {
+          claim_strategy: `update_by_query`,


hmmm ... should we test both claimers? I think for the test this is ultimately making, it's not needed.

But I was curious about hard-coding the claim_strategy, and I did notice a lot of usage of claim_strategy: 'default' in the code (search on claim_strategy), which is presumably logging an error once, and then returning "the default". :-)

kibana/x-pack/plugins/task_manager/server/task_claimers/index.ts

Lines 52 to 65 in c28af87

export function getTaskClaimer(logger: Logger, strategy: string): TaskClaimerFn {

switch (strategy) {

case CLAIM_STRATEGY_UPDATE_BY_QUERY:

return claimAvailableTasksUpdateByQuery;

case CLAIM_STRATEGY_MGET:

return claimAvailableTasksMget;

}

if (!WarnedOnInvalidClaimer) {

WarnedOnInvalidClaimer = true;

logger.warn(`Unknown task claiming strategy "${strategy}", falling back to update_by_query`);

}

return claimAvailableTasksUpdateByQuery;

}

So, thinking maybe change this to default - if testing with one is fine. That way it'll track the default we use, when we switch the default to mget.

I guess I can remove the hard-coded claimer and it will just use whatever the default is, whenever we switch it, it'll test the default? That makes sense to me.

I can create a followup issue for the default claim strategies. Probably an oversight from when we switched from default to update_by_query

Updated all the claim_strategy: 'default' to update_by_query in this commit: a58040c

Removed the hard-coded claim strategy in the jest integration test in this commit: f463ff7

ymao1 · 2024-09-24T22:57:36Z

Should we run the flaky tests again? Didn't see any running right now - or maybe you were using those for some research?

I originally tried updating a functional test and was running the flaky test runner on that FT but abandoned that approach in favor of a jest integration test.

guskovaue

LGTM!

ymao1 · 2024-09-25T12:32:11Z

@elasticmachine merge upstream

kibana-ci · 2024-09-25T14:18:50Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 573c902

Failed CI Steps

Metrics [docs]

✅ unchanged

History

💔 Build #236941 failed a58040c
💛 Build #236457 was flaky c7cec63
💔 Build #236043 failed ea470d6bb5b0f6fbaae2beccf46e1a0279bd3077
💔 Build #235974 failed 2e7ad40ba9fa116481c645e2a86881c9b52f291e
💛 Build #235765 was flaky 1ff1aa7c41fd7cf72aa6ebe8458938f99fc0631d

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

cc @ymao1

…rkloadAggregator` errors when there are unrecognized task types. (elastic#193479) Fixes elastic/kibana-team#1036 ## Summary Adding integration test as RCA action for incident where unrecognized task types was causing issues generating the workload portion of the task manager health report. ## To verify Add this line to your code to that will throw an error when there are unrecognized task types when generating the health report ``` --- a/x-pack/plugins/task_manager/server/task_type_dictionary.ts +++ b/x-pack/plugins/task_manager/server/task_type_dictionary.ts @@ -128,6 +128,7 @@ export class TaskTypeDictionary { } public get(type: string): TaskDefinition | undefined { + this.ensureHas(type); return this.definitions.get(type); } ``` Run the integration test `node scripts/jest_integration.js x-pack/plugins/task_manager/server/integration_tests/removed_types.test.ts` and see that it fails because a `WorkloadAggregator` error is logged. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> (cherry picked from commit 01eae15)

kibanamachine · 2024-09-25T14:26:55Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…no `WorkloadAggregator` errors when there are unrecognized task types. (#193479) (#194016) # Backport This will backport the following commits from `main` to `8.x`: - [[Response Ops][Task Manager] Adding integration test to ensure no `WorkloadAggregator` errors when there are unrecognized task types. (#193479)](#193479)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Ying Mao <ying.mao@elastic.co>

Adding integration test

c7cec63

ymao1 force-pushed the rca-issue-1036 branch from ea470d6 to c7cec63 Compare September 23, 2024 17:06

ymao1 changed the title ~~Adding check for workload values~~ [Response Ops][Task Manager] Adding integration test to ensure no WorkloadAggregator errors when there are unrecognized task types. Sep 23, 2024

ymao1 self-assigned this Sep 23, 2024

ymao1 added release_note:skip Skip the PR/issue when compiling release notes Feature:Task Manager Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v9.0.0 v8.16.0 labels Sep 23, 2024

ymao1 marked this pull request as ready for review September 23, 2024 19:07

ymao1 requested a review from a team as a code owner September 23, 2024 19:07

ymao1 added the backport:prev-minor label Sep 23, 2024

ymao1 requested review from guskovaue and pmuellr September 23, 2024 19:08

pmuellr approved these changes Sep 24, 2024

View reviewed changes

guskovaue approved these changes Sep 25, 2024

View reviewed changes

ymao1 added 3 commits September 25, 2024 06:45

Merge branch 'main' of github.com:elastic/kibana into rca-issue-1036

a05cebe

PR feedback

f463ff7

PR feedback

a58040c

Merge branch 'main' into rca-issue-1036

573c902

ymao1 merged commit 01eae15 into elastic:main Sep 25, 2024

ymao1 deleted the rca-issue-1036 branch September 25, 2024 14:22

kibanamachine mentioned this pull request Sep 25, 2024

[8.x] [Response Ops][Task Manager] Adding integration test to ensure no `WorkloadAggregator` errors when there are unrecognized task types. (#193479) #194016

Merged

	export function getTaskClaimer(logger: Logger, strategy: string): TaskClaimerFn {
	switch (strategy) {
	case CLAIM_STRATEGY_UPDATE_BY_QUERY:
	return claimAvailableTasksUpdateByQuery;
	case CLAIM_STRATEGY_MGET:
	return claimAvailableTasksMget;
	}

	if (!WarnedOnInvalidClaimer) {
	WarnedOnInvalidClaimer = true;
	logger.warn(`Unknown task claiming strategy "${strategy}", falling back to update_by_query`);
	}
	return claimAvailableTasksUpdateByQuery;
	}

Conversation

ymao1 commented Sep 19, 2024 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

To verify

Uh oh!

kibanamachine commented Sep 19, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6983

Uh oh!

kibanamachine commented Sep 20, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6989

Uh oh!

kibanamachine commented Sep 20, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#6991

Uh oh!

elasticmachine commented Sep 23, 2024

Uh oh!

pmuellr commented Sep 24, 2024

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

ymao1 Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

ymao1 Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

ymao1 Sep 25, 2024

Choose a reason for hiding this comment

Uh oh!

ymao1 commented Sep 24, 2024

Uh oh!

guskovaue left a comment

Choose a reason for hiding this comment

Uh oh!

ymao1 commented Sep 25, 2024

Uh oh!

kibana-ci commented Sep 25, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

Uh oh!

kibanamachine commented Sep 25, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ymao1 commented Sep 19, 2024 •

edited by kibanamachine

Loading