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

Fix race condition in flaky alerting test #60438

Merged
merged 4 commits into from
Mar 19, 2020

Conversation

mikecote
Copy link
Contributor

In this PR, I'm fixing a race condition where the alert gets disabled before the cleanup of the action task params can finish.

I'm renaming in the task manager utils waitForIdle to waitForEmpty and adding a new waitForAllTasksIdle function that will be called before disabling an alert. This allows the actions that run after an alert execution to also finish running.

Reproducing steps

You can reproduce the flakiness by adding await new Promise(r => setTimeout(r, 3000));
here:

Fixes #58991
Fixes #58643

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Mar 17, 2020
@mikecote mikecote requested a review from a team as a code owner March 17, 2020 20:19
@mikecote mikecote self-assigned this Mar 17, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@mikecote
Copy link
Contributor Author

mikecote commented Mar 17, 2020

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.

LGTM

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I hate that we need this though, I'd really like to expose a proper TM api for this kind of thing.

@mikecote
Copy link
Contributor Author

I hate that we need this though, I'd really like to expose a proper TM api for this kind of thing.

Agreed, I'm also hoping the advanced API key invalidation (#53868) will help reduce the need for code like this. Since the general invalidation of API keys (#53732), we've had a lot of race conditions in our tests.

@mikecote
Copy link
Contributor Author

Found some flakiness (fixed in 413b6d2), doing another flaky test runner run here: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/264/.

@mikecote
Copy link
Contributor Author

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

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.

still LGTM

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@mikecote mikecote merged commit bafd45f into elastic:master Mar 19, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Mar 19, 2020
* Fix race condition in flaky test

* Fix flakiness in test

* Fix more flakiness
mikecote added a commit that referenced this pull request Mar 19, 2020
* Fix race condition in flaky test

* Fix flakiness in test

* Fix more flakiness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment