[Response Ops] Remove ephemeral tasks from action and alerting plugins #197421

JiaweiWu · 2024-10-23T12:16:41Z

Summary

Removes all reference to ephemeral tasks in the alerting and actions plugin. As well as unit and E2E tests while maintaining backwards compatibility for xpack.alerting.maxEphemeralActionsPerAlert flag.

Deprecates the following configuration settings:

xpack.alerting.maxEphemeralActionsPerAlert

No action is required on the user's end as this deprecation is made so if the above settings are defined, kibana will simply do nothing. However their settings will no longer have any effect as ephemeral tasks are now removed.

Checklist

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

elasticmachine · 2024-10-23T12:16:44Z

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

github-actions · 2024-10-23T12:17:00Z

A documentation preview will be available soon.

🔨 Buildkite builds
📚 HTML diff
📙 Preview page

Request a new doc build by commenting

Rebuild this PR: run docs-build
Rebuild this PR and all Elastic docs: run docs-build rebuild

_{run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.}

_{If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.}

ymao1 · 2024-10-24T19:45:00Z

x-pack/plugins/triggers_actions_ui/public/application/components/health_check.test.tsx

@@ -127,84 +127,6 @@ describe('health check', () => {
    );
  });

-  test('renders warning if encryption key is ephemeral', async () => {


i think we probably want to keep this test? i don't think it's related to ephemeral tasks

mikecote · 2024-10-25T13:50:42Z

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

+      cacheInterval: schema.number({ defaultValue: DEFAULT_CACHE_INTERVAL_MS }),
+    }),
+  },
+  { unknowns: 'allow' }


nit: Preventing unknowns adds the benefit of users knowing when they have typos and such. Perhaps if we change maxEphemeralActionsPerAlert to a schema.maybe(schema.number()), we won't see it anywhere else (it'll be empty by default now)

JiaweiWu · 2024-11-13T07:30:27Z

Im wondering if we can just remove the cleanup function here: https://github.com/JiaweiWu/kibana/blob/main/x-pack/plugins/actions/server/lib/task_runner_factory.ts#L212

Since the isPersistedActionTask seems to check if the task is an ephemeral task (which is now removed).

…-fix'

mikecote · 2024-11-13T19:24:14Z

Im wondering if we can just remove the cleanup function here: https://github.com/JiaweiWu/kibana/blob/main/x-pack/plugins/actions/server/lib/task_runner_factory.ts#L212

Since the isPersistedActionTask seems to check if the task is an ephemeral task (which is now removed).

I believe so, ephemeral tasks are not persisted so once users upgrade to the version containing this change, the task runners will only run normal tasks.

JiaweiWu · 2024-11-18T18:59:24Z

Also, let me know if we want to keep these deprecation messages: https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/index.ts#L87

What's a good way to test that the ephemeral tasks have been removed without affect other code paths? (Aside from existing unit/E2E tests passing)

…-fix'

mikecote · 2024-11-19T11:56:56Z

Also, let me know if we want to keep these deprecation messages: https://github.com/elastic/kibana/blob/main/x-pack/plugins/alerting/server/index.ts#L87

Given we're switching to becoming a no-op, perhaps we can change the message. Something like The setting "xpack.alerting.maxEphemeralActionsPerAlert" is deprecated and currently ignored by the system. Please remove this setting.

What's a good way to test that the ephemeral tasks have been removed without affect other code paths? (Aside from existing unit/E2E tests passing)

Hmm, I wonder if turning on all the settings that we have as no-op and creating some rules that fire actions. If we see the actions still working, that would be one way to manually verify the changes are working. Thoughts?

mikecote

Changes LGTM, I would also update the deprecation message as we discussed in #197421 (comment).

mikecote · 2024-11-21T18:43:07Z

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

    const bulkScheduleRequest: EnqueueExecutionOptions[] = [];

    for (const result of allActionsToScheduleResult) {
-      await this.runActionAsEphemeralOrAddToBulkScheduleRequest({
-        enqueueOptions: result.actionToEnqueue,
-        bulkScheduleRequest,
-      });
+      bulkScheduleRequest.push(result.actionToEnqueue);


nit:

We can remove bulkScheduleRequest and use allActionsToScheduleResult variable directly now.

ymao1 · 2024-11-25T19:10:44Z

docs/settings/alert-action-settings.asciidoc

-deprecated:[8.8.0]
-Sets the number of actions that will run ephemerally. To use this, enable
-ephemeral tasks in task manager first with
-<<task-manager-settings,`xpack.task_manager.ephemeral_tasks.enabled`>>


We'll probably need a cloud PR to remove them from the allow-list: https://github.com/elastic/cloud/pull/84878

hmmm ... not sure. I know the allow-list must be used when creating / updating the Kibana yml in the clould admin panels, but when else would it be used? If it gets used at some other time (cluster restart), then potentially it could prevent the restart because of invalid config?

It's fine if it's only used while creating / updating the Kibana yml in the cloud admin panel - presumably the config wouldn't be saveable next time a customer edited it, till they removed the config setting.

Have we done this in the past? I feel like we must have ...

I guess that's true. I guess we're just ignoring it now if it's set so it doesn't matter if it's still there

Perhaps we should create a PR to label these in comments as deprecated, if we're not sure? Breadcrumbs for future cleanups?

From https://github.com/elastic/cloud/pull/133738#discussion_r1828211350, we can set a max value as we have to keep it around for deployments running 8.x for a long time.

ymao1 · 2024-11-25T19:20:58Z

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

@@ -362,104 +334,6 @@ describe('Task Runner', () => {
    ).toHaveBeenCalled();
  });

-  test.each(ephemeralTestParams)(
-    'actionsPlugin.execute is called per alert alert that is scheduled %s',


i think we want to keep all of these tests, just not the iteration over the different bulk enqueue execution functions. We want to keep testing that actionsClient.bulkEnqueueExecution is called in these scenarios.

ya, seems that way ...

pmuellr · 2024-11-26T15:24:54Z

Code LGTM, but have the same issues as Ying. Will review once the tests have been re-instated ...

JiaweiWu · 2024-12-02T08:52:30Z

@elasticmachine merge upstream

elasticmachine · 2024-12-02T10:58:03Z

💚 Build Succeeded

Buildkite Build
Commit: 5958a7d

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
`actions`	316	315	-1
`alerting`	849	848	-1
total			-2

Unknown metric groups

API count

id	before	after	diff
`actions`	322	321	-1
`alerting`	881	880	-1
total			-2

ESLint disabled line counts

id	before	after	diff
`@kbn/test-suites-xpack`	722	721	-1
`actions`	25	24	-1
total			-2

Total ESLint disabled count

id	before	after	diff
`@kbn/test-suites-xpack`	747	746	-1
`actions`	27	26	-1
total			-2

History

💚 Build #255475 succeeded b2981fa
💔 Build #252230 failed 684485f
💛 Build #252136 was flaky 5a2ed3f
💔 Build #250675 failed 9bcd9a9
💔 Build #245036 failed e60b9f9

ymao1

LGTM

elastic#197421) ## Summary Issue: elastic#151461 Removes all reference to ephemeral tasks in the alerting and actions plugin. As well as unit and E2E tests while maintaining backwards compatibility for `xpack.alerting.maxEphemeralActionsPerAlert` flag. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

## Summary Resolves: #151463 Removes all reference to ephemeral tasks from the task manager plugin. As well as unit and E2E tests while maintaining backwards compatibility for `xpack.task_manager.ephemeral_tasks` flag to no-op if set. This PR has some dependencies from the PR to remove ephemeral task support from the alerting and actions plugin (#197421). So it should be merged after the other PR. Deprecates the following configuration settings: - xpack.task_manager.ephemeral_tasks.enabled - xpack.task_manager.ephemeral_tasks.request_capacity The user doesn't have to change anything on their end if they don't wish to. This deprecation is made so if the above settings are defined, kibana will simply do nothing. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Remove ephemeral tasks from action and alerting plugins

e60b9f9

JiaweiWu 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) v9.0.0 labels Oct 23, 2024

JiaweiWu requested a review from a team as a code owner October 23, 2024 12:16

JiaweiWu added the backport:skip This commit does not require backporting label Oct 23, 2024

JiaweiWu changed the title ~~Remove ephemeral tasks from action and alerting plugins~~ [Response Ops] Remove ephemeral tasks from action and alerting plugins Oct 23, 2024

JiaweiWu and others added 2 commits October 24, 2024 13:57

Merge branch 'main' into issue-151461-remove-ephemeral-tasks

21ff4d2

fix CI

96dc47f

ymao1 reviewed Oct 24, 2024

View reviewed changes

mikecote reviewed Oct 25, 2024

View reviewed changes

Addressed comments

d8b76a1

mikecote mentioned this pull request Nov 4, 2024

[ResponseOps] Remove 7.x deprecated kibana.yml settings #198435

Merged

Merge conflicts

814585b

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

9bcd9a9

…-fix'

JiaweiWu added 2 commits November 18, 2024 10:55

Merge branch 'main' into issue-151461-remove-ephemeral-tasks

270fbc3

Fix type check and remove unused test

5a2ed3f

JiaweiWu and others added 2 commits November 18, 2024 23:01

remove ephemeral task check in cleanup function

ffe4a29

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

684485f

…-fix'

mikecote approved these changes Nov 21, 2024

View reviewed changes

JiaweiWu mentioned this pull request Nov 22, 2024

[Response Ops] Remove ephemeral tasks from task manager plugin #201313

Merged

1 task

ymao1 reviewed Nov 25, 2024

View reviewed changes

JiaweiWu added 2 commits November 28, 2024 16:04

Merge branch 'main' into issue-151461-remove-ephemeral-tasks

6935204

Bring back task runner tests and address comments

b2981fa

Merge branch 'main' into issue-151461-remove-ephemeral-tasks

5958a7d

ymao1 approved these changes Dec 2, 2024

View reviewed changes

JiaweiWu merged commit 7e71e5a into elastic:main Dec 2, 2024
8 checks passed

JiaweiWu added release_note:deprecation and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Response Ops] Remove ephemeral tasks from action and alerting plugins #197421

[Response Ops] Remove ephemeral tasks from action and alerting plugins #197421

JiaweiWu commented Oct 23, 2024 •

edited

Loading

elasticmachine commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

ymao1 Oct 24, 2024

mikecote Oct 25, 2024

JiaweiWu commented Nov 13, 2024

mikecote commented Nov 13, 2024

JiaweiWu commented Nov 18, 2024

mikecote commented Nov 19, 2024

mikecote left a comment

mikecote Nov 21, 2024

ymao1 Nov 25, 2024

pmuellr Nov 26, 2024

ymao1 Nov 26, 2024

pmuellr Nov 27, 2024

mikecote Nov 27, 2024 •

edited

Loading

ymao1 Nov 25, 2024

pmuellr Nov 26, 2024

pmuellr commented Nov 26, 2024

JiaweiWu commented Dec 2, 2024

elasticmachine commented Dec 2, 2024

API count

ESLint disabled line counts

Total ESLint disabled count

ymao1 left a comment

[Response Ops] Remove ephemeral tasks from action and alerting plugins #197421

[Response Ops] Remove ephemeral tasks from action and alerting plugins #197421

Conversation

JiaweiWu commented Oct 23, 2024 • edited Loading

Summary

Checklist

elasticmachine commented Oct 23, 2024

github-actions bot commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaweiWu commented Nov 13, 2024

mikecote commented Nov 13, 2024

JiaweiWu commented Nov 18, 2024

mikecote commented Nov 19, 2024

mikecote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikecote Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmuellr commented Nov 26, 2024

JiaweiWu commented Dec 2, 2024

elasticmachine commented Dec 2, 2024

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

ymao1 left a comment

Choose a reason for hiding this comment

JiaweiWu commented Oct 23, 2024 •

edited

Loading

mikecote Nov 27, 2024 •

edited

Loading