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 Solutions] Fixes jest test memory leaks within the server side detection engine #117964

Closed

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Nov 9, 2021

Summary

Fixes the jest memory leaks within (in a workaround way) within:

x-pack/plugins/security_solution/server/lib/detection_engine

See these comments:
#117255 (comment)
#117255 (comment)

On how those two issues were worked around.

Other things is I did a import type everywhere I could within the detection_engine to reduce chances of other memory leaks from imports.

How to test this memory leak has been fixed?

You can run as low as 600 megs for space size here. If you try to use a different branch such as main, it will not complete in under 600 megs and will blow up.

node --max-old-space-size=600 --expose-gc ./node_modules/.bin/jest --runInBand --logHeapUsage --detectOpenHandles --no-cache --config x-pack/plugins/security_solution/jest.config.js x-pack/plugins/security_solution/server/lib/detection_engine

Checklist

import {

// We _must_ import from here and not from "alerting/server" above or we will get a memory leak. See https://github.com/elastic/kibana/issues/117255
import { parseDuration } from '../../../../../alerting/common/parse_duration';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: Here is one key area where I had to import as a workaround for the memory leaks found by importing at a higher level within alerting.

@FrankHassanabad FrankHassanabad self-assigned this Nov 9, 2021
@FrankHassanabad FrankHassanabad added v8.0.0 v8.1.0 release_note:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform Security Solution Platform Team auto-backport Deprecated - use backport:version if exact versions are needed labels Nov 9, 2021

// We have to disable this path otherwise we leak memory and cannot directly import from "../../../../../../actions/server/"
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { actionsClientMock } from '../../../../../../actions/server/actions_client.mock';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: I have to use this and cannot use import { actionsClientMock } from '../../../../../../actions/server/mocks'; without having a memory leak within Jest.

Copy link
Member

Choose a reason for hiding this comment

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

Is there anyway we can add a linter rule to this and the above infringing mocks to prevent folks from importing them this way in the future (or warn at least?), or best just to prioritize identifying and fixing the underlying issues? Just curious what we can do to prevent these memory leaks from slowly sneaking their way back in... 🐌💧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking that maybe this isn't the right way for us to go here. I am thinking of putting this back into draft mode and talking to more operations people. The module loader in some cases looks to be re-loading or leaking the modules and handling this at an infrastructure level might be better for us.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

cc @FrankHassanabad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform Security Solution Platform Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants