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] Unskip perform bulk action ESS FTR tests #197660

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Oct 24, 2024

Closes: #196470
Closes: #196462

Summary

This PR unskips perform_bulk_action_ess.ts functional test.

Details

perform_bulk_action_ess.ts includes a number of functional tests where some of them were flaky. Investigation revealed that creating enabled rules and performing bulk actions may lead to race conditions. Under that conditions rule's SO isn't updated as expected. For example legacy rule actions aren't persisted in rule's SO when it's expected by the test.

This PR includes perform_bulk_action_ess.ts refactoring to create disabled rules in the majority of tests. Enabled rules are created only in tests checking behavior upon rules disabling. These tests were checked multiple times and didn't demonstrate flakiness.

Additionally @kbn/expect was replaced with expect to make asserting more transparent and avoid unclear error messages.

Flaky test results

100 runs 🟢

@maximpn maximpn added test failed-test A test failure on a tracked branch, potentially flaky-test release_note:skip Skip the PR/issue when compiling release notes 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 backport:prev-major Backport to (8.x, 8.18, 8.17, 8.16) the previous major branch and other branches in development labels Oct 24, 2024
@maximpn maximpn self-assigned this Oct 24, 2024
@maximpn maximpn force-pushed the unskip-perform-bulk-action-ess branch from 51d3ab2 to 8d2800d Compare October 25, 2024 04:42
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7259

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.

see run history

@elastic elastic deleted a comment from kibanamachine Oct 25, 2024
@maximpn maximpn force-pushed the unskip-perform-bulk-action-ess branch 2 times, most recently from 2ee6a9b to e4a79fb Compare October 26, 2024 05:47
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7272

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.

see run history

@maximpn maximpn force-pushed the unskip-perform-bulk-action-ess branch from e4a79fb to ce46713 Compare October 26, 2024 08:21
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7273

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.

see run history

@maximpn maximpn force-pushed the unskip-perform-bulk-action-ess branch from ce46713 to 6dfbf78 Compare October 28, 2024 07:12
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7274

[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/rules_management/rule_bulk_actions/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed.

see run history

@maximpn maximpn marked this pull request as ready for review October 28, 2024 09:00
@maximpn maximpn requested review from a team as code owners October 28, 2024 09:00
@maximpn maximpn requested a review from dplumlee October 28, 2024 09:00
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@xcrzx
Copy link
Contributor

xcrzx commented Oct 28, 2024

Investigation revealed that creating enabled rules and performing bulk actions may lead to race conditions. Under that conditions rule's SO isn't updated as expected.

@maximpn Could you expand on the details of the race condition? If the rule SO doesn't get updated properly in the test environment, is there a chance that this could also happen in production?

@xcrzx xcrzx self-requested a review October 28, 2024 09:50
@maximpn
Copy link
Contributor Author

maximpn commented Oct 28, 2024

@xcrzx I observed quite stable flakiness with the following scenario where a test does the following

  • creates an enabled rule
  • creates a legacy notification action
  • disable the rule via bulk action

ER: Rule's legacy notification action gets migrated.
AR: Around ~5-10% cases had the test failed since the rule doesn't have any actions at the end where it's expected.

For example should disable rules and migrate actions demonstrated such flakiness in Flaky Test Runner and locally.

Creation of only disabled rules fixed the flakiness. It looks like the task manager tries to run the rule the same time a bulk operation is performed.


I was about to create a ticket for DE team but after rebasing on the latest commit in main I don't observe such flakiness anymore.

I checked the legacy notification actions migration code together with bulk actions rules client code on potential issues like refresh: false for ES client but everything looks fine.

@xcrzx
Copy link
Contributor

xcrzx commented Oct 29, 2024

I was about to create a ticket for DE team but after rebasing on the latest commit in main I don't observe such flakiness anymore.

So the flakiness resolved itself without any changes to the tests? In that case, do we still need the refactoring introduced in this PR?

@maximpn
Copy link
Contributor Author

maximpn commented Oct 29, 2024

@xcrzx indeed it's just enough unskip the test since the problem seems to be fixed on itself. Though refactoring improves error messages. It should simplify understanding on what part failed if the test suddenly fails. For example there are two error messages below where the first one before the refactoring and the second is

before:

└- ✖ fail: Rules Management - Rule Bulk Action API @ess perform_bulk_action - ESS specific logic should disable rules and migrate actions
--
  | │      TypeError: Cannot read properties of undefined (reading 'uuid')

after:

└- ✖ fail: Rules Management - Rule Bulk Action API @ess perform_bulk_action - ESS specific logic edit action should migrate legacy actions on edit
--
  | │      Error: expect(received).toEqual(expected) // deep equality
  | │
  | │ - Expected  - 17
  | │ + Received  +  1
  | │
  | │ - Array [
  | │ -   Object {
  | │ -     "action_type_id": ".slack",
  | │ -     "frequency": Object {
  | │ -       "notifyWhen": "onThrottleInterval",
  | │ -       "summary": true,
  | │ -       "throttle": "1h",
  | │ -     },
  | │ -     "group": "default",
  | │ -     "id": "1b73eb9e-54fa-42ff-8bc3-a1dd686f8423",
  | │ -     "params": Object {
  | │ -       "message": "Hourly
  | │ - Rule {{context.rule.name}} generated {{state.signals_count}} alerts",
  | │ -     },
  | │ -     "uuid": Any<String>,
  | │ -   },
  | │ - ]
  | │ + Array []

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

Thanks for the additional info. I looked through the changes and the refactoring looks good 👍

@maximpn maximpn enabled auto-merge (squash) November 5, 2024 08:40
@maximpn maximpn merged commit bf37a01 into elastic:main Nov 5, 2024
23 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.15, 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11682191876

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #45 / discover/group1 discover test nested query should support querying on nested fields

Metrics [docs]

✅ unchanged

History

cc @maximpn

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 5, 2024
…#197660)

**Closes: elastic#196470
**Closes: elastic#196462

## Summary

This PR unskips `perform_bulk_action_ess.ts` functional test.

## Details

`perform_bulk_action_ess.ts` includes a number of functional tests where some of them were flaky. Investigation revealed that creating enabled rules and performing bulk actions may lead to race conditions. Under that conditions rule's SO isn't updated as expected. For example legacy rule actions aren't persisted in rule's SO when it's expected by the test.

This PR includes `perform_bulk_action_ess.ts` refactoring to create disabled rules in the majority of tests. Enabled rules are created only in tests checking behavior upon rules disabling. These tests were checked multiple times and didn't demonstrate flakiness.

Additionally `@kbn/expect` was replaced with `expect` to make asserting more transparent and avoid unclear error messages.

## Flaky test results

[100 runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274) 🟢

(cherry picked from commit bf37a01)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 5, 2024
…#197660)

**Closes: elastic#196470
**Closes: elastic#196462

## Summary

This PR unskips `perform_bulk_action_ess.ts` functional test.

## Details

`perform_bulk_action_ess.ts` includes a number of functional tests where some of them were flaky. Investigation revealed that creating enabled rules and performing bulk actions may lead to race conditions. Under that conditions rule's SO isn't updated as expected. For example legacy rule actions aren't persisted in rule's SO when it's expected by the test.

This PR includes `perform_bulk_action_ess.ts` refactoring to create disabled rules in the majority of tests. Enabled rules are created only in tests checking behavior upon rules disabling. These tests were checked multiple times and didn't demonstrate flakiness.

Additionally `@kbn/expect` was replaced with `expect` to make asserting more transparent and avoid unclear error messages.

## Flaky test results

[100 runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274) 🟢

(cherry picked from commit bf37a01)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.15 Backport failed because of merge conflicts
8.16
8.x

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

Manual backport

To create the backport manually run:

node scripts/backport --pr 197660

Questions ?

Please refer to the Backport tool documentation

Copy link

@sharie90 sharie90 left a comment

Choose a reason for hiding this comment

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

Ok

Copy link

@sharie90 sharie90 left a comment

Choose a reason for hiding this comment

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

[]()

@maximpn maximpn deleted the unskip-perform-bulk-action-ess branch November 5, 2024 10:49
kibanamachine added a commit that referenced this pull request Nov 5, 2024
…197660) (#198908)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Security Solution] Unskip perform bulk action ESS FTR tests
(#197660)](#197660)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-05T10:24:37Z","message":"[Security
Solution] Unskip perform bulk action ESS FTR tests
(#197660)\n\n**Closes:
https://github.com/elastic/kibana/issues/196470**\r\n**Closes:
https://github.com/elastic/kibana/issues/196462**\r\n\r\n##
Summary\r\n\r\nThis PR unskips `perform_bulk_action_ess.ts` functional
test.\r\n\r\n## Details\r\n\r\n`perform_bulk_action_ess.ts` includes a
number of functional tests where some of them were flaky. Investigation
revealed that creating enabled rules and performing bulk actions may
lead to race conditions. Under that conditions rule's SO isn't updated
as expected. For example legacy rule actions aren't persisted in rule's
SO when it's expected by the test.\r\n\r\nThis PR includes
`perform_bulk_action_ess.ts` refactoring to create disabled rules in the
majority of tests. Enabled rules are created only in tests checking
behavior upon rules disabling. These tests were checked multiple times
and didn't demonstrate flakiness.\r\n\r\nAdditionally `@kbn/expect` was
replaced with `expect` to make asserting more transparent and avoid
unclear error messages. \r\n\r\n## Flaky test results\r\n\r\n[100
runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274)
🟢","sha":"bf37a019d857e89d6dad3a6cf450ec323f0783e0","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","failed-test","release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","backport:prev-major"],"title":"[Security Solution] Unskip
perform bulk action ESS FTR
tests","number":197660,"url":"https://github.com/elastic/kibana/pull/197660","mergeCommit":{"message":"[Security
Solution] Unskip perform bulk action ESS FTR tests
(#197660)\n\n**Closes:
https://github.com/elastic/kibana/issues/196470**\r\n**Closes:
https://github.com/elastic/kibana/issues/196462**\r\n\r\n##
Summary\r\n\r\nThis PR unskips `perform_bulk_action_ess.ts` functional
test.\r\n\r\n## Details\r\n\r\n`perform_bulk_action_ess.ts` includes a
number of functional tests where some of them were flaky. Investigation
revealed that creating enabled rules and performing bulk actions may
lead to race conditions. Under that conditions rule's SO isn't updated
as expected. For example legacy rule actions aren't persisted in rule's
SO when it's expected by the test.\r\n\r\nThis PR includes
`perform_bulk_action_ess.ts` refactoring to create disabled rules in the
majority of tests. Enabled rules are created only in tests checking
behavior upon rules disabling. These tests were checked multiple times
and didn't demonstrate flakiness.\r\n\r\nAdditionally `@kbn/expect` was
replaced with `expect` to make asserting more transparent and avoid
unclear error messages. \r\n\r\n## Flaky test results\r\n\r\n[100
runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274)
🟢","sha":"bf37a019d857e89d6dad3a6cf450ec323f0783e0"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197660","number":197660,"mergeCommit":{"message":"[Security
Solution] Unskip perform bulk action ESS FTR tests
(#197660)\n\n**Closes:
https://github.com/elastic/kibana/issues/196470**\r\n**Closes:
https://github.com/elastic/kibana/issues/196462**\r\n\r\n##
Summary\r\n\r\nThis PR unskips `perform_bulk_action_ess.ts` functional
test.\r\n\r\n## Details\r\n\r\n`perform_bulk_action_ess.ts` includes a
number of functional tests where some of them were flaky. Investigation
revealed that creating enabled rules and performing bulk actions may
lead to race conditions. Under that conditions rule's SO isn't updated
as expected. For example legacy rule actions aren't persisted in rule's
SO when it's expected by the test.\r\n\r\nThis PR includes
`perform_bulk_action_ess.ts` refactoring to create disabled rules in the
majority of tests. Enabled rules are created only in tests checking
behavior upon rules disabling. These tests were checked multiple times
and didn't demonstrate flakiness.\r\n\r\nAdditionally `@kbn/expect` was
replaced with `expect` to make asserting more transparent and avoid
unclear error messages. \r\n\r\n## Flaky test results\r\n\r\n[100
runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274)
🟢","sha":"bf37a019d857e89d6dad3a6cf450ec323f0783e0"}}]}] BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
kibanamachine added a commit that referenced this pull request Nov 5, 2024
…197660) (#198909)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Unskip perform bulk action ESS FTR tests
(#197660)](#197660)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-05T10:24:37Z","message":"[Security
Solution] Unskip perform bulk action ESS FTR tests
(#197660)\n\n**Closes:
https://github.com/elastic/kibana/issues/196470**\r\n**Closes:
https://github.com/elastic/kibana/issues/196462**\r\n\r\n##
Summary\r\n\r\nThis PR unskips `perform_bulk_action_ess.ts` functional
test.\r\n\r\n## Details\r\n\r\n`perform_bulk_action_ess.ts` includes a
number of functional tests where some of them were flaky. Investigation
revealed that creating enabled rules and performing bulk actions may
lead to race conditions. Under that conditions rule's SO isn't updated
as expected. For example legacy rule actions aren't persisted in rule's
SO when it's expected by the test.\r\n\r\nThis PR includes
`perform_bulk_action_ess.ts` refactoring to create disabled rules in the
majority of tests. Enabled rules are created only in tests checking
behavior upon rules disabling. These tests were checked multiple times
and didn't demonstrate flakiness.\r\n\r\nAdditionally `@kbn/expect` was
replaced with `expect` to make asserting more transparent and avoid
unclear error messages. \r\n\r\n## Flaky test results\r\n\r\n[100
runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274)
🟢","sha":"bf37a019d857e89d6dad3a6cf450ec323f0783e0","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["test","failed-test","release_note:skip","v9.0.0","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","backport:prev-major"],"title":"[Security Solution] Unskip
perform bulk action ESS FTR
tests","number":197660,"url":"https://github.com/elastic/kibana/pull/197660","mergeCommit":{"message":"[Security
Solution] Unskip perform bulk action ESS FTR tests
(#197660)\n\n**Closes:
https://github.com/elastic/kibana/issues/196470**\r\n**Closes:
https://github.com/elastic/kibana/issues/196462**\r\n\r\n##
Summary\r\n\r\nThis PR unskips `perform_bulk_action_ess.ts` functional
test.\r\n\r\n## Details\r\n\r\n`perform_bulk_action_ess.ts` includes a
number of functional tests where some of them were flaky. Investigation
revealed that creating enabled rules and performing bulk actions may
lead to race conditions. Under that conditions rule's SO isn't updated
as expected. For example legacy rule actions aren't persisted in rule's
SO when it's expected by the test.\r\n\r\nThis PR includes
`perform_bulk_action_ess.ts` refactoring to create disabled rules in the
majority of tests. Enabled rules are created only in tests checking
behavior upon rules disabling. These tests were checked multiple times
and didn't demonstrate flakiness.\r\n\r\nAdditionally `@kbn/expect` was
replaced with `expect` to make asserting more transparent and avoid
unclear error messages. \r\n\r\n## Flaky test results\r\n\r\n[100
runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274)
🟢","sha":"bf37a019d857e89d6dad3a6cf450ec323f0783e0"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197660","number":197660,"mergeCommit":{"message":"[Security
Solution] Unskip perform bulk action ESS FTR tests
(#197660)\n\n**Closes:
https://github.com/elastic/kibana/issues/196470**\r\n**Closes:
https://github.com/elastic/kibana/issues/196462**\r\n\r\n##
Summary\r\n\r\nThis PR unskips `perform_bulk_action_ess.ts` functional
test.\r\n\r\n## Details\r\n\r\n`perform_bulk_action_ess.ts` includes a
number of functional tests where some of them were flaky. Investigation
revealed that creating enabled rules and performing bulk actions may
lead to race conditions. Under that conditions rule's SO isn't updated
as expected. For example legacy rule actions aren't persisted in rule's
SO when it's expected by the test.\r\n\r\nThis PR includes
`perform_bulk_action_ess.ts` refactoring to create disabled rules in the
majority of tests. Enabled rules are created only in tests checking
behavior upon rules disabling. These tests were checked multiple times
and didn't demonstrate flakiness.\r\n\r\nAdditionally `@kbn/expect` was
replaced with `expect` to make asserting more transparent and avoid
unclear error messages. \r\n\r\n## Flaky test results\r\n\r\n[100
runs](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7274)
🟢","sha":"bf37a019d857e89d6dad3a6cf450ec323f0783e0"}}]}] BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-major Backport to (8.x, 8.18, 8.17, 8.16) the previous major branch and other branches in development failed-test A test failure on a tracked branch, potentially flaky-test 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.16.0 v8.17.0 v9.0.0
Projects
None yet
5 participants