[Fleet] Upgrade managed package policies in a background task by nchaulet · Pull Request #191097 · elastic/kibana

nchaulet · 2024-08-22T14:55:55Z

Summary

Upgrading managed package policies is taking a long time, even when trying to optimize the code. To avoid setup that are too long that PR move the process of upgrading managed package policies from the fleet setup to a background task triggered by the setup.

In addition to this I still implemented a few optimization to make that code path a little quicker:

Implemented a cache on getPackageInfo and getAssetsMap the cache is scopped to the task with small size so it should not impact to much Kibana memory

I capture a few APM trace of that background task with 100 synthetics package policies, and it went from ~5 minute to do the upgrade to ~1 minute

Todo

Write some integration tests for the upgradeManagedPackagePolicies
Unit test cache

obltmachine · 2024-08-22T14:56:10Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

nchaulet · 2024-08-22T17:34:05Z

/ci

nchaulet · 2024-08-22T19:39:12Z

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

Should we make those key configurable through kibana config under fleet.internal.*

It could be useful, but is there a risk that the user adds a number too small or too big and creates some issue? If we decide to do it we should document it very clearly.

yes I think if we make this configurable it will be under an internal keyword probably that could be useful for supportability (SDH with a scenario we did not think of)

We can start without making this configurable, those size are pretty small it should not have a huge impact

nchaulet · 2024-08-22T19:42:51Z

x-pack/plugins/fleet/server/services/package_policy.ts

There is no reason for this to change during an upgrade and that codepath was extremely slow

Do we have tests that cover this removal? Just making sure that we don't inadvertently break anything.

We do not, but it seems that codepath was doing nothing, it seems it was only here for supporting an UI that do not exists anymore #190613

Thanks for checking!

nchaulet · 2024-08-22T19:45:38Z

/ci

nchaulet · 2024-08-23T13:39:05Z

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

  ignoreUnverified?: boolean;
  prerelease?: boolean;
 }): Promise<PackageInfo> {
+  const cacheResult = getPackageInfoCache(pkgName, pkgVersion);


Curious to get your though on that pattern for caching, it seems easier to use that passing packageInfo through all codebase

I agree, currently we have to pass the packageInfo object around and is not the best pattern. With this we could probably replace it.

elasticmachine · 2024-08-23T13:39:47Z

Pinging @elastic/fleet (Team:Fleet)

…-managed-pacakge-policies-task

nchaulet · 2024-08-26T12:48:05Z

@elasticmachine merge upstream

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

criamico · 2024-08-26T15:03:03Z

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

It could be useful, but is there a risk that the user adds a number too small or too big and creates some issue? If we decide to do it we should document it very clearly.

criamico · 2024-08-26T15:04:17Z

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

  ignoreUnverified?: boolean;
  prerelease?: boolean;
 }): Promise<PackageInfo> {
+  const cacheResult = getPackageInfoCache(pkgName, pkgVersion);


I agree, currently we have to pass the packageInfo object around and is not the best pattern. With this we could probably replace it.

criamico · 2024-08-26T15:05:40Z

x-pack/plugins/fleet/server/services/package_policy.ts

Do we have tests that cover this removal? Just making sure that we don't inadvertently break anything.

x-pack/plugins/fleet/server/services/setup/managed_package_policies.ts

nchaulet · 2024-08-28T13:11:59Z

@elasticmachine merge upstream

criamico

LGTM 🚢

x-pack/test/fleet_api_integration/apis/fleet_setup.ts

kibana-ci · 2024-08-28T18:09:03Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: e5796a9

Failed CI Steps

FTR Configs #27

Test Failures

[job] [logs] FTR Configs #27 / Cloud Security Posture Test adding Cloud Security Posture Integrations CNVM CNVM AWS Hyperlink on PostInstallation Modal should have the correct URL

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`fleet`	13	12	-1

Total ESLint disabled count

id	before	after	diff
`fleet`	57	56	-1

History

💛 Build #230431 was flaky 5fc1c2b
💚 Build #229848 succeeded 852c171
💚 Build #229762 succeeded 0fc55f7
💛 Build #229540 was flaky e8da1e4
💔 Build #229497 failed 2f23da6
💔 Build #229469 failed 3fb0b29

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

cc @nchaulet

ymao1

response ops changes LGTM.

[Fleet] Upgrade managed package policies in a background task

e96e134

fix tests

bbac081

nchaulet commented Aug 22, 2024

View reviewed changes

add cache

4ef0d1b

nchaulet force-pushed the feature-upgrade-managed-pacakge-policies-task branch from effdca7 to 4ef0d1b Compare August 22, 2024 19:45

Add tests for managed package policies upgrade

3fb0b29

nchaulet commented Aug 23, 2024

View reviewed changes

nchaulet added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team labels Aug 23, 2024

nchaulet self-assigned this Aug 23, 2024

nchaulet marked this pull request as ready for review August 23, 2024 13:39

nchaulet requested review from a team as code owners August 23, 2024 13:39

nchaulet added 3 commits August 23, 2024 10:57

fix test

466e2d3

unit test cache

2f23da6

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

e8da1e4

…-managed-pacakge-policies-task

Merge branch 'main' into feature-upgrade-managed-pacakge-policies-task

0fc55f7

criamico reviewed Aug 26, 2024

View reviewed changes

fix after review

852c171

nchaulet requested review from a team and criamico August 26, 2024 17:47

Merge branch 'main' into feature-upgrade-managed-pacakge-policies-task

5fc1c2b

criamico approved these changes Aug 28, 2024

View reviewed changes

x-pack/test/fleet_api_integration/apis/fleet_setup.ts Outdated Show resolved Hide resolved

Update fleet_setup.ts

e5796a9

ymao1 approved these changes Aug 29, 2024

View reviewed changes

nchaulet merged commit fe0d310 into elastic:main Aug 29, 2024

nchaulet deleted the feature-upgrade-managed-pacakge-policies-task branch August 29, 2024 12:09

kibanamachine added v8.16.0 backport:skip This PR does not require backporting labels Aug 29, 2024

Conversation

nchaulet commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Todo

Uh oh!

obltmachine commented Aug 22, 2024

🤖 GitHub comments

Uh oh!

nchaulet commented Aug 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchaulet commented Aug 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 23, 2024

Uh oh!

nchaulet commented Aug 26, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nchaulet commented Aug 28, 2024

Uh oh!

criamico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibana-ci commented Aug 28, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

ESLint disabled in files

Total ESLint disabled count

History

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nchaulet commented Aug 22, 2024 •

edited

Loading