[Fleet] Cache call to getBundledPackages #172640

nchaulet · 2023-12-05T21:45:59Z

Summary

getBundledPackages read all the bundled package from disk every time it's called, if that function is called concurrently it will lead to OOM errors, as reading all packages is quite memory consuming.

That draft PR propose a simple in memory cache implementation, and open the discussion on how we can fix/improve that.

I tested this locally with a --max_old_space_size=717 and there is no OOM memory anymore. @kpollich if it's the way we want to go I can fix the remaining tests

I also changed the interface of BundledPackage to lazily get the buffer only when we need.

apmmachine · 2023-12-05T21:46:15Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

kpollich · 2023-12-06T14:18:33Z

I think this is a good path forward, and I'd be happy to see the remaining tests fixed. Do you have any concerns with this approach based on what we've been discussing in Slack?

nchaulet · 2023-12-06T15:29:51Z

I think this is a good path forward, and I'd be happy to see the remaining tests fixed. Do you have any concerns with this approach based on what we've been discussing in Slack?

This will add a little memory overhead, but for bundled packages, I think we maybe able to do something a little smarter by changing the BundledPackage to something like this

interface BundledPackage
{
  pkgName: string,
  pkgVersion: string,
  buffer: async () => Buffer // Change here to read buffer only when we need it,
}

What do you think? the only part that will be store in memory will the pkgName, version and the buffer will only be retrieved when needed

kpollich · 2023-12-06T15:51:02Z

Yeah that change to bundled package objects makes sense to me.

…undled-packags

elasticmachine · 2023-12-06T19:30:25Z

Pinging @elastic/fleet (Team:Fleet)

nchaulet · 2023-12-06T19:47:22Z

@elasticmachine merge upstream

elasticmachine · 2023-12-06T19:47:33Z

Pinging @elastic/apm-ui (Team:APM)

kpollich

LGTM 🚀

nchaulet · 2023-12-07T14:26:06Z

@kpollich do you think we should backport this one? I think we should

kpollich · 2023-12-07T14:57:14Z

Yes let's backport this - it will fix a substantial memory issue with Fleet so worth the backport.

nchaulet · 2023-12-07T15:11:49Z

@elasticmachine merge upstream

elasticmachine · 2023-12-07T20:32:33Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

dgieselaar · 2023-12-07T20:49:34Z

x-pack/plugins/fleet/server/services/epm/packages/bundled_packages.ts

        const { pkgName, pkgVersion } = splitPkgKey(zipFile.replace(/\.zip$/, ''));

+        const getBuffer = () => fs.readFile(path.join(bundledPackageLocation, zipFile));


drive-by comment: maybe this could be wrapped in once()? Behaviour should be similar as before (this change), but no risk in reading from disk multiple times if the function is called multiple times for whatever reason as well.

@dgieselaar with the once this will cache the file in memory forever right? it's probably something we want to avoid, we are trying to reduce the fleet memory footprint in kibana too.

what we try to achieve with that PR is to avoid to load all the bundled package in memory when we call getBundledPackage (that is called in a lot of place in fleet code)

with the once this will cache the file in memory forever right?
No, only in the scope of this call to getBundledPackages. Which would be the same as before. Wrapping this in once simply means that if downstream getBuffer is called twice, it is only read from disk once. Previously it was also read from disk once - this change introduces the possibility that it is read from disk multiple times for each getBundledPackages call.

@dgieselaar that PR changed the getBundledPackages to cache the results, so those bundled package object we are returning are now global, I made a change in d2c575e to make a copy for each getBundledPackages and use a once there does it make sense to you?

ah, yes, makes sense!

nchaulet · 2023-12-08T13:56:27Z

That PR seems to drastically help with an issue I currently have with bumping the revision of a large number of agent policies

Trigger a policy change with 500 agent policies:
Before that change (look at the external memory(buffer))

With that change

cauemarcondes

LGTM

…undled-packags

kibana-ci · 2023-12-08T17:06:07Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: fcc392f

Failed CI Steps

Detection Engine - Security Solution Cypress Tests #6

Test Failures

[job] [logs] Detection Engine - Security Solution Cypress Tests #6 / Alert user assignment - ESS & Serverless Basic rendering alert with some assignees in alerts table alert with some assignees in alerts table

Metrics [docs]

✅ unchanged

History

💛 Build #182517 was flaky 50e77d2
💛 Build #182271 was flaky aeceed3
💛 Build #182244 was flaky 1713909
💔 Build #182214 failed 3b1f209
💔 Build #182202 failed a56663a
💔 Build #182018 failed 50e617d

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

cc @nchaulet

(cherry picked from commit 2c0e988)

kibanamachine · 2023-12-08T17:11:51Z

💚 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

# Backport This will backport the following commits from `main` to `8.12`: - [[Fleet] Cache call to getBundledPackages (#172640)](#172640)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nicolas Chaulet <[email protected]>

nchaulet added bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team labels Dec 5, 2023

nchaulet self-assigned this Dec 5, 2023

[Fleet] Cache call to getBundledPackages

50e617d

nchaulet force-pushed the feature-cache-bundled-packags branch from d0e950a to 50e617d Compare December 5, 2023 21:47

nchaulet added 4 commits December 6, 2023 11:28

Merge branch 'main' of github.com:elastic/kibana into feature-cache-b…

c046b89

…undled-packags

lazy ready buffer

a56663a

fix type

3b1f209

Fix tests

1713909

nchaulet marked this pull request as ready for review December 6, 2023 19:30

nchaulet requested review from a team as code owners December 6, 2023 19:30

Merge branch 'main' into feature-cache-bundled-packags

aeceed3

botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 6, 2023

nchaulet added release_note:skip Skip the PR/issue when compiling release notes and removed Team:APM All issues that need APM UI Team support labels Dec 6, 2023

kpollich approved these changes Dec 7, 2023

View reviewed changes

botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 7, 2023

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

Merge branch 'main' into feature-cache-bundled-packags

50e77d2

nchaulet requested a review from cauemarcondes December 7, 2023 18:03

smith added Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team and removed Team:APM All issues that need APM UI Team support labels Dec 7, 2023

smith added the apm:review label Dec 7, 2023

dgieselaar reviewed Dec 7, 2023

View reviewed changes

botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 7, 2023

nchaulet requested a review from dgieselaar December 8, 2023 00:02

cauemarcondes approved these changes Dec 8, 2023

View reviewed changes

nchaulet added 3 commits December 8, 2023 10:23

Use once for getBuffer

d2c575e

fix type

ab3f3ea

Merge branch 'main' of github.com:elastic/kibana into feature-cache-b…

fcc392f

…undled-packags

nchaulet enabled auto-merge (squash) December 8, 2023 16:07

nchaulet merged commit 2c0e988 into elastic:main Dec 8, 2023
40 checks passed

kibanamachine added the v8.13.0 label Dec 8, 2023

nchaulet deleted the feature-cache-bundled-packags branch December 8, 2023 17:11

kibanamachine mentioned this pull request Dec 8, 2023

[8.12] [Fleet] Cache call to getBundledPackages (#172640) #172978

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 8, 2023

[Fleet] Cache call to getBundledPackages (elastic#172640)

a177d6e

(cherry picked from commit 2c0e988)

kibanamachine added the v8.12.0 label Dec 8, 2023

nchaulet mentioned this pull request Dec 11, 2023

[Fleet] Fix get file handler for bundled package (#172182) #173058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] Cache call to getBundledPackages #172640

[Fleet] Cache call to getBundledPackages #172640

nchaulet commented Dec 5, 2023 •

edited by kibanamachine

Loading

apmmachine commented Dec 5, 2023

kpollich commented Dec 6, 2023

nchaulet commented Dec 6, 2023 •

edited

Loading

kpollich commented Dec 6, 2023

elasticmachine commented Dec 6, 2023

nchaulet commented Dec 6, 2023

elasticmachine commented Dec 6, 2023

kpollich left a comment

nchaulet commented Dec 7, 2023

kpollich commented Dec 7, 2023

nchaulet commented Dec 7, 2023

elasticmachine commented Dec 7, 2023

dgieselaar Dec 7, 2023

nchaulet Dec 8, 2023

nchaulet Dec 8, 2023

dgieselaar Dec 8, 2023

nchaulet Dec 8, 2023

dgieselaar Dec 8, 2023

nchaulet commented Dec 8, 2023 •

edited

Loading

cauemarcondes left a comment

kibana-ci commented Dec 8, 2023

kibanamachine commented Dec 8, 2023

		const { pkgName, pkgVersion } = splitPkgKey(zipFile.replace(/\.zip$/, ''));

		const getBuffer = () => fs.readFile(path.join(bundledPackageLocation, zipFile));

[Fleet] Cache call to getBundledPackages #172640

[Fleet] Cache call to getBundledPackages #172640

Conversation

nchaulet commented Dec 5, 2023 • edited by kibanamachine Loading

Summary

apmmachine commented Dec 5, 2023

🤖 GitHub comments

kpollich commented Dec 6, 2023

nchaulet commented Dec 6, 2023 • edited Loading

kpollich commented Dec 6, 2023

elasticmachine commented Dec 6, 2023

nchaulet commented Dec 6, 2023

elasticmachine commented Dec 6, 2023

kpollich left a comment

Choose a reason for hiding this comment

nchaulet commented Dec 7, 2023

kpollich commented Dec 7, 2023

nchaulet commented Dec 7, 2023

elasticmachine commented Dec 7, 2023

dgieselaar Dec 7, 2023

Choose a reason for hiding this comment

nchaulet Dec 8, 2023

Choose a reason for hiding this comment

nchaulet Dec 8, 2023

Choose a reason for hiding this comment

dgieselaar Dec 8, 2023

Choose a reason for hiding this comment

nchaulet Dec 8, 2023

Choose a reason for hiding this comment

dgieselaar Dec 8, 2023

Choose a reason for hiding this comment

nchaulet commented Dec 8, 2023 • edited Loading

cauemarcondes left a comment

Choose a reason for hiding this comment

kibana-ci commented Dec 8, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

kibanamachine commented Dec 8, 2023

💚 All backports created successfully

Questions ?

nchaulet commented Dec 5, 2023 •

edited by kibanamachine

Loading

nchaulet commented Dec 6, 2023 •

edited

Loading

nchaulet commented Dec 8, 2023 •

edited

Loading