[Security Solution] Add `retryIfConflict` util for `409` conflicts in Integration tests #174185

jpdjere · 2024-01-03T15:11:49Z

Summary

NOTE: the test where this was reported wasn't skipped, so this PR does not unskip any tests. However, the Flaky Test Runs help us determine that the issue is no longer reproducible.

The deleteAllPrebuiltRuleAssets utility reported a 409 Conflict, presumably from security-rule assets that were attempted to be deleted while they were being updated by a parallel process.

This PR wraps the es.deleteByQuery calls in the utils deleteAllPrebuiltRuleAssets and deleteAllTimelines with a new retryIfConflict helper, that will retry the operation if the ES request fails with a 409.

Flaky test run

bundled_prebuilt_rules_package - ESS and Serverless: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4790

large_prebuilt_rules_package - ESS and Serverless: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4791

update_prebuilt_rules_package - ESS and Serverless: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4792

management - ESS and Serverless: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4793

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-01-03T16:41:13Z

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

elasticmachine · 2024-01-03T16:41:14Z

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

elasticmachine · 2024-01-03T16:41:14Z

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

banderror

I don't think retryIfConflicts does what it should 🙂

x-pack/test/security_solution_api_integration/package.json

...ecurity_solution_api_integration/test_suites/detections_response/utils/retry_if_conflicts.ts

...est_suites/detections_response/utils/rules/prebuilt_rules/delete_all_prebuilt_rule_assets.ts

jpdjere · 2024-01-08T22:58:17Z

@banderror

Ok, so I had to rethink this a bit.

Responses from the Elasticsearch client vary from one type of operation to another, so there's no status or statusCode property that I can read from all different types of ES responses to check if they are 409. The response from DeleteByQuery looks like:

export interface DeleteByQueryResponse {
    batches?: long;
    deleted?: long;
    failures?: BulkIndexByScrollFailure[];
    noops?: long;
    requests_per_second?: float;
    // other stuff......
    total?: long;
    version_conflicts?: long;
}

and as you can see from the error in the ticket, the 409 error is reported in the failures array of the request body. And this failure prop doesn't exist in other ES responses.

(Also, I cannot get access to anything else apart from the body when doing requests directly to ES in integration tests)

So I refactored the code so that this utility is specific to retry DeleteByQuery operations if it hits 409 conflict errors, which seems to be, at least until now, the ES operation where this conflict is happening.

I think we can try to generalize this utility, or create other specific version of retries if we need them in the future, and use this as a first step. Let me know what you think.

Also, rerunning the Flaky test runner.

..._license/prebuilt_rules/large_prebuilt_rules_package/install_large_prebuilt_rules_package.ts

banderror · 2024-01-09T18:35:29Z

...ion_api_integration/test_suites/detections_response/utils/retry_delete_by_query_conflicts.ts

+  for (const failure of operationResult?.failures) {
+    if (failure.status === 409) {
+      // if no retries left, throw it
+      if (retries <= 0) {
+        logger.error(`${name} conflict, exceeded retries`);
+        throw new Error(`${name} conflict, exceeded retries`);
+      }


If we had this in the deleteAllPrebuiltRuleAssets function where we call the delete by query method, we could use the generic retry function and get rid of this retryIfDeleteByQueryConflicts.

If you think that retryIfDeleteByQueryConflicts has enough use cases to exist, maybe we could use the retry function inside of it to avoid conceptual code duplication between retry and retryIfDeleteByQueryConflicts.

...ion_api_integration/test_suites/detections_response/utils/retry_delete_by_query_conflicts.ts

banderror

The Flaky Test Runner is happy. Approving the PR to not being a blocker for it - posted only a bunch of minor comments. Thanks for addressing the feedback, LGTM 👍

kibana-ci · 2024-01-11T11:58:09Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: da3ad61

Failed CI Steps

Jest Tests #5

Metrics [docs]

✅ unchanged

History

💚 Build #186764 succeeded ebf4bd2
💛 Build #186495 was flaky c5e1050
💔 Build #186492 failed 0ebcb25
💚 Build #185775 succeeded 1088c87

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

cc @jpdjere

kibanamachine · 2024-01-11T12:49:58Z

💔 All backports failed

Status	Branch	Result
❌	8.12	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 174185

Questions ?

Please refer to the Backport tool documentation

… Integration tests (elastic#174185) ## Summary Fixes: elastic#171428 **NOTE: the test where this was reported wasn't skipped, so this PR does not unskip any tests.** However, the Flaky Test Runs help us determine that the issue is no longer reproducible. The `deleteAllPrebuiltRuleAssets` utility reported a `409 Conflict`, presumably from `security-rule` assets that were attempted to be deleted while they were being updated by a parallel process. This PR wraps the `es.deleteByQuery` calls in the utils `deleteAllPrebuiltRuleAssets` and `deleteAllTimelines` with a new `retryIfConflict` helper, that will retry the operation if the ES request fails with a `409`. ## Flaky test run `bundled_prebuilt_rules_package` - **ESS** and **Serverless**: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4790 `large_prebuilt_rules_package` - **ESS** and **Serverless**: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4791 `update_prebuilt_rules_package` - **ESS** and **Serverless**: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4792 `management` - **ESS** and **Serverless**: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4793 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit b8c7306) # Conflicts: # x-pack/test/security_solution_api_integration/package.json

jpdjere · 2024-01-12T14:19:32Z

💚 All backports created successfully

Status	Branch	Result
✅	8.12

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-01-12T21:48:59Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…icts in Integration tests (#174185) (#174762) # Backport This will backport the following commits from `main` to `8.12`: - [[Security Solution] Add `retryIfConflict` util for `409` conflicts in Integration tests (#174185)](#174185)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

mistic · 2024-01-17T16:56:59Z

This PR didn't make it into the latest BC for 8.12.0. Updating the labels.

…sts (#189813) Fixes #176445 ## Summary Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6679 This PR fixes the advanced settings API integration tests that seem to be flaky. The reason for the occasional failures is most likely a `version_conflict_engine_exception` which is thrown when another node indexes the same documents. This can happen when we save an advanced setting since the settings API uses saved objects under the hood, and in CI, multiple nodes can try to save an advanced setting simultaneously. The solution in this PR is to retry the request if we encounter a 409 error. This is adapted from the solution in #174185 which resolves a similar failure.

…sts (elastic#189813) Fixes elastic#176445 Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6679 This PR fixes the advanced settings API integration tests that seem to be flaky. The reason for the occasional failures is most likely a `version_conflict_engine_exception` which is thrown when another node indexes the same documents. This can happen when we save an advanced setting since the settings API uses saved objects under the hood, and in CI, multiple nodes can try to save an advanced setting simultaneously. The solution in this PR is to retry the request if we encounter a 409 error. This is adapted from the solution in elastic#174185 which resolves a similar failure. (cherry picked from commit cc29eea)

Add retryIfConflict util

6eefee5

jpdjere changed the title ~~Unskip Integration test - Add retryIfConflict util~~ [Security Solution] Add retryIfConflict util for 409 conflicts in Integration tests Jan 3, 2024

jpdjere added 2 commits January 3, 2024 17:38

fix package.json

1b48eb5

Merge branch 'main' into fix-flake-171428

dfaac2b

jpdjere marked this pull request as ready for review January 3, 2024 16:39

jpdjere requested review from a team as code owners January 3, 2024 16:39

jpdjere requested a review from banderror January 3, 2024 16:39

jpdjere self-assigned this Jan 3, 2024

jpdjere added v8.12.0 v8.12.1 labels Jan 3, 2024

Merge branch 'main' into fix-flake-171428

1088c87

banderror reviewed Jan 5, 2024

View reviewed changes

Rewrote retry function

0ebcb25

jpdjere added 2 commits January 9, 2024 00:00

Merge branch 'main' into fix-flake-171428

433b9a2

Additional changes

c5e1050

jpdjere requested a review from banderror January 9, 2024 11:15

banderror reviewed Jan 9, 2024

View reviewed changes

...ion_api_integration/test_suites/detections_response/utils/retry_delete_by_query_conflicts.ts Outdated Show resolved Hide resolved

banderror approved these changes Jan 9, 2024

View reviewed changes

jpdjere added 2 commits January 10, 2024 11:29

Fix constant names

ebf4bd2

Merge branch 'main' into fix-flake-171428

da3ad61

jpdjere merged commit b8c7306 into elastic:main Jan 11, 2024
20 checks passed

kibanamachine added the v8.13.0 label Jan 11, 2024

jpdjere mentioned this pull request Jan 12, 2024

[8.12] [Security Solution] Add retryIfConflict util for 409 conflicts in Integration tests (#174185) #174762

Merged

kibanamachine mentioned this pull request Jan 12, 2024

[Detections Response][FTR] FTR migration cleanup #173403

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 12, 2024

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 15, 2024

mistic removed the v8.12.0 label Jan 17, 2024

ElenaStoeva mentioned this pull request Aug 2, 2024

[Advanced Settings] Add retry for 409 conflicts in API integration tests #189813

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Add `retryIfConflict` util for `409` conflicts in Integration tests #174185

[Security Solution] Add `retryIfConflict` util for `409` conflicts in Integration tests #174185

jpdjere commented Jan 3, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Jan 3, 2024

elasticmachine commented Jan 3, 2024

elasticmachine commented Jan 3, 2024

banderror left a comment

jpdjere commented Jan 8, 2024 •

edited

Loading

banderror Jan 9, 2024

banderror left a comment

kibana-ci commented Jan 11, 2024

kibanamachine commented Jan 11, 2024

jpdjere commented Jan 12, 2024

kibanamachine commented Jan 12, 2024

mistic commented Jan 17, 2024

[Security Solution] Add retryIfConflict util for 409 conflicts in Integration tests #174185

[Security Solution] Add retryIfConflict util for 409 conflicts in Integration tests #174185

Conversation

jpdjere commented Jan 3, 2024 • edited by kibanamachine Loading

Summary

Flaky test run

For maintainers

elasticmachine commented Jan 3, 2024

elasticmachine commented Jan 3, 2024

elasticmachine commented Jan 3, 2024

banderror left a comment

Choose a reason for hiding this comment

jpdjere commented Jan 8, 2024 • edited Loading

banderror Jan 9, 2024

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 11, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

History

kibanamachine commented Jan 11, 2024

💔 All backports failed

Manual backport

Questions ?

jpdjere commented Jan 12, 2024

💚 All backports created successfully

Questions ?

kibanamachine commented Jan 12, 2024

mistic commented Jan 17, 2024

[Security Solution] Add `retryIfConflict` util for `409` conflicts in Integration tests #174185

[Security Solution] Add `retryIfConflict` util for `409` conflicts in Integration tests #174185

jpdjere commented Jan 3, 2024 •

edited by kibanamachine

Loading

jpdjere commented Jan 8, 2024 •

edited

Loading