[Fleet] cleanup old package assets #112644

juliaElastic · 2021-09-21T09:51:22Z

Summary

Resolved #94340

Added logic to cleanup old version of package assets in case there are no dependent policies.
Calling this currently after package policy upgrade.
Next steps:

call same logic after package update
do a more generic check on all outdated package assets to clean up those that are already updated

How I tested:

install old version of apache e.g.

POST http://elastic:changeme@localhost:5601/julia/api/fleet/epm/packages/apache-0.3.3
kbn-xsrf: kibana

{ "force": true }

click on update to get 1.0.0 installed (with policy upgrade, if there are policies)
the cleanup logic runs, assets of 0.3.3 are deleted, while 1.0.0 remain

Tested with multiple old versions installed: 0.3.4, 0.3.3 and update -> both old versions are being deleted

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

joshdover · 2021-09-21T12:34:58Z

However didn't find a suitable function on server to remove old installation, as sendRemovePackage deletes the latest one, even though I call it with an older version.
Should I change the server to delete the given version?

In general, I think we should try to keep as much of this type of business logic on the server-side so that users who are using the Fleet API also get the same behavior as users of the UI.

I think the appropriate place to do this cleanup would be in the package policy service during upgrades: https://github.com/elastic/kibana/blob/master/x-pack/plugins/fleet/server/services/package_policy.ts/#L541

I suspect that you'll also need to add new logic for removing assets from an existing package without deleting the entire package (as you noted, the existing removeInstallation function isn't really what we want). We can probably add a new file for this behavior in a location like x-pack/plugins/fleet/server/services/epm/packages/cleanup.ts that performs this logic. The only things we really want to remove are the epm-packages-assets Saved Objects for any older package versions than the current one. We don't want to touch any Elasticsearch assets (such as index templates, pipelines, etc.).

juliaElastic · 2021-09-21T13:08:36Z

I think the appropriate place to do this cleanup would be in the package policy service during upgrades: https://github.com/elastic/kibana/blob/master/x-pack/plugins/fleet/server/services/package_policy.ts/#L541

Is it enough to clean up after package policy upgrade? What if a package is updated without any policies? Shouldn't cleanup happen after package update?

joshdover · 2021-09-21T13:16:36Z

Is it enough to clean up after package policy upgrade? What if a package is updated without any policies? Shouldn't cleanup happen after package update?

Good point, we should probably add logic during the package upgrade as well just for that case. Either way, I think we'll need new logic for removing these asset objects without removing the entire package.

joshdover · 2021-09-22T12:01:36Z

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

+    // how to query if more than one page?
+    perPage: 100,


I recommend using the savedObjectsClient.createPointInTimeFinder utility for paging which will give you a stable "cursor" of sorts that can be used in conjunction with the find API.

thanks, I'll check this

I rewrote this to use point in time finder

joshdover · 2021-09-22T12:02:23Z

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

+  const { total } = await packagePolicyService.list(savedObjectsClient, {
+    kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name:${pkgName} AND ${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.version:${oldVersion}`,
+    page: 0,
+    perPage: 0,


Will perPage: 0 ever return a total more than 0?

Also maybe we should do this check first before paging through all the refs and deleting them. That way we can delete the refs in batches without requiring that we page through all and load them into memory before deleting.

yes, I took this code from remove.ts where the same check is used, also tested and it returned > 0 if there were policies
good point about the order, will do the check first

joshdover · 2021-09-22T12:07:44Z

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

+  savedObjectsClient: SavedObjectsClientContract;
+  pkgName: string;
+  oldVersion: string;


I think we may want to change this API to handle the scenario where there are multiple outdated versions that haven't been cleaned up. This would help cover the case where customers have been using Fleet for a while before they upgrade to the version that has this cleanup logic. So instead, I'd expect that we pass in the currentVersion and then do a best effort to cleanup all older assets that are not in use by a package policy.

yes, I was planning to extend as a next step.
do you think we can run the same cleanup logic on package update/policy upgrade for all older versions? so we don't need two versions of cleanup.
regarding the implementation, I was thinking to use the find api to find all lower version assets and check for each version that there are no dependent policies. would that work with an aggregation to find distinct versions?

I updated this to find all older versions and delete assets for each. Tested with multiple old versions, seems to be working fine.

do you think we can run the same cleanup logic on package update/policy upgrade for all older versions? so we don't need two versions of cleanup.

I can't think of a reason it wouldn't work for both scenarios. It would be good to reuse the same logic for each.

regarding the implementation, I was thinking to use the find api to find all lower version assets and check for each version that there are no dependent policies. would that work with an aggregation to find distinct versions?

An aggregation makes total sense to me. The find API does support aggregations as well, let me know if you have any trouble, but I suspect a terms aggregation on the package_policy field should yield what you're trying to achieve. This will give you the number of documents that match each unique version keyword.

yes, I found that, it is working

juliaElastic · 2021-09-22T14:26:54Z

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

@@ -267,7 +268,12 @@ async function installPackageFromRegistry({
      installType,
      installSource: 'registry',
    })
-      .then((assets) => {
+      .then(async (assets) => {
+        await removeOldAssets({


doing cleanup after installing (updating) a package as well

elasticmachine · 2021-09-23T07:58:24Z

Pinging @elastic/fleet (Team:Fleet)

kpollich

Code looks good after latest changes, and all works well. Thanks for the tests as well. 🚀

juliaElastic · 2021-09-27T07:53:03Z

@elasticmachine merge upstream

kibanamachine · 2021-09-27T09:51:44Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #155278 succeeded ba2d6e2
💔 Build #155211 failed 8511eff
💔 Build #154991 failed 53fda55
💚 Build #154920 succeeded ac9cd50
💚 Build #154503 succeeded 699778ebb007277b828114b24db1d159925b70f7

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

* cleanup on server side * cleanup all older versions * fixed type errors * added unit tests Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-09-27T09:54:28Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* cleanup on server side * cleanup all older versions * fixed type errors * added unit tests Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: juliaElastic <[email protected]>

cleanup on server side

ac9cd50

juliaElastic force-pushed the cleanup-package branch from fbb6519 to ac9cd50 Compare September 22, 2021 11:43

joshdover reviewed Sep 22, 2021

View reviewed changes

cleanup all older versions

53fda55

juliaElastic commented Sep 22, 2021

View reviewed changes

fixed type errors

8511eff

juliaElastic added auto-backport Deprecated - use backport:version if exact versions are needed Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 v8.0.0 labels Sep 23, 2021

juliaElastic marked this pull request as ready for review September 23, 2021 07:58

juliaElastic requested a review from a team as a code owner September 23, 2021 07:58

juliaElastic added the release_note:skip Skip the PR/issue when compiling release notes label Sep 23, 2021

juliaElastic and others added 2 commits September 23, 2021 13:08

added unit tests

ab6ac60

Merge branch 'elastic:master' into cleanup-package

ba2d6e2

kpollich approved these changes Sep 23, 2021

View reviewed changes

Merge branch 'master' into cleanup-package

fcdd9c7

juliaElastic enabled auto-merge (squash) September 27, 2021 07:58

juliaElastic merged commit aeaad1f into elastic:master Sep 27, 2021

kibanamachine mentioned this pull request Sep 27, 2021

[7.x] [Fleet] cleanup old package assets (#112644) #113101

Merged

juliaElastic deleted the cleanup-package branch September 27, 2021 09:54

kpollich mentioned this pull request Nov 9, 2021

[Change Proposal] Add package_policy_upgrade_strategy field to support Fleet upgrade behavior elastic/package-spec#244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] cleanup old package assets #112644

[Fleet] cleanup old package assets #112644

juliaElastic commented Sep 21, 2021 •

edited

Loading

joshdover commented Sep 21, 2021

juliaElastic commented Sep 21, 2021

joshdover commented Sep 21, 2021

joshdover Sep 22, 2021

juliaElastic Sep 22, 2021

juliaElastic Sep 22, 2021

joshdover Sep 22, 2021

juliaElastic Sep 22, 2021

joshdover Sep 22, 2021

juliaElastic Sep 22, 2021

juliaElastic Sep 22, 2021

joshdover Sep 22, 2021

juliaElastic Sep 22, 2021

juliaElastic Sep 22, 2021

elasticmachine commented Sep 23, 2021

kpollich left a comment

juliaElastic commented Sep 27, 2021

kibanamachine commented Sep 27, 2021

kibanamachine commented Sep 27, 2021

[Fleet] cleanup old package assets #112644

[Fleet] cleanup old package assets #112644

Conversation

juliaElastic commented Sep 21, 2021 • edited Loading

Summary

Checklist

joshdover commented Sep 21, 2021

juliaElastic commented Sep 21, 2021

joshdover commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Sep 23, 2021

kpollich left a comment

Choose a reason for hiding this comment

juliaElastic commented Sep 27, 2021

kibanamachine commented Sep 27, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Sep 27, 2021

💚 Backport successful

juliaElastic commented Sep 21, 2021 •

edited

Loading