Fixing hanging tasks for correlations by goyamegh · Pull Request #874 · opensearch-project/security-analytics

goyamegh · 2024-03-01T06:44:06Z

Description

While dealing with correlations, we noticed unexpectedly high resource consumption in CPU and JVM, resulting in node drops. Upon closer examination, we found a subset of subscribe/findings tasks consistently in a hanging state throughout the data node's lifecycle until it either dropped or required manual restart. This information can be corroborated through the _cat/tasks API.

The root cause of these hanging tasks was the way action listeners handle exceptions today, causing the parent listener to not close in some cases, if the exceptions are not caught correctly.

In this specific case, the exception causing the contention of resources was:
java.lang.ArrayIndexOutOfBoundsException: Index 0 out of bounds for length 0 at org.opensearch.securityanalytics.correlation.VectorEmbeddingsEngine$1$1.onResponse(VectorEmbeddingsEngine.java:142) at org.opensearch.securityanalytics.correlation.VectorEmbeddingsEngine$1$1.onResponse(VectorEmbeddingsEngine.java:115) at org.opensearch.action.support.TransportAction$1.onResponse(TransportAction.java:113) at org.opensearch.action.support.TransportAction$1.onResponse(TransportAction.java:107) at org.opensearch.action.search.TransportMultiSearchAction$1.finish(TransportMultiSearchAction.java:209) at org.opensearch.action.search.TransportMultiSearchAction$1.handleResponse(TransportMultiSearchAction.java:195) at org.opensearch.action.search.TransportMultiSearchAction$1.onResponse(TransportMultiSearchAction.java:183) at org.opensearch.action.search.TransportMultiSearchAction$1.onResponse(TransportMultiSearchAction.java:180) at org.opensearch.action.support.TransportAction$1.onResponse(TransportAction.java:113) at org.opensearch.action.support.TransportAction$1.onResponse(TransportAction.java:107) at org.opensearch.performanceanalyzer.action.PerformanceAnalyzerActionListener.onResponse(PerformanceAnalyzerActionListener.java:55) at org.opensearch.action.support.TimeoutTaskCancellationUtility$TimeoutRunnableListener.onResponse(TimeoutTaskCancellationUtility.java:132)

This will PR will be followed up by two more solutions:

Fix the working of child action listeners across Security Analytics to handle the closing of parent listener tasks correctly
Analyze and see how the methods 'getHits().lengthand getTotalHits().value` are different and if we need to modify the way the search requests have been created in order to restore this usage in future.

Issues Resolved

This PR aims to fix the current exception being thrown by using getHits().length instead of getTotalHits().value as a mitigation to avoid any tasks to hang.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Megha Goyal <goyamegh@amazon.com>

codecov · 2024-03-01T09:01:55Z

Codecov Report

Attention: Patch coverage is 0% with 646 lines in your changes are missing coverage. Please review.

Project coverage is 25.07%. Comparing base (8ef0a3f) to head (f653429).
Report is 1 commits behind head on main.

❗ Current head f653429 differs from pull request most recent head 98fbc25. Consider uploading reports for the commit 98fbc25 to get more accurate results

Files	Patch %	Lines
...yanalytics/correlation/VectorEmbeddingsEngine.java	0.00%	280 Missing ⚠️
...arch/securityanalytics/correlation/JoinEngine.java	0.00%	191 Missing ⚠️
...ics/transport/TransportCorrelateFindingAction.java	0.00%	175 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #874      +/-   ##
============================================
+ Coverage     24.82%   25.07%   +0.24%     
+ Complexity     1030     1029       -1     
============================================
  Files           277      277              
  Lines         12717    12572     -145     
  Branches       1401     1372      -29     
============================================
- Hits           3157     3152       -5     
+ Misses         9296     9156     -140     
  Partials        264      264

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

engechas · 2024-03-01T17:12:24Z

src/main/java/org/opensearch/securityanalytics/correlation/VectorEmbeddingsEngine.java

-                            long totalHits = response.getResponse().getHits().getTotalHits().value;
+                            long totalHits = response.getResponse().getHits().getHits().length;


What's the rationale to swap from the total hits value to the hits array length?

We are seeing inconsistent results from search response where the getTotalHits().value is non zero, but the actual response array has no objects. The idea is to use the same array for both, validation and then fetching data.

eirsep · 2024-03-04T18:11:19Z

src/main/java/org/opensearch/securityanalytics/transport/TransportCorrelateFindingAction.java

                client.search(searchRequest, new ActionListener<>() {
                    @Override
                    public void onResponse(SearchResponse response) {
+                        if (response.getHits().getHits().length == 0) {


is not getting any hits really an exception? i thought if totalHits>0 && response.getHits().getHits().length == 0 only then its an exceptional case

The search request expects the result to have one doc (size is set to 1), we can anyway get only two responses from the hits array length (0 or 1). Total hits isn't a required check for this invocation.

eirsep · 2024-03-05T18:36:53Z

src/main/java/org/opensearch/securityanalytics/transport/TransportCorrelateFindingAction.java

        }

        public void onFailures(Exception t) {
+            log.error("Exception occurred while processing correlations", t);


redundant log line. the correct message should be logged at caller.

This line will be the only place we log errors before throwing the exception out to Alerting, I made sure of this while testing for most happy cases, will follow up and remove them in future PRs.

Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit db025ce)

Signed-off-by: Megha Goyal <goyamegh@amazon.com>

opensearch-trigger-bot · 2024-03-11T22:55:09Z

The backport to 2.9 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.9 2.9
# Navigate to the new working tree
cd .worktrees/backport-2.9
# Create a new branch
git switch --create backport/backport-874-to-2.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 db025ce69c0201798b9e862a9156a658d2f2d241
# Push it to GitHub
git push --set-upstream origin backport/backport-874-to-2.9
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.9

Then, create a pull request where the base branch is 2.9 and the compare/head branch is backport/backport-874-to-2.9.

opensearch-trigger-bot · 2024-03-11T22:55:10Z

The backport to 2.7 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.7 2.7
# Navigate to the new working tree
cd .worktrees/backport-2.7
# Create a new branch
git switch --create backport/backport-874-to-2.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 db025ce69c0201798b9e862a9156a658d2f2d241
# Push it to GitHub
git push --set-upstream origin backport/backport-874-to-2.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.7

Then, create a pull request where the base branch is 2.7 and the compare/head branch is backport/backport-874-to-2.7.

Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit db025ce) Co-authored-by: Megha Goyal <56077967+goyamegh@users.noreply.github.com>

Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit db025ce) Co-authored-by: Megha Goyal <56077967+goyamegh@users.noreply.github.com> (cherry picked from commit 73de979)

Fixing hanging tasks for correlations

98fbc25

Signed-off-by: Megha Goyal <goyamegh@amazon.com>

goyamegh force-pushed the main-correlations branch from 0ee0288 to 98fbc25 Compare March 1, 2024 08:46

goyamegh marked this pull request as ready for review March 1, 2024 15:09

goyamegh requested review from AWSHurneyt, amsiglan, eirsep, engechas, getsaurabh02, jowg-amazon, lezzago, praveensameneni, riysaxen-amzn and sbcd90 as code owners March 1, 2024 15:09

engechas previously approved these changes Mar 1, 2024

View reviewed changes

goyamegh dismissed engechas’s stale review via f653429 March 3, 2024 00:28

goyamegh force-pushed the main-correlations branch from f653429 to 98fbc25 Compare March 3, 2024 00:32

goyamegh mentioned this pull request Mar 4, 2024

[FEATURE] Enhance System Performance to address Hanging Tasks by Optimizing Correlations #879

Closed

eirsep reviewed Mar 4, 2024

View reviewed changes

eirsep approved these changes Mar 5, 2024

View reviewed changes

eirsep reviewed Mar 5, 2024

View reviewed changes

engechas approved these changes Mar 5, 2024

View reviewed changes

goyamegh merged commit db025ce into opensearch-project:main Mar 5, 2024

goyamegh added backport 2.x backport 2.11 labels Mar 7, 2024

opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 7, 2024

Fixing hanging tasks for correlations (#874)

553244c

Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit db025ce)

opensearch-trigger-bot bot mentioned this pull request Mar 7, 2024

[Backport 2.11] Fixing hanging tasks for correlations #891

Merged

opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 7, 2024

Fixing hanging tasks for correlations (#874)

085c887

Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit db025ce)

opensearch-trigger-bot bot mentioned this pull request Mar 7, 2024

[Backport 2.x] Fixing hanging tasks for correlations #892

Closed

eirsep pushed a commit to eirsep/security-analytics that referenced this pull request Mar 7, 2024

Fixing hanging tasks for correlations (opensearch-project#874)

f44e6ef

Signed-off-by: Megha Goyal <goyamegh@amazon.com>

eirsep mentioned this pull request Mar 8, 2024

Backport #874, #860, #873 to 2.x #894

Closed

eirsep pushed a commit to eirsep/security-analytics that referenced this pull request Mar 8, 2024

Fixing hanging tasks for correlations (opensearch-project#874)

edb33f7

Signed-off-by: Megha Goyal <goyamegh@amazon.com>

goyamegh added backport 2.7 backport 2.9 backports to 2.9 labels Mar 11, 2024

goyamegh added a commit that referenced this pull request Mar 12, 2024

Fixing hanging tasks for correlations (#874) (#891)

73de979

Signed-off-by: Megha Goyal <goyamegh@amazon.com> (cherry picked from commit db025ce) Co-authored-by: Megha Goyal <56077967+goyamegh@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing hanging tasks for correlations#874

Fixing hanging tasks for correlations#874
goyamegh merged 1 commit intoopensearch-project:mainfrom
goyamegh:main-correlations

goyamegh commented Mar 1, 2024 •

edited

Loading

Uh oh!

codecov bot commented Mar 1, 2024 •

edited

Loading

Uh oh!

engechas Mar 1, 2024

Uh oh!

goyamegh Mar 3, 2024

Uh oh!

eirsep Mar 4, 2024

Uh oh!

goyamegh Mar 5, 2024

Uh oh!

eirsep Mar 5, 2024

Uh oh!

goyamegh Mar 5, 2024

Uh oh!

opensearch-trigger-bot bot commented Mar 11, 2024

Uh oh!

opensearch-trigger-bot bot commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		long totalHits = response.getResponse().getHits().getTotalHits().value;
		long totalHits = response.getResponse().getHits().getHits().length;

Conversation

goyamegh commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

codecov bot commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

engechas Mar 1, 2024

Choose a reason for hiding this comment

Uh oh!

goyamegh Mar 3, 2024

Choose a reason for hiding this comment

Uh oh!

eirsep Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

goyamegh Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

eirsep Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

goyamegh Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

opensearch-trigger-bot bot commented Mar 11, 2024

Uh oh!

opensearch-trigger-bot bot commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

goyamegh commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading