Skip to content
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] Remove upgradePackage and consolidate it with installPackage, optimize calls to create index patterns #94490

Merged
merged 30 commits into from
Mar 23, 2021

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Mar 11, 2021

Summary

This PR resolves Fleet plugin's portion of #91715 by removing direct calls to index pattern saved objects in server/services/epm/kibana/index_pattern/install.ts and replacing them with calls to data plugin's IndexPatternService.

Changes were made to data plugin's IndexPatternService to allow non-scripted fields to be returned from getAsSavedObjectBody when allowNoIndex is true: afc91ce. This allows the call to create and save the index pattern to work correctly.

March 17 update:

  • This PR no longer has any change to the way EPM installs index patterns, the current IndexPatternService does not work for the way EPM installs index patterns. We decided to pull out the changes I had here to get the service working for EPM in favor of having more discussions about potential solutions first, so unfortunately we still need to write directly to the index pattern saved objects themselves in the meantime 😞
  • However, this PR still sets us up to easily swap out SO for IndexPatternService when that's ready. Plus I think some of the simplifications introduced in this PR are beneficial for our error handling efforts, so I'd like to still get these changes in if the team agrees.

This PR introduces the following changes to EPM package installation:

  • Removes upgradePackage method, which was only being used by bulkInstallPackages, since installPackage can do the same thing (upgrade a package)
    • Adjusts the code in bulkInstallPackages to use installPackage instead
    • Preserve upgradePackage's use of handleInstallPackageFailure by adding it to installPackageFromRegistry
  • Adds new type InstallResult and use in various places where we previously had just AssetReference[], this new type will allow us to be more verbose about install results in the future, for now it includes an additional status: 'installed' | 'already_installed' property
  • Runs installIndexPatterns as a skippable post-install operation in installPackage, rather than in _installPackage()
    • This allows bulkInstallPackages to skip this operation and run installIndexPatterns after all package installations are done to avoid race conditions that create duplicate index patterns (for example, on plugin setup call where we install multiple default packages, multiple logs-* and metrics-* patterns were created)
    • The race condition was not an issue before when we were overwriting directly to SO based on ID We are keeping writing to SOs, but this still reduces the the amount of times we write to them
  • Removes our custom index pattern field typings in favor of the ones from data plugin and adjust our field generation code to return the correct field format
    • analyzed and doc_value properties were removed for index pattern fields after offline discussions, they are not used by Kibana, they are preserved for index template fields
    • type will default to string for unknown types since IndexPatternService does not support undefined type
  • Adds logging around modified areas

Even though there are quite a lot of changes here, everything around package installation and index patterns should behave the same way as before. To test this PR, ensure that the index patterns are created properly, with the right fields, whenever packages are installed, ungraded, or removed.

…rn creation & deletion, instead of interacting directly with index pattern SOs
…r of consolidating with installPackage(), use installPackage() for bulk install instead
…bulk package installation, install index pattern only if one or more packages are actually new installs, add debug logging
@jen-huang jen-huang self-assigned this Mar 11, 2021
@jen-huang jen-huang added auto-backport Deprecated - use backport:version if exact versions are needed Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0 v8.0.0 backport:skip This commit does not require backporting labels Mar 16, 2021
This reverts commit b9770fd.
@jen-huang jen-huang marked this pull request as ready for review March 16, 2021 21:33
@jen-huang jen-huang requested review from a team as code owners March 16, 2021 21:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@mattkime
Copy link
Contributor

@jen-huang I'm skeptical but we can discuss it tomorrow.

@skh
Copy link
Contributor

skh commented Mar 17, 2021

@skh FYI on the changes to package installation here. I know you are working on a related area re: handling upgrade failures, I tried to focus on simplification in this PR, so I hope these changes are actually beneficial for that work. Let me know if there is anything in this PR that conflicts with your work or is a concern.

Changes look good to me, good job on the simplification! I'll have to redo some bits from what I'm working on, but if this PR gets in reasonably quickly that's not a problem at all.

@lizozom lizozom requested a review from mattkime March 17, 2021 15:58
@jonathan-buttner
Copy link
Contributor

jonathan-buttner commented Mar 17, 2021

@jen-huang I tested installing the endpoint package using the _bulk API and upgrading it and both worked:

Installing the package for the first time

image

Upgrading to 0.19.0

image

I noticed that the transform was being reinitialized on each _bulk API call ever after the latest package is installed though.

@jen-huang jen-huang removed request for a team and mattkime March 17, 2021 23:55
@jen-huang jen-huang changed the title [Fleet] Use indexPatternService instead of writing directly to SOs [Fleet] Remove upgradePackage and consolidate it with installPackage, optimize calls to create index patterns Mar 18, 2021
@jen-huang
Copy link
Contributor Author

@elastic/fleet @jonathan-buttner Per discussion with @mattkime and others, we decided to table the changes to IndexPatternService for now, so this means EPM still needs to rely on writing directly to SOs. I've updated the code accordingly and updated PR description with more details. Let me know if any Qs or concerns about the rest of the changes. Thank you!

@jen-huang
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 344.7KB 344.4KB -324.0B

History

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

cc @jen-huang

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this locally and did not manage to break it, so I'd say let's get this in rather sooner than later 🚀

I very much like the simplifications in the package installation parts.

@jen-huang
Copy link
Contributor Author

Thanks for testing this @skh, appreciate it! I will merge and backport this now.

@jen-huang jen-huang merged commit d886979 into elastic:master Mar 23, 2021
@jen-huang jen-huang deleted the epm/missing-index-patterns branch March 23, 2021 18:34
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Mar 23, 2021
…ge`, optimize calls to create index patterns (elastic#94490)

* Add data plugin to server app context

* First attempt at switching to indexPatternService for EPM index pattern creation & deletion, instead of interacting directly with index pattern SOs

* Simplify bulk install package, remove upgradePackage() method in favor of consolidating with installPackage(), use installPackage() for bulk install instead

* Update tests

* Change cache logging of objects to trace level

* Install index patterns as a post-package installation operation, for bulk package installation, install index pattern only if one or more packages are actually new installs, add debug logging

* Allow getAsSavedObjectBody to return non-scripted fields when allowNoIndex is true

* Allow `getFieldsForWildcard` to return fields saved on index pattern when allowNoIndices is true

* Bring back passing custom ID for index pattern SO

* Fix tests

* Revert "Merge branch 'index-pattern/allow-no-index' into epm/missing-index-patterns"

This reverts commit 8e712e9, reversing
changes made to af0fb0e.

* Allow getAsSavedObjectBody to return non-scripted fields when allowNoIndex is true

(cherry picked from commit 69b93da)

* Update API docs

* Run post-install ops for install by upload too

* Remove allowedInstallTypes param

* Adjust force install conditions

* Revert "Update API docs"

This reverts commit b9770fd.

* Revert "Allow getAsSavedObjectBody to return non-scripted fields when allowNoIndex is true"

This reverts commit afc91ce.

* Go back to using SO client for index patterns :(

* Stringify attributes again for SO client saving

* Fix condition for reinstall same pkg version

* Remove unused type

* Adjust comment

* Update snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #95219

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 23, 2021
…ge`, optimize calls to create index patterns (#94490) (#95219)

* Add data plugin to server app context

* First attempt at switching to indexPatternService for EPM index pattern creation & deletion, instead of interacting directly with index pattern SOs

* Simplify bulk install package, remove upgradePackage() method in favor of consolidating with installPackage(), use installPackage() for bulk install instead

* Update tests

* Change cache logging of objects to trace level

* Install index patterns as a post-package installation operation, for bulk package installation, install index pattern only if one or more packages are actually new installs, add debug logging

* Allow getAsSavedObjectBody to return non-scripted fields when allowNoIndex is true

* Allow `getFieldsForWildcard` to return fields saved on index pattern when allowNoIndices is true

* Bring back passing custom ID for index pattern SO

* Fix tests

* Revert "Merge branch 'index-pattern/allow-no-index' into epm/missing-index-patterns"

This reverts commit 8e712e9, reversing
changes made to af0fb0e.

* Allow getAsSavedObjectBody to return non-scripted fields when allowNoIndex is true

(cherry picked from commit 69b93da)

* Update API docs

* Run post-install ops for install by upload too

* Remove allowedInstallTypes param

* Adjust force install conditions

* Revert "Update API docs"

This reverts commit b9770fd.

* Revert "Allow getAsSavedObjectBody to return non-scripted fields when allowNoIndex is true"

This reverts commit afc91ce.

* Go back to using SO client for index patterns :(

* Stringify attributes again for SO client saving

* Fix condition for reinstall same pkg version

* Remove unused type

* Adjust comment

* Update snapshot

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Jen Huang <its.jenetic@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants