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

[Security Solution] Fix value lists tests flakiness #164253

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Aug 19, 2023

Fixes: #164056

Summary

This PR fixes value_lists.cy.ts tests flakiness.

The flakiness reason

Value list items are processed in a bulk via bulk creation and refresh=wait_for is used. The problem it returns sometimes earlier than data is available. Bulk API docs say the following

Only the shards that receive the bulk request will be affected by refresh. Imagine a _bulk?refresh=wait_for request with three documents in it that happen to be routed to different shards in an index with five shards. The request will only wait for those three shards to refresh. The other two shards that make up the index do not participate in the _bulk request at all.

While (it seems) only one shard is used in tests but it still cause issues (approx. 1 test per 50 fails) so adding explicit index refresh helps to get rid of flakiness.

Flaky test runner

value_lists.cy.ts (150 runs) 🟢

@maximpn maximpn added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team labels Aug 19, 2023
@maximpn maximpn self-assigned this Aug 19, 2023
@maximpn maximpn force-pushed the check-list-items-refresh branch from 15e5bc3 to ae74116 Compare August 19, 2023 17:11
@maximpn maximpn changed the title [Security Solution] check value list bulk update refresh [Security Solution] Fix value lists tests flakiness Aug 19, 2023
@maximpn maximpn force-pushed the check-list-items-refresh branch from a87fd8d to 8bd2d25 Compare August 19, 2023 19:00
@maximpn maximpn added test release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Feature:Rule Value Lists Security Solution Detection Rule Value Lists area v8.10.0 labels Aug 19, 2023
@maximpn maximpn requested a review from jpdjere August 19, 2023 19:03
@maximpn maximpn marked this pull request as ready for review August 20, 2023 11:13
@maximpn maximpn requested a review from a team as a code owner August 20, 2023 11:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@maximpn maximpn force-pushed the check-list-items-refresh branch 2 times, most recently from 98625da to e6b4658 Compare August 21, 2023 10:19
Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

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

Changes look good to me, but letting @elastic/security-detection-engine review it

Copy link
Contributor

@e40pud e40pud left a comment

Choose a reason for hiding this comment

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

LGTM

@maximpn maximpn force-pushed the check-list-items-refresh branch from e6b4658 to ceaac99 Compare August 21, 2023 12:22
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

  • 💛 Build #151794 was flaky e6b4658e6785ee5c6b1de3e6721b8af6143f8b54
  • 💔 Build #151763 failed 98625daab5f30a0d390313ef4f1cf0f7b8e58fd8
  • 💛 Build #151677 was flaky 8bd2d25ac03f65a0f71a6c85e32bd6118e55effc
  • 💔 Build #151672 failed a87fd8d580cca2c016192f0529c795aa67e3a5c1
  • 💔 Build #151671 failed ae74116ca4be1fae16e41a80cb7c4cb2b563e83d
  • 💔 Build #151668 failed 15e5bc33e490124cae481569a7892723042c080c

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

cc @maximpn

@banderror banderror added v8.11.0 and removed backport:skip This commit does not require backporting labels Aug 21, 2023
@maximpn maximpn merged commit d34c845 into elastic:main Aug 21, 2023
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.10

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 21, 2023
**Fixes:** elastic#164056

## Summary

This PR fixes [value_lists.cy.ts](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts) tests flakiness.

## The flakiness reason

Value list items are processed in a bulk via bulk creation and `refresh=wait_for` is [used](https://github.com/elastic/kibana/blob/main/x-pack/plugins/lists/server/services/items/create_list_items_bulk.ts#L87). The problem it returns sometimes earlier than data is available. [Bulk API docs](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh) say the following

> Only the shards that receive the bulk request will be affected by refresh. Imagine a _bulk?refresh=wait_for request with three documents in it that happen to be routed to different shards in an index with five shards. The request will only wait for those three shards to refresh. The other two shards that make up the index do not participate in the _bulk request at all.

While (it seems) only one shard is used in tests but it still cause issues (approx. 1 test per 50 fails) so adding explicit index refresh helps to get rid of flakiness.

## Flaky test runner

[value_lists.cy.ts (150 runs)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2924) 🟢

(cherry picked from commit d34c845)
kibanamachine added a commit that referenced this pull request Aug 21, 2023
…164324)

# Backport

This will backport the following commits from `main` to `8.10`:
- [[Security Solution] Fix value lists tests flakiness
(#164253)](#164253)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-21T14:31:21Z","message":"[Security
Solution] Fix value lists tests flakiness (#164253)\n\n**Fixes:**
https://github.com/elastic/kibana/issues/164056\r\n\r\n##
Summary\r\n\r\nThis PR fixes
[value_lists.cy.ts](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts)
tests flakiness.\r\n\r\n## The flakiness reason\r\n\r\nValue list items
are processed in a bulk via bulk creation and `refresh=wait_for` is
[used](https://github.com/elastic/kibana/blob/main/x-pack/plugins/lists/server/services/items/create_list_items_bulk.ts#L87).
The problem it returns sometimes earlier than data is available. [Bulk
API
docs](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh)
say the following\r\n\r\n> Only the shards that receive the bulk request
will be affected by refresh. Imagine a _bulk?refresh=wait_for request
with three documents in it that happen to be routed to different shards
in an index with five shards. The request will only wait for those three
shards to refresh. The other two shards that make up the index do not
participate in the _bulk request at all.\r\n\r\nWhile (it seems) only
one shard is used in tests but it still cause issues (approx. 1 test per
50 fails) so adding explicit index refresh helps to get rid of
flakiness.\r\n\r\n## Flaky test runner\r\n\r\n[value_lists.cy.ts (150
runs)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2924)
🟢","sha":"d34c845955e2cb1cd6bae630ac3d0d551544f916","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","release_note:skip","Team:Detections
and Resp","Team: SecuritySolution","Feature:Rule Value
Lists","Team:Detection Rule
Management","v8.10.0","v8.11.0"],"number":164253,"url":"https://github.com/elastic/kibana/pull/164253","mergeCommit":{"message":"[Security
Solution] Fix value lists tests flakiness (#164253)\n\n**Fixes:**
https://github.com/elastic/kibana/issues/164056\r\n\r\n##
Summary\r\n\r\nThis PR fixes
[value_lists.cy.ts](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts)
tests flakiness.\r\n\r\n## The flakiness reason\r\n\r\nValue list items
are processed in a bulk via bulk creation and `refresh=wait_for` is
[used](https://github.com/elastic/kibana/blob/main/x-pack/plugins/lists/server/services/items/create_list_items_bulk.ts#L87).
The problem it returns sometimes earlier than data is available. [Bulk
API
docs](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh)
say the following\r\n\r\n> Only the shards that receive the bulk request
will be affected by refresh. Imagine a _bulk?refresh=wait_for request
with three documents in it that happen to be routed to different shards
in an index with five shards. The request will only wait for those three
shards to refresh. The other two shards that make up the index do not
participate in the _bulk request at all.\r\n\r\nWhile (it seems) only
one shard is used in tests but it still cause issues (approx. 1 test per
50 fails) so adding explicit index refresh helps to get rid of
flakiness.\r\n\r\n## Flaky test runner\r\n\r\n[value_lists.cy.ts (150
runs)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2924)
🟢","sha":"d34c845955e2cb1cd6bae630ac3d0d551544f916"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/164253","number":164253,"mergeCommit":{"message":"[Security
Solution] Fix value lists tests flakiness (#164253)\n\n**Fixes:**
https://github.com/elastic/kibana/issues/164056\r\n\r\n##
Summary\r\n\r\nThis PR fixes
[value_lists.cy.ts](https://github.com/elastic/kibana/blob/main/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/value_lists/value_lists.cy.ts)
tests flakiness.\r\n\r\n## The flakiness reason\r\n\r\nValue list items
are processed in a bulk via bulk creation and `refresh=wait_for` is
[used](https://github.com/elastic/kibana/blob/main/x-pack/plugins/lists/server/services/items/create_list_items_bulk.ts#L87).
The problem it returns sometimes earlier than data is available. [Bulk
API
docs](https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh)
say the following\r\n\r\n> Only the shards that receive the bulk request
will be affected by refresh. Imagine a _bulk?refresh=wait_for request
with three documents in it that happen to be routed to different shards
in an index with five shards. The request will only wait for those three
shards to refresh. The other two shards that make up the index do not
participate in the _bulk request at all.\r\n\r\nWhile (it seems) only
one shard is used in tests but it still cause issues (approx. 1 test per
50 fails) so adding explicit index refresh helps to get rid of
flakiness.\r\n\r\n## Flaky test runner\r\n\r\n[value_lists.cy.ts (150
runs)](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2924)
🟢","sha":"d34c845955e2cb1cd6bae630ac3d0d551544f916"}}]}] BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
@maximpn maximpn deleted the check-list-items-refresh branch August 21, 2023 14:44
@jpdjere jpdjere added the flake-docs Temp label to gather PRs used to create dev docs label Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Rule Value Lists Security Solution Detection Rule Value Lists area flake-docs Temp label to gather PRs used to create dev docs release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. test v8.10.0 v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants