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

[Profiling] Manage indices via Elasticsearch #157949

Merged
merged 33 commits into from
Jun 5, 2023

Conversation

jbcrail
Copy link
Contributor

@jbcrail jbcrail commented May 17, 2023

Summary

This PR sets up the profiling indices via Elasticsearch instead of using the single-click installation in Kibana.

Notable changes

  • Set Elasticsearch flag (xpack.profiling.templates.enabled) to true so that indices are bootstrapped in Elasticsearch
  • Remove component templates and mappings (handled in Elasticsearch plugin now)
  • Remove six steps from Kibana's single-click installation (handled in Elasticsearch plugin now)
  • Use new status API in Elasticsearch to check if resources (templates, indices, etc) are ready
  • Invoke creation of collector and symbolizer package policies in parallel to updating APM policy
  • Refactor initialization and verification steps so that we can later give the user more granular insight into the status of steps

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

@jbcrail jbcrail force-pushed the manage-indices-via-elasticsearch branch from ac3a554 to 7273fd9 Compare May 17, 2023 01:33
@jbcrail jbcrail force-pushed the manage-indices-via-elasticsearch branch 2 times, most recently from 1798b6e to 3f7e318 Compare May 24, 2023 20:47
@jbcrail jbcrail marked this pull request as ready for review May 24, 2023 22:57
@jbcrail jbcrail requested a review from a team as a code owner May 24, 2023 22:57
@jbcrail jbcrail added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.9.0 labels May 24, 2023
Comment on lines 20 to 21
const isProfilingTemplatesEnabled =
settings.persistent.xpack?.profiling?.templates?.enabled ?? false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check can likely be replaced by querying the new status API after it is merged.

@jbcrail
Copy link
Contributor Author

jbcrail commented May 25, 2023

The latest Cypress tests fail with the following error:

Failed to fetch latest version of profiler_symbolizer from registry: Cannot read properties of null (reading 'version')

@inge4pres @florianl Is this error related to the configuration in x-pack/plugins/profiling/server/lib/setup/steps/get_fleet_policy_step.ts? I updated to the latest version for the symbolizer package. However, the tests failed both before and after this change.

@florianl
Copy link
Member

The latest Cypress tests fail with the following error:

Failed to fetch latest version of profiler_symbolizer from registry: Cannot read properties of null (reading 'version')

maybe @joshdover or @kpollich can tell more about this error?

The package profiler_symbolizer exists now for some time and wasn't touched since its creation. And the bundling with kibana did also not change:

{
"name": "profiler_symbolizer",
"version": "8.8.0-preview",
"forceAlignStackVersion": true,
"allowSyncToPrerelease": true
},

@jbcrail jbcrail force-pushed the manage-indices-via-elasticsearch branch 3 times, most recently from b35cec8 to cfa7b90 Compare May 26, 2023 02:15
@rockdaboot
Copy link
Contributor

rockdaboot commented May 26, 2023

To understand the error, the code around here is key:

`Failed to fetch latest version of ${packageName} from registry: ${error.message}`

@rockdaboot rockdaboot requested a review from a team as a code owner May 26, 2023 07:22
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label May 26, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@rockdaboot rockdaboot marked this pull request as draft May 26, 2023 07:24
@florianl
Copy link
Member

With the following command I verified that the package is available:

$ curl -s "https://epr.elastic.co/search?package=profiler_symbolizer&prerelease=true&all=true"
[
  {
    "name": "profiler_symbolizer",
    "title": "Universal Profiling Symbolizer",
    "version": "8.8.0-preview",
    ...
    }
 ]

Not sure where and why it fails here.

@rockdaboot rockdaboot force-pushed the manage-indices-via-elasticsearch branch from 976143a to 246c027 Compare May 26, 2023 07:59
@rockdaboot rockdaboot removed the Team:Fleet Team label for Observability Data Collection Fleet team label May 26, 2023
@rockdaboot rockdaboot marked this pull request as ready for review May 26, 2023 08:01
@rockdaboot
Copy link
Contributor

rockdaboot commented May 26, 2023

With the following command I verified that the package is available:

$ curl -s "https://epr.elastic.co/search?package=profiler_symbolizer&prerelease=true&all=true"
[
  {
    "name": "profiler_symbolizer",
    "title": "Universal Profiling Symbolizer",
    "version": "8.8.0-preview",
    ...
    }
 ]

Not sure where and why it fails here.

Yes, the point is that the prerelease is set to false for some reasons.
(I logged the URL in a commit that I meanwhile removed.)

This corrects the URL, but then the tests the fail later on, not sure if that is related to UP.

diff --git a/x-pack/plugins/fleet/server/services/epm/registry/index.ts b/x-pack/plugins/fleet/server/services/epm/registry/index.ts
index 3b9b37f31d9..ff3768e3118 100644
--- a/x-pack/plugins/fleet/server/services/epm/registry/index.ts
+++ b/x-pack/plugins/fleet/server/services/epm/registry/index.ts
@@ -100,7 +100,7 @@ async function _fetchFindLatestPackage(
 
     // temporary workaround to allow synthetics package beta version until there is a GA available
     // needed because synthetics is installed by default on kibana startup
-    const prereleaseAllowedExceptions = ['synthetics'];
+    const prereleaseAllowedExceptions = ['synthetics', 'profiler_symbolizer'];
 
     const prereleaseEnabled = prerelease || prereleaseAllowedExceptions.includes(packageName);

Maybe someone knows how to set prerelease=true without changing the code !?

@inge4pres inge4pres self-assigned this May 26, 2023
@jbcrail jbcrail force-pushed the manage-indices-via-elasticsearch branch 2 times, most recently from 543aa59 to 92092ee Compare May 27, 2023 03:15
@jbcrail
Copy link
Contributor Author

jbcrail commented May 27, 2023

I found out today that the prerelease error was not the reason why the tests were failing. It turns out that the steps needed to set up the policies, permissions, and settings were running at the same time as Elasticsearch was setting up the templates, indices, etc, so it was likely a timing issue instead.

Unfortunately one of the UI tests is failing after I fixed several things, but at least we have progress. I worked on these today:

  • use new status API in Elasticsearch to check if resources (templates, indices, etc) are ready
  • invoke creation of collector and symbolizer package policies in parallel to updating APM policy
  • refactor initialization and verification steps so that we can later give the user more granular insight into the status of steps

Here's what left to do next week:

  • address any feedback
  • fix failed UI test
  • remove duplication
  • add checks to see if the collector and symbolizer package policies were created

@jbcrail jbcrail force-pushed the manage-indices-via-elasticsearch branch from 893bb75 to 43f4404 Compare June 5, 2023 20:14
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 497 501 +4
total +6

History

  • 💚 Build #132321 succeeded a7cff1f4e7c366afc54675f796bcd191b8ed594a
  • 💔 Build #132297 failed 69e9a654e1b7fb22662bc70dbbf50868b80c5379
  • 💚 Build #132264 succeeded 4b575aef65d726b1f07f7a8eb91005f90689f60e
  • 💚 Build #132231 succeeded fd210ce23834760321b9f93b00435072096c1e47
  • 💚 Build #131947 succeeded d5e21de9c349bc4f8d8b0eb197ded3e01e48f851
  • 💛 Build #131925 was flaky fa477a41305ed3b8133bdfc011e3fa13bd437b84

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

cc @inge4pres

@jbcrail jbcrail merged commit 98dd709 into elastic:main Jun 5, 2023
@jbcrail jbcrail deleted the manage-indices-via-elasticsearch branch June 5, 2023 21:36
jbcrail added a commit that referenced this pull request Jun 7, 2023
## Summary

This PR addresses two bugs discovered after
#157949:

* when setting up security roles, the incorrect status was set
* when merging the statuses of all steps, the merged status could lose
some statuses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants