-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Fleet] Avoid loading package saved objects into memory before deleting them #188004
Conversation
70f2487
to
7b015ab
Compare
7b015ab
to
d70cfd6
Compare
Pinging @elastic/fleet (Team:Fleet) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've refactored getKibanaAssets
a little bit to avoid intermediate array creation and reduce memory consumption when installing big packages.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
cc @xcrzx |
@@ -237,9 +266,9 @@ async function deleteAssets( | |||
// then the other asset types | |||
await Promise.all([ | |||
...deleteESAssets(otherAssets, esClient), | |||
deleteKibanaAssets(installedKibana, spaceId), | |||
resolveAndDeleteKibanaAssets(installedKibana, spaceId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason the resolve is needed here, don't we need the version check here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we can add the check here too. Where is this code path used? I'm not super familiar with this code, so I'd rely on your opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is being called when deleting a package or when rolling back a package installation. I think it would be good to add the same version check here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll be introducing another optimization in the remove method soon. I'll address your suggestion in that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, asked a clarifying question.
// only in 8.x or later. If so, we can skip SO resolution step altogether | ||
// and delete the assets directly. Otherwise, we need to resolve the assets | ||
// which might create high memory pressure if a package has a lot of assets. | ||
if (minKibana && minKibana.major >= 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not even sure we need that condition, we have a constant that we bump to force reinstall package when upgrading Kibana, and I am pretty sure this one as been bumped in the 8.x cycle. The condition is the package has been installed in 8.x correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we need this check to account for cases when users upgrade directly from 7.17
to 8.current
. Our official documentation suggests upgrading to 8.0
first, but I'm not sure if that is enforced in any way. If there's a possibility our users can upgrade to 8.current
directly without installing 8.0
first, I think we should account for that.
Resolves: #187975
Summary
When upgrading or re-installing a package, all saved objects from a previous package are loaded into memory using
bulkResolve
. This creates unnecessary memory pressure for packages containing thousands of saved objects, like thesecurity_detection_engine
package.To mitigate that, we are now skipping saved object resolution for packages known to be installed in
8.x
.While testing locally on a package containing ~5000 detection rules, I observed a significant drop in memory usage, from 1.17GB to 1.05GB at peak.
Before:
After: