Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove task to cleanup action_task_params of failed tasks #151873

Merged
merged 18 commits into from
Mar 29, 2023

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Feb 22, 2023

Part of #79977 (step 2).
Resolves #79977.

In this PR, I'm removing the recurring task defined by the actions plugin that removes unused action_task_params SOs. With the #152841 PR, tasks will no longer get marked as failed and we have a migration script (excludeOnUpgrade) that removes all tasks and action_task_params that are leftover during the migration https://github.com/elastic/kibana/blob/main/x-pack/plugins/actions/server/saved_objects/index.ts#L81-L94.

NOTE: I will hold off merging this PR until #152841 is merged. (merged)

To verify

Not much to test here, but on a Kibana from main there will be this task type running in the background and moving to this PR will cause the task to get deleted because it is part of the REMOVED_TYPES array in Task Manager.

@mikecote mikecote 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) Feature:Actions/Framework Issues related to the Actions Framework v8.8.0 labels Feb 22, 2023
@mikecote mikecote self-assigned this Feb 22, 2023
@mikecote mikecote marked this pull request as ready for review March 24, 2023 17:29
@mikecote mikecote requested a review from a team as a code owner March 24, 2023 17:29
@elasticmachine
Copy link
Contributor

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

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote mikecote marked this pull request as draft March 27, 2023 18:04
@mikecote
Copy link
Contributor Author

Moving PR back to draft, the previous PR (#152841) caused a lot of flaky tests that I will focus on first.

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

References to deprecated APIs

id before after diff
actions 35 34 -1

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @mikecote

@mikecote mikecote marked this pull request as ready for review March 29, 2023 12:56
@pmuellr
Copy link
Member

pmuellr commented Mar 29, 2023

Reviewed the code, looked good, so tried a migration from 8.2.3 to this PR.

Weirdly, it looks like it migrated task manager in place - I know that's a new thing now, but since we did have migrations between then - adding the enabled field at least - I figured it wouldn't do it in place. But it did!

Here's _cat alias for TM indices:

.kibana_task_manager       .kibana_task_manager_8.2.3_001 - - - -
.kibana_task_manager_8.8.0 .kibana_task_manager_8.2.3_001 - - - -

So, the odd-ball thing is that the old task that SHOULD HAVE been deleted, is still there. With an enabled flag of true - so I know it got migrated. That seems to indicate some issue with us "removing" tasks during these "in place" migrations. At least I was expecting it to be deleted.

Thinking we should have someone else repro this, and if it's not me "holding it wrong", open an issue to get the "removed tasks" bit working again ...

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM; Kibana survived a migration from 8.2.3, but I left a comment in the PR because it turns out it didn't actually delete the old task (which I was expecting) - not a bug in this code though, so approving :-)

@pmuellr
Copy link
Member

pmuellr commented Mar 29, 2023

Reading through the "REMOVED_TYPES" processing in task manager, it looks like we don't actually delete the docs, we just don't search for them anymore. So I guess, working-as-designed? Seems weird to have enabled task docs in the index, that won't be read, based on a list of task types in the code. I wonder whether we should somehow make more explicit in the docs themselves, that they are no longer in use, if we're going to keep them around.

@mikecote
Copy link
Contributor Author

Good observation, @pmuellr! I created #153964 to discuss the oddity now that we are using REMOVED_TYPES more, so we can discuss if we'd like to do anything further. The original PR adding REMOVED_TYPES can be found here: #123963, basically we fixed the issue where tasks would get marked as unrecognized too soon and would have to re-create the task to have it working again. Deleting the task seems like a fair next step to investigate 👍

@mikecote mikecote merged commit a199cfc into elastic:main Mar 29, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 29, 2023
jgowdyelastic pushed a commit to jgowdyelastic/kibana that referenced this pull request Mar 30, 2023
…1873)

Part of elastic#79977 (step 2).
Resolves elastic#79977.

In this PR, I'm removing the recurring task defined by the actions
plugin that removes unused `action_task_params` SOs. With the
elastic#152841 PR, tasks will no longer
get marked as failed and we have a migration script (`excludeOnUpgrade`)
that removes all tasks and action_task_params that are leftover during
the migration
https://github.com/elastic/kibana/blob/main/x-pack/plugins/actions/server/saved_objects/index.ts#L81-L94.

~~NOTE: I will hold off merging this PR until
elastic#152841 is merged.~~ (merged)

## To verify

Not much to test here, but on a Kibana from `main` there will be this
task type running in the background and moving to this PR will cause the
task to get deleted because it is part of the `REMOVED_TYPES` array in
Task Manager.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Actions/Framework Issues related to the Actions Framework 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.8.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Failed Task Manager task documents are never cleaned up bloating the index
5 participants