-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Fleet] Upgrade managed package policies in a background task #191097
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
Changes from all commits
e96e134
bbac081
4ef0d1b
3fb0b29
466e2d3
2f23da6
e8da1e4
0fc55f7
852c171
5fc1c2b
e5796a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { | ||
| getPackageAssetsMapCache, | ||
| getPackageInfoCache, | ||
| runWithCache, | ||
| setPackageAssetsMapCache, | ||
| setPackageInfoCache, | ||
| } from './cache'; | ||
|
|
||
| const PKG_NAME = 'test'; | ||
| const PKG_VERSION = '1.0.0'; | ||
|
|
||
| describe('EPM CacheSession', () => { | ||
| describe('outside of a cache session', () => { | ||
| it('should not cache package info', () => { | ||
| setPackageInfoCache(PKG_NAME, PKG_VERSION, { | ||
| name: 'test', | ||
| } as any); | ||
| const cache = getPackageInfoCache(PKG_NAME, PKG_VERSION); | ||
| expect(cache).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should not cache assetsMap', () => { | ||
| setPackageAssetsMapCache(PKG_NAME, PKG_VERSION, new Map()); | ||
| const cache = getPackageAssetsMapCache(PKG_NAME, PKG_VERSION); | ||
| expect(cache).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('in of a cache session', () => { | ||
| it('should cache package info', async () => { | ||
| function setCache() { | ||
| setPackageInfoCache(PKG_NAME, PKG_VERSION, { | ||
| name: 'test', | ||
| } as any); | ||
| } | ||
| function getCache() { | ||
| const cache = getPackageInfoCache(PKG_NAME, PKG_VERSION); | ||
| expect(cache).toEqual({ name: 'test' }); | ||
| } | ||
|
|
||
| await runWithCache(async () => { | ||
| setCache(); | ||
| getCache(); | ||
| }); | ||
| }); | ||
|
|
||
| it('should cache assetsMap', async () => { | ||
| function setCache() { | ||
| const map = new Map(); | ||
| map.set('test.yaml', Buffer.from('name: test')); | ||
| setPackageAssetsMapCache(PKG_NAME, PKG_VERSION, map); | ||
| } | ||
| function getCache() { | ||
| const cache = getPackageAssetsMapCache(PKG_NAME, PKG_VERSION); | ||
| expect(cache).not.toBeUndefined(); | ||
| expect(cache?.get('test.yaml')?.toString()).toEqual('name: test'); | ||
| } | ||
|
|
||
| await runWithCache(async () => { | ||
| setCache(); | ||
| getCache(); | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { AsyncLocalStorage } from 'async_hooks'; | ||
|
|
||
| import LRUCache from 'lru-cache'; | ||
|
|
||
| import type { AssetsMap } from '../../../../common/types'; | ||
|
|
||
| import type { PackageInfo } from '../../../../common'; | ||
|
|
||
| const cacheStore = new AsyncLocalStorage<CacheSession>(); | ||
|
|
||
| const PACKAGE_INFO_CACHE_SIZE = 20; | ||
| const PACKAGE_ASSETS_MAP_CACHE_SIZE = 1; | ||
|
|
||
| class CacheSession { | ||
| private _packageInfoCache?: LRUCache<string, PackageInfo>; | ||
|
|
||
| private _packageAssetsMap?: LRUCache<string, AssetsMap>; | ||
|
|
||
| getPackageInfoCache() { | ||
| if (!this._packageInfoCache) { | ||
| this._packageInfoCache = new LRUCache<string, PackageInfo>({ | ||
| max: PACKAGE_INFO_CACHE_SIZE, | ||
| }); | ||
| } | ||
| return this._packageInfoCache; | ||
| } | ||
|
|
||
| getPackageAssetsMapCache() { | ||
| if (!this._packageAssetsMap) { | ||
| this._packageAssetsMap = new LRUCache<string, AssetsMap>({ | ||
| max: PACKAGE_ASSETS_MAP_CACHE_SIZE, | ||
| }); | ||
| } | ||
| return this._packageAssetsMap; | ||
| } | ||
| } | ||
|
|
||
| export function getPackageInfoCache(pkgName: string, pkgVersion: string) { | ||
| return cacheStore.getStore()?.getPackageInfoCache()?.get(`${pkgName}:${pkgVersion}`); | ||
| } | ||
|
|
||
| export function setPackageInfoCache(pkgName: string, pkgVersion: string, packageInfo: PackageInfo) { | ||
| return cacheStore.getStore()?.getPackageInfoCache()?.set(`${pkgName}:${pkgVersion}`, packageInfo); | ||
| } | ||
|
|
||
| export function getPackageAssetsMapCache(pkgName: string, pkgVersion: string) { | ||
| return cacheStore.getStore()?.getPackageAssetsMapCache()?.get(`${pkgName}:${pkgVersion}`); | ||
| } | ||
|
|
||
| export function setPackageAssetsMapCache( | ||
| pkgName: string, | ||
| pkgVersion: string, | ||
| assetsMap: AssetsMap | ||
| ) { | ||
| return cacheStore | ||
| .getStore() | ||
| ?.getPackageAssetsMapCache() | ||
| ?.set(`${pkgName}:${pkgVersion}`, assetsMap); | ||
| } | ||
|
|
||
| export async function runWithCache<T = any>(cb: () => Promise<T>): Promise<T> { | ||
| const cache = new CacheSession(); | ||
|
|
||
| return cacheStore.run(cache, cb); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,12 @@ import { auditLoggingService } from '../../audit_logging'; | |
| import { getFilteredSearchPackages } from '../filtered_packages'; | ||
|
|
||
| import { createInstallableFrom } from '.'; | ||
| import { | ||
| getPackageAssetsMapCache, | ||
| setPackageAssetsMapCache, | ||
| getPackageInfoCache, | ||
| setPackageInfoCache, | ||
| } from './cache'; | ||
|
|
||
| export { getFile } from '../registry'; | ||
|
|
||
|
|
@@ -415,6 +421,10 @@ export async function getPackageInfo({ | |
| ignoreUnverified?: boolean; | ||
| prerelease?: boolean; | ||
| }): Promise<PackageInfo> { | ||
| const cacheResult = getPackageInfoCache(pkgName, pkgVersion); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious to get your though on that pattern for caching, it seems easier to use that passing packageInfo through all codebase
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, currently we have to pass the |
||
| if (cacheResult) { | ||
| return cacheResult; | ||
| } | ||
| const [savedObject, latestPackage] = await Promise.all([ | ||
| getInstallationObject({ savedObjectsClient, pkgName }), | ||
| Registry.fetchFindLatestPackageOrUndefined(pkgName, { prerelease }), | ||
|
|
@@ -468,7 +478,10 @@ export async function getPackageInfo({ | |
| }; | ||
| const updated = { ...packageInfo, ...additions }; | ||
|
|
||
| return createInstallableFrom(updated, savedObject); | ||
| const installable = createInstallableFrom(updated, savedObject); | ||
| setPackageInfoCache(pkgName, pkgVersion, installable); | ||
|
|
||
| return installable; | ||
| } | ||
|
|
||
| export const getPackageUsageStats = async ({ | ||
|
|
@@ -720,6 +733,10 @@ export async function getPackageAssetsMap({ | |
| logger: Logger; | ||
| ignoreUnverified?: boolean; | ||
| }) { | ||
| const cache = getPackageAssetsMapCache(packageInfo.name, packageInfo.version); | ||
| if (cache) { | ||
| return cache; | ||
| } | ||
| const installedPackageWithAssets = await getInstalledPackageWithAssets({ | ||
| savedObjectsClient, | ||
| pkgName: packageInfo.name, | ||
|
|
@@ -736,6 +753,7 @@ export async function getPackageAssetsMap({ | |
| } else { | ||
| assetsMap = installedPackageWithAssets.assetsMap; | ||
| } | ||
| setPackageAssetsMapCache(packageInfo.name, packageInfo.version, assetsMap); | ||
|
|
||
| return assetsMap; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -118,7 +118,6 @@ import { | |
| mapPackagePolicySavedObjectToPackagePolicy, | ||
| preflightCheckPackagePolicy, | ||
| } from './package_policies'; | ||
| import { updateDatastreamExperimentalFeatures } from './epm/packages/update'; | ||
| import type { | ||
| PackagePolicyClient, | ||
| PackagePolicyClientFetchAllItemsOptions, | ||
|
|
@@ -1633,13 +1632,6 @@ class PackagePolicyClientImpl implements PackagePolicyClient { | |
|
|
||
| await this.update(soClient, esClient, id, updatePackagePolicy, updateOptions); | ||
|
|
||
| // Persist any experimental feature opt-ins that come through the upgrade process to the Installation SO | ||
| await updateDatastreamExperimentalFeatures( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no reason for this to change during an upgrade and that codepath was extremely slow
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have tests that cover this removal? Just making sure that we don't inadvertently break anything.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for checking! |
||
| soClient, | ||
| packagePolicy.package!.name, | ||
| experimentalDataStreamFeatures | ||
| ); | ||
|
|
||
| result.push({ | ||
| id, | ||
| name: packagePolicy.name, | ||
|
|
||
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.
Should we make those key configurable through kibana config under
fleet.internal.*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 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.
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.
yes I think if we make this configurable it will be under an
internalkeyword probably that could be useful for supportability (SDH with a scenario we did not think of)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.
We can start without making this configurable, those size are pretty small it should not have a huge impact