[Security Solution] Fix flaky test: x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts - update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package #163241

jpdjere · 2023-08-07T07:39:12Z

Fixes: #162658

Summary

Fixes flaky test: x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts
Test title: update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package

Passing flaky test runner

300 runs: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2799

Root cause and what this PR does

On initial investigation, the flaky test runner was executed for this test with 300 iterations and all of them succeeded. This gives us great confidence that the test is not actually flaky.
Further investigation showed that @kibanamachine reported this tests as failing when running the CI for two backport PRs to 8.9:
- https://buildkite.com/elastic/kibana-on-merge/builds/33282#0189987d-3a80-49c2-8332-3105ec3c2109
- https://buildkite.com/elastic/kibana-on-merge/builds/33444#0189b1fa-4bc4-4422-9ce9-5c9a24f11ad5
These flakiness was caused by a race condition between the writing of rules into indeces, and them being made available for reading. This is a known issue, already and solved for other integration test, by manually refreshing ES's indeces.
In order to reduce the probability of flakiness in a similar scenario, this PR adds code to refresh the indices after each rule installation or upgrade during the test.

Refactor

Moves the refreshing of the indexes within the utility function that write to indexes:
- installPrebuiltRules
- upgradePrebuiltRules
- installPrebuiltRulesAndTimelines (legacy, but still used)
- installPrebuiltRulesFleetPackage
Creates 2 new utils:
- installPrebuiltRulesPackageByVersion, which installs security_detection_engine package via Fleet API, with its version passed in as param
- getInstalledRules, reusable function to fetch all installed rules

jpdjere · 2023-08-07T07:40:15Z

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2795

jpdjere · 2023-08-07T10:11:02Z

Second attempt - 300 runs: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2799

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2023-08-08T15:10:31Z

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

elasticmachine · 2023-08-08T15:10:32Z

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

maximpn

@jpdjere thank you for fixing this flaky test 👍

Could you describe the root cause in the ticket's description? I'd be good to isolate es.indices.refresh() to the utility functions where it's needed and have a clear comment describing why it's needed there.

Have you checked also all the above requests have ?refresh=wait_for so the changed data is available? I'm a bit afraid we don't make it clear here in tests so any further readers will be confused.

maximpn · 2023-08-10T10:10:49Z

...tegration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package.ts

@@ -133,6 +134,8 @@ export default ({ getService }: FtrProviderContext): void => {
      // Verify that the _perform endpoint returns the same number of installed rules as the status endpoint
      // and the _review endpoint
      const installPrebuiltRulesResponse = await installPrebuiltRules(supertest);
+      await es.indices.refresh({ index: ALL_SAVED_OBJECT_INDICES });


It's quite easy to forget why es.indices.refresh() is needed and what part of test it's related to. It'd be nicer to have it right before an HTTP call depending on it so anybody reusing it doesn't have to keep in mind es.indices.refresh() has to be called before. For example getPrebuiltRulesStatus() should contain es.indices.refresh() inside.

maximpn · 2023-08-10T10:12:22Z

...tegration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package.ts

@@ -133,6 +134,8 @@ export default ({ getService }: FtrProviderContext): void => {
      // Verify that the _perform endpoint returns the same number of installed rules as the status endpoint
      // and the _review endpoint
      const installPrebuiltRulesResponse = await installPrebuiltRules(supertest);
+      await es.indices.refresh({ index: ALL_SAVED_OBJECT_INDICES });
+


Installed rules fetching chunk looks to be repeated. It could be a reusable function with es.indices.refresh() inside

const { body: rulesResponse } = await supertest .get(`${DETECTION_ENGINE_RULES_URL_FIND}?per_page=10000`) .set('kbn-xsrf', 'true') .send() .expect(200);

jpdjere · 2023-08-10T14:39:02Z

@maximpn Refactored the utils according to your feedback.

In the case of utility functions that write to an index, I moved the refresh method to within the util and added a comment of why there. Notice that this is done after writing to the index for each case, to leave the test in a state in which it is ready and updated for any subsequent read operation and assertion on those results.

Also moved the "find rules" to its own utility getInstalledRules; this one doesn't need an index refresh since it's a read operation.

maximpn

@jpdjere thank you for addressing my comments 🙏

maximpn · 2023-08-11T10:50:17Z

x-pack/test/detection_engine_api_integration/utils/prebuilt_rules/get_installed_rules.ts

+ * @returns Fleet install package response
+ */
+
+export const getInstalledRules = async (


nit: IMHO fetchInstalledRules would be the best name for the functionality under the hood as it clearly describes that it fetches the data presumable from an endpoint. While get usually used for sync functions extracting some data like Map.get. At the same time getInstalledRules conforms with the other functions in the folder like getPrebuiltRulesStatus. Ideally getPrebuiltRulesStatus should be renamed to fetchPrebuiltRulesStatus as well as the others but it definitely out of scope of this PR.

Makes sense. I'll address this in any of the following PRs I'm working on. Thanks for all the feedback!

kibana-ci · 2023-08-11T12:18:14Z

💚 Build Succeeded

Buildkite Build
Commit: 4857be9

Metrics [docs]

✅ unchanged

History

💚 Build #149294 succeeded 4b11128
💔 Build #149260 failed 2d0a116
💚 Build #148411 succeeded ce1cb78

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

cc @jpdjere

* main: (64 commits) [ML] Transforms: Fix privileges check. (elastic#163687) [Log Explorer] Add test suite for Dataset Selector (elastic#163079) [Security Solution][Endpoint] Add API checks to Endpoint Policy create/update for checking `endpointPolicyProtections` is enabled (elastic#163429) [Security Solution] Fix flaky test: x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts - update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package (elastic#163241) [Security Solution] expandable flyout - replace feature flag with advanced settings toggle (elastic#161614) [DOCS] Adds the release notes for the 8.9.1 release. (elastic#163578) [FTR] Implement browser network condition utils (elastic#163633) [Security Solution] Unskip rules table auto-refresh Cypress tests (elastic#163451) [Security Solution] Re-enable fixed rule snoozing Cypress test (elastic#160037) [Flaky Test elastic#111821] Mock `moment` to avoid midnight TZ issues (elastic#163157) Document interactive setup (elastic#163619) [Lens] Align decoration color with text color for layer actions (elastic#163630) [Lens] Relax counter field checks for saved visualizations with unsupported operations (elastic#163515) [Security Solution][Endpoint] Removes pMap and uses a for loop instead (elastic#163509) [Enterprise Search] Update Workplace Search connectors doclink (elastic#163676) Update APM (main) (elastic#163623) [Serverless] Partially fix lens/maps/visualize breadcrumbs missing title (elastic#163476) [Flaky elastic#118272] Unskip tests (elastic#163319) [APM] Make service group saved objects exportable (elastic#163569) [Observability AI Assistant] Action menu item (elastic#163463) ...

@kibanamachine

…e_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts - update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package (#164558) **NOTE: This is a manual backport of #163241. It had to be done manually because of changes that happened from 8.9 -> 8.10 like the common folder restructuring and some type changes because of the OpenAPI effort, but it is mostly the changes to the same files as that PR.** **Original PR description follows:** Fixes: #162658 ## Summary - Fixes flaky test: `x-pack/test/detection_engine_api_integration/security_and_spaces/update_prebuilt_rules_package/update_prebuilt_rules_package·ts` - Test title: `update_prebuilt_rules_package should allow user to install prebuilt rules from scratch, then install new rules and upgrade existing rules from the new package` ## Passing flaky test runner 300 runs: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2799 ## Root cause and what this PR does - On initial investigation, the flaky test runner was executed for this test with 300 iterations and all of them succeeded. This gives us great confidence that the test is not actually flaky. - Further investigation showed that @kibanamachine reported this tests as failing when running the CI for two backport PRs to `8.9`: - https://buildkite.com/elastic/kibana-on-merge/builds/33282#0189987d-3a80-49c2-8332-3105ec3c2109 - https://buildkite.com/elastic/kibana-on-merge/builds/33444#0189b1fa-4bc4-4422-9ce9-5c9a24f11ad5 - These flakiness was caused **by a race condition** between the writing of rules into indeces, and them being made available for reading. This is a known issue, already and solved for other integration test, by manually refreshing ES's indeces. - In order to reduce the probability of flakiness in a similar scenario, this PR adds code to refresh the indices after each rule installation or upgrade during the test. ## Refactor - Moves the refreshing of the indexes within the utility function that write to indexes: - `installPrebuiltRules` - `upgradePrebuiltRules` - `installPrebuiltRulesAndTimelines` (legacy, but still used) - `installPrebuiltRulesFleetPackage` - Creates 2 new utils: - `installPrebuiltRulesPackageByVersion`, which installs `security_detection_engine` package via Fleet API, with its version passed in as param - `getInstalledRules`, reusable function to fetch all installed rules -

RUn flaky tets runner

a25be3f

jpdjere self-assigned this Aug 7, 2023

jpdjere and others added 2 commits August 8, 2023 16:53

Add indices refresh after installing and upgrading rules

d7472f8

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

e8395b3

…-ref HEAD~1..HEAD --fix'

jpdjere requested a review from maximpn August 8, 2023 15:09

jpdjere marked this pull request as ready for review August 8, 2023 15:10

jpdjere requested a review from a team as a code owner August 8, 2023 15:10

Merge branch 'main' into update-prebuilt-rules-package-flay-test-runner

ce1cb78

maximpn reviewed Aug 10, 2023

View reviewed changes

Refactor

7ee84f3

jpdjere added 2 commits August 10, 2023 16:44

Merge branch 'main' into update-prebuilt-rules-package-flay-test-runner

2d0a116

Fix

4b11128

maximpn approved these changes Aug 11, 2023

View reviewed changes

jpdjere added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Aug 11, 2023

Merge branch 'main' into update-prebuilt-rules-package-flay-test-runner

4857be9

jpdjere removed the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Aug 11, 2023

jpdjere merged commit 2ba659d into elastic:main Aug 11, 2023

kibanamachine added the backport:skip This commit does not require backporting label Aug 11, 2023

jpdjere added the flake-docs Temp label to gather PRs used to create dev docs label Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jpdjere commented Aug 7, 2023 •

edited

Loading

jpdjere commented Aug 7, 2023

jpdjere commented Aug 7, 2023

elasticmachine commented Aug 8, 2023

elasticmachine commented Aug 8, 2023

maximpn left a comment

maximpn Aug 10, 2023

maximpn Aug 10, 2023

jpdjere commented Aug 10, 2023

maximpn left a comment

maximpn Aug 11, 2023

jpdjere Aug 11, 2023

kibana-ci commented Aug 11, 2023

Conversation

jpdjere commented Aug 7, 2023 • edited Loading

Summary

Passing flaky test runner

Root cause and what this PR does

Refactor

jpdjere commented Aug 7, 2023

jpdjere commented Aug 7, 2023

elasticmachine commented Aug 8, 2023

elasticmachine commented Aug 8, 2023

maximpn left a comment

Choose a reason for hiding this comment

maximpn Aug 10, 2023

Choose a reason for hiding this comment

maximpn Aug 10, 2023

Choose a reason for hiding this comment

jpdjere commented Aug 10, 2023

maximpn left a comment

Choose a reason for hiding this comment

maximpn Aug 11, 2023

Choose a reason for hiding this comment

jpdjere Aug 11, 2023

Choose a reason for hiding this comment

kibana-ci commented Aug 11, 2023

💚 Build Succeeded

Metrics [docs]

History

jpdjere commented Aug 7, 2023 •

edited

Loading