[Fleet] optimizing package policy upgrade and dry run #126088

juliaElastic · 2022-02-21T14:20:33Z

Summary

Optimizing package policy upgrade and dry run based on comments here.

Original suggestion from @joshdover:

This getUpgradeDryRunDiff function may have some opportunities for other improvements. It currently makes these queries:

Within getUpgradePackagePolicyInfo

It fetches the package policy again (we already have it fetched above)

It fetches getInstallation again (we already have it fetched above)

It fetches getPackageInfo - this has many of the same fields that Installation does

It fetches the package from the registry Registry.fetchInfo - the data needed here could be compiled from the package info I believe. Related to [Fleet] Rely on the content of a package when installing it #115032

The actual improvements made:

Within getUpgradeDryRunDiff and upgrade:

Passing optional packagePolicy, so the already fetched object can be reused when called from setup.
Passing optional pkgVersion, so that getInstallation can be skipped as well when called from setup
_compilePackagePolicyInputs and _compilePackageStreams didn't use installablePackage parameter, so removed it

Open questions:

Installation doesn't seem to have the policy_template of the package, so I had to keep getPackageInfo. Am I missing something? EDIT: confirmed that getPackageInfo is still needed for now
It is not clear whether pkgInfo.elasticsearch has the same value as installablePackage.elasticsearch, can someone confirm? EDIT: confirmed that they have the same value by testing

elasticmachine · 2022-02-21T14:20:38Z

Pinging @elastic/fleet (Team:Fleet)

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

joshdover · 2022-02-22T10:16:35Z

Installation doesn't seem to have the policy_template of the package, so I had to keep getPackageInfo. Am I missing something?

You're correct, I don't think the policy_template is available on the Installation object directly. I think it's only available via the manifest.yml file from the epm-package-assets objects for the package, so we'd have to reconstruct it from there. Probably makes sense to hold off on this until we tackle #115032 with across the entire project.

@kpollich is this a scenario where lack of registry connectivity would cause Fleet setup to fail?

kpollich · 2022-02-22T13:49:51Z

Installation doesn't seem to have the policy_template of the package, so I had to keep getPackageInfo. Am I missing something?

You're correct, I don't think the policy_template is available on the Installation object directly. I think it's only available via the manifest.yml file from the epm-package-assets objects for the package, so we'd have to reconstruct it from there. Probably makes sense to hold off on this until we tackle #115032 with across the entire project.

@kpollich is this a scenario where lack of registry connectivity would cause Fleet setup to fail?

We consider failures to upgrade policies non-fatal here:

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

Line 116 in c94d5fd

    
           const nonFatalErrors = [...preconfiguredPackagesNonFatalErrors, ...packagePolicyUpgradeErrors];

So, an error from the registry connection here shouldn't cause the overall setup process to fail.

juliaElastic · 2022-02-22T13:54:44Z

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

+        if (!packagePolicy) {
+          ({ packagePolicy, packageInfo } = await this.getUpgradePackagePolicyInfo(soClient, id));
+        } else if (!packageInfo) {
+          packageInfo = await getPackageInfo({


one thing I noticed that getPackageInfo is called in a for loop for each package policy id, this could be optimized to query once for each package only

getPackageInfo should resolve from Fleet's in-memory cache on subsequent runs, so we might be duplicating memoization efforts here. But, it's worth looking into for sure.

I measured this earlier locally, and it seemed that getPackageInfo takes about 100ms for the first time, and immediate subsequent calls take about 50ms. This still seems much if someone has 10-20 integration policies.
Can you point me to the code where this in-memory caching is done?

EDIT: found the cache here:

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

Lines 40 to 45 in c94d5fd

const packageInfoCache: Map<SharedKeyString, ArchivePackage | RegistryPackage> = new Map();

const sharedKey = ({ name, version }: SharedKey) => `${name}-${version}`;

export const getPackageInfo = (args: SharedKey) => {

return packageInfoCache.get(sharedKey(args));

};

still it looks strange that it takes that much time for the same package
e.g. just with a few integration policies (e.g. apache), toggling between Integration policies and Settings tab on UI triggers a few calls of getPackageInfo:

[2022-02-22T17:05:59.746+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from ES apache_1.3.4 not found in cache, took: 107 ms [2022-02-22T17:05:59.817+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 56 ms [2022-02-22T17:06:02.220+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 481 ms [2022-02-22T17:06:02.308+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 60 ms [2022-02-22T17:06:04.146+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 52 ms [2022-02-22T17:06:04.214+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 58 ms [2022-02-22T17:06:06.078+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 171 ms [2022-02-22T17:06:06.147+01:00][DEBUG][plugins.fleet] retrieved installed package apache-1.3.4 from cache apache_1.3.4 not found in cache, took: 60 ms

getPackageFromSource will resolve the package from cache if possible:

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

Lines 160 to 165 in c94d5fd

const getPackageRes = await getPackageFromSource({

pkgName,

pkgVersion: responsePkgVersion,

savedObjectsClient,

installedPkg: savedObject?.attributes,

});

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

Lines 237 to 247 in c94d5fd

if (installedPkg && installedPkg.version === pkgVersion) {

const { install_source: pkgInstallSource } = installedPkg;

// check cache

res = getArchivePackage({

name: pkgName,

version: pkgVersion,

});

if (res) {

logger.debug(`retrieved installed package ${pkgName}-${pkgVersion} from cache`);

}

But, we don't cache the installation object or latest package record in getPackageInfo, so that's likely where the added time is coming from

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

Lines 147 to 150 in c94d5fd

const [savedObject, latestPackage] = await Promise.all([

getInstallationObject({ savedObjectsClient, pkgName }),

Registry.fetchFindLatestPackageOrUndefined(pkgName),

]);

Yes, it seems like Registry.fetchFindLatestPackageOrUndefined(pkgName) is the culprit, we are calling this each time even when the packageInfo object is cached, to check whether there is a new version of the package in the registry.
I think this is an overkill, perhaps the cache could be made smarter to avoid calling the registry for some time e.g. 1 day, since I think it's a rare event to find new versions published. I could open another issue for this if the team agrees.

Yeah I agree there. I think caching the actual registry responses is a good idea. We do have this issue to track potentially honoring various cache-control headers on responses from EPR, but I don't know that that applies to server-side requests: #125794

I do think determining a caching strategy for our "latest package" requests is out of scope for this PR, though, so we don't need to come up with a solution in order to land this.

Good point, cache-control definitely helps with caching Fleet API calls.
As for server side, I think that depends on node fetch capabilities, which are different than in browser.

kpollich

LGTM - thanks for your work on improving this! 🚀

juliaElastic · 2022-02-23T13:42:56Z

@elasticmachine merge upstream

juliaElastic · 2022-02-23T14:26:08Z

@elasticmachine merge upstream

kibana-ci · 2022-02-23T15:50:44Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`fleet`	1150	1228	+78

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`fleet`	10	9	-1

Unknown metric groups

API count

id	before	after	diff
`fleet`	1266	1344	+78

History

💔 Build #25877 failed f74e211
💚 Build #25783 succeeded b24fa22
💚 Build #25495 succeeded 349af05
💔 Build #25470 failed a1eb081
💔 Build #25333 failed 98dcb58
💔 Build #25314 failed 46671c8

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

cc @juliaElastic

* optimizing package policy upgrade and dry run * optimizing package policy upgrade and dry run * optimizing package policy upgrade and dry run * missing params * fixed tests * removed unused import * fixed tests * fixed checks * reduced duplication, separated validate logic * reduced duplication, separated validate logic Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

jen-huang · 2022-03-16T19:07:48Z

~~[kicking off a backport into 8.1 in an effort to make backporting #127673 cleaner]~~

Nevermind, I misread the conflicts 😅

kibanamachine · 2022-03-16T19:17:56Z

⚪ Backport skipped

The pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually.

Manual backport

To create the backport manually run:

node scripts/backport --pr 126088

Questions ?

Please refer to the Backport tool documentation

juliaElastic added 3 commits February 21, 2022 15:07

optimizing package policy upgrade and dry run

89775a5

optimizing package policy upgrade and dry run

778adca

optimizing package policy upgrade and dry run

9b88ebe

juliaElastic added release_note:skip Skip the PR/issue when compiling release notes v8.2.0 labels Feb 21, 2022

juliaElastic requested a review from a team as a code owner February 21, 2022 14:20

juliaElastic self-assigned this Feb 21, 2022

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Feb 21, 2022

juliaElastic commented Feb 21, 2022

View reviewed changes

x-pack/plugins/fleet/server/services/package_policy.ts Show resolved Hide resolved

juliaElastic added 3 commits February 21, 2022 15:53

missing params

46671c8

fixed tests

5326f73

removed unused import

98dcb58

juliaElastic added 2 commits February 22, 2022 11:41

fixed tests

a1eb081

fixed checks

349af05

juliaElastic mentioned this pull request Feb 22, 2022

[Fleet] Refactorize package policy service #119844

Open

juliaElastic commented Feb 22, 2022

View reviewed changes

juliaElastic added 2 commits February 23, 2022 10:00

reduced duplication, separated validate logic

015dec1

reduced duplication, separated validate logic

b24fa22

kpollich approved these changes Feb 23, 2022

View reviewed changes

juliaElastic added the backport:skip This commit does not require backporting label Feb 23, 2022

Merge branch 'main' into dry-run-refactor

f74e211

juliaElastic enabled auto-merge (squash) February 23, 2022 13:53

Merge branch 'main' into dry-run-refactor

2e6e2a8

juliaElastic merged commit f96af16 into elastic:main Feb 23, 2022

juliaElastic deleted the dry-run-refactor branch February 23, 2022 17:06

jen-huang added auto-backport Deprecated - use backport:version if exact versions are needed v8.1.1 and removed backport:skip This commit does not require backporting labels Mar 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] optimizing package policy upgrade and dry run #126088

[Fleet] optimizing package policy upgrade and dry run #126088

juliaElastic commented Feb 21, 2022 •

edited

Loading

elasticmachine commented Feb 21, 2022

joshdover commented Feb 22, 2022 •

edited

Loading

kpollich commented Feb 22, 2022

juliaElastic Feb 22, 2022

kpollich Feb 22, 2022

juliaElastic Feb 22, 2022 •

edited

Loading

kpollich Feb 22, 2022

juliaElastic Feb 23, 2022 •

edited

Loading

kpollich Feb 23, 2022

juliaElastic Feb 23, 2022

kpollich left a comment

juliaElastic commented Feb 23, 2022

juliaElastic commented Feb 23, 2022

kibana-ci commented Feb 23, 2022

API count

jen-huang commented Mar 16, 2022 •

edited

Loading

kibanamachine commented Mar 16, 2022

	const packageInfoCache: Map<SharedKeyString, ArchivePackage \| RegistryPackage> = new Map();
	const sharedKey = ({ name, version }: SharedKey) => `${name}-${version}`;

	export const getPackageInfo = (args: SharedKey) => {
	return packageInfoCache.get(sharedKey(args));
	};

	const getPackageRes = await getPackageFromSource({
	pkgName,
	pkgVersion: responsePkgVersion,
	savedObjectsClient,
	installedPkg: savedObject?.attributes,
	});

	if (installedPkg && installedPkg.version === pkgVersion) {
	const { install_source: pkgInstallSource } = installedPkg;
	// check cache
	res = getArchivePackage({
	name: pkgName,
	version: pkgVersion,
	});

	if (res) {
	logger.debug(`retrieved installed package ${pkgName}-${pkgVersion} from cache`);
	}

	const [savedObject, latestPackage] = await Promise.all([
	getInstallationObject({ savedObjectsClient, pkgName }),
	Registry.fetchFindLatestPackageOrUndefined(pkgName),
	]);

[Fleet] optimizing package policy upgrade and dry run #126088

[Fleet] optimizing package policy upgrade and dry run #126088

Conversation

juliaElastic commented Feb 21, 2022 • edited Loading

Summary

elasticmachine commented Feb 21, 2022

joshdover commented Feb 22, 2022 • edited Loading

kpollich commented Feb 22, 2022

juliaElastic Feb 22, 2022

Choose a reason for hiding this comment

kpollich Feb 22, 2022

Choose a reason for hiding this comment

juliaElastic Feb 22, 2022 • edited Loading

Choose a reason for hiding this comment

kpollich Feb 22, 2022

Choose a reason for hiding this comment

juliaElastic Feb 23, 2022 • edited Loading

Choose a reason for hiding this comment

kpollich Feb 23, 2022

Choose a reason for hiding this comment

juliaElastic Feb 23, 2022

Choose a reason for hiding this comment

kpollich left a comment

Choose a reason for hiding this comment

juliaElastic commented Feb 23, 2022

juliaElastic commented Feb 23, 2022

kibana-ci commented Feb 23, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

History

jen-huang commented Mar 16, 2022 • edited Loading

kibanamachine commented Mar 16, 2022

⚪ Backport skipped

Manual backport

Questions ?

juliaElastic commented Feb 21, 2022 •

edited

Loading

joshdover commented Feb 22, 2022 •

edited

Loading

juliaElastic Feb 22, 2022 •

edited

Loading

juliaElastic Feb 23, 2022 •

edited

Loading

jen-huang commented Mar 16, 2022 •

edited

Loading