-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Move mappings from index template to component template #124013
[Fleet] Move mappings from index template to component template #124013
Conversation
294949f
to
9f94cd5
Compare
Pinging @elastic/fleet (Team:Fleet) |
Can you elaborate on this? What is exactly merged with what?
I assume you refer here to settings made into the package and not by the users so these are constant for a certain version of a package. More general question, it would be good to have docs / overview of all the templates that exist for each data stream and what the priority of each is. You have started this above but I think at least How is the migration going to work to this new setup? As the full change is non breaking I assume no rollover of the data streams is triggered. So the new structure will only be visible after the rollover of a data stream? Could this have any side effects? |
+1 on this, I thought that the package-specific mappings would be in |
What's the difference between putting this in the index template and putting this in the |
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/install.ts
Show resolved
Hide resolved
e2206f3
to
6e00c6b
Compare
@elasticmachine merge upstream |
@joshdover @ruflin I have rewritten the PR description to hopefully be clearer about the changes I am making. Here are some quick summaries to address your comments above:
|
Looked at the list of component templates again above and I think the
Do we really have to reinstall? I don't know exactly what happens on reinstall but I assume it will push all assets to the stack again. If someone has quite a few packages installed, this could take quite a bit of time. I understand it is to cleanup. My assumption is that the summary of all mappings before and after this change is identical. The ingest pipeline names are different. If we do these changes, do we have to force it on startup? Lets assume we got something wrong, now Kibana can't start anymore? Is there any asset that exists now but is not around anymore afterwards? Could it happen that we get stuck in the middle of the migration? My concerns are not around the change itself but I want to make sure, migration (if needed) for our users is seamless and we don't block any users from upgrading. |
We're planning to post documentation about how to use For context, the change to reinstall bundled packages in this PR is more of a fix than a new change. We already reinstalled registry packages when these global component templates changed (eg. in the 8.0.0 release for #119380), but it wasn't being done for bundled packages because those are marked as being installed by upload.
We don't block or fail Kibana startup today and don't plan to in the near future. So if something failed, Kibana would still be functional, but Fleet may not be able to setup correctly. We should always be sure that Fleet can setup correctly though :)
Nevermind, I see it was addressed in the description above, this template is not removed for now. |
@hop-dev is there a reason we need to retain the As an aside, do you know what this is actually used for? Maybe the |
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.
Looking good! This is my first(-ish) pass, still need to do more in-depth upgrade testing and review of test code, but would like some of these questions answer first.
Questions:
- Is there any reason we don't want the content in
.fleet_globals-1
to be override-able by@custom
templates? It currently contains the dynamic template for forcingkeyword
mappings and thedate_detection: false
setting.
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
Yes that is the case, I've added a section in the PR description. I've been trying to run some performance tests on the reinstalls but I've been severely hampered by #126611 causing too many install failures at the moment. Anecdotally for 20 packages I was seeing individual install times of approx 20 - 50s (but with an error rate of ~20%) but the whole process taking ~75s as they are run concurrently. |
3765433
to
c067e5e
Compare
@joshdover @nchaulet I've created an issue for |
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.
Last few comments on the implementation here. Working on some final manual testing right now.
@@ -540,14 +536,13 @@ const updateExistingDataStream = async ({ | |||
// update settings after mappings was successful to ensure | |||
// pointing to the new pipeline is safe | |||
// for now, only update the pipeline | |||
const { settings } = indexTemplate.template; | |||
if (!settings.index.default_pipeline) return; | |||
if (!settings?.index?.default_pipeline) return; |
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.
I wonder if there's ever an issue in a case where a pipeline was removed from an integration package and we don't unset the pipeline from the current write index.
Maybe we should track a separate issue for this?
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts
Outdated
Show resolved
Hide resolved
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.
Ran some upgrade testing today and found no issues. Here's a summary of what I did:
- Start ES 8.2.0-SNAPSHOT (for simplicity's sake I didn't test upgrading ES)
- Start Kibana 8.1.0 and install several packages
- Make some changes to some
@custom
templates, for example I added settings and mappings to themetrics-system.cpu@custom
component template - Shutdown Kibana 8.1.0
- Start Kibana from this branch against the same ES instance
- Wait for Fleet setup to complete
- Verify that @Custom settings and mappings were preserved and new @mappings template was added to all index templates.
I'm comfortable merging this PR after my last few suggestions are fixed. I do wonder if we're introducing a subtle breaking change by allowing @custom
to override mappings when they previously couldn't. However, I think being able to override the mappings was the intended behavior and we could consider the old behavior a bug.
Confirmed with @ruflin that this is the desired behavior. LGTM ✅ |
204a4c7
to
67aafbb
Compare
💚 Build SucceededMetrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @hop-dev |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Thanks Jen! |
…tic#124013) * cherry pick from old branch * use global template * re-add _meta to index template * simplify merge * fix component_template test * add settings to mapping component template * put mapping settings on @mapping component template * fix snapshot * fix integration tests * fix component template order and test * use mapping component template for updating backing indices * fix tests * move code to util functions * add comment * split global templates * update snaphsot * rename variables * fix tests again * reinstall bundled packages when component templates change * combine compoised_of generation statements * remove duplicated global fields * remove unnecessary rollover call * use rollover API * use simulate API to get template content * remove unused parameters * fix unit test * improve simulate error handling * re-add removed mapping assertions * fix test
Hi @hop-dev
Observations: As per results there are 02 missing properties and 36 unequal results.
Build details: Json file copied from fleet: Please let us know if we are missing anything. |
Hi @amolnater-qasource, thanks for this, I have reviewed the differences and they are all acceptable ✅ . The issue is that I generated |
Hi @hop-dev Please let us know if anything else is required from our end. |
Summary
Closes #121184.
A few changes to the component templates we create as part of package install and fleet setup:
@mapping
component template. This means that every data stream will have the@mapping
component template going forward..fleet_component_template-1
into 2 component templates (more detail below):.fleet_globals-1
- contains fleet global settings previously applied to the index template.fleet_agent_id_verification-1
- optionally added when agent id verification is enabled (basically just sets.fleet_final_pipeline-1
)These changes will be applied to all existing datastreams on startup by force installing each package, triggering a rollover.
Technical Details / Decisions
What to do with
.fleet_component_template-1
?I had considered deleting the old component template to "clean up", however I have taken the stance that this introduces more risk than benefit. There is the chance that a data stream is still using the component template. We could investigate a way of finding out if it is being used but I thought there is little harm in leaving the template.
Package Custom Mappings
We do allow integrations to specify custom elastic mappings directly (on top of the field mappings that are calculated from the package .yml files in the package spec) for examples APM enables dynamic mapping.
Previously these custom mappings were the ONLY mappings specified in the
@mapping
component template, and therefore the@mapping
component template was only created if a package specified these mappings. This PR moves ALL mappings into the@mapping
component template, so we manually merge the field mappings with the packages custom mappings if they are present (previously this was handled by template inheritence).Mapping settings
One complication has been that if an integration exceeds an elasticsearch default (e.g the max number of nested fields allowed) in the mapping for a given datastream, (e.g logs-endpoint.alerts
has more than 50 nested fields, so they have to add a settings override to increase that limit) then the
@mapping
component template becomes invalid and is rejected by elasticsearch. This means we have to add any custom index settings which are related to the mapping to ensure the component template is valid. This means the@mapping
component template now must containsettings.index.mapping
template settings.New component template hierarchy and content
Here is a summary of the index and component template content as they now stand (highest precedence to lowest precedence):
.fleet_agent_id_verification-1
- optionally added when agent id verification is enabled, sets the.fleet_final_pipeline-1
and agent ID mappings..fleet_globals-1
- contains fleet global settings and mappings, applied to every data stream@custom
component template - empty, available for user to apply custom settings@settings
component template - fleet default settings plus any custom settings specified by the integration (EXCEPTsettings.index.mapping
in both cases)@mappings
component template - fleet default settings forsettings.index.mapping
(e.g field_limit=10000) and anysettings.index.mapping
custom settings specified by the integration. Plus also all field mappings which previously were specified on the index template.Test Steps
In order to test this we need to verify that the computed template remains the same now that we have moved the mapping to the component template.
We also need to verify that any custom mappings set by the package are applied, for example APM sets custom mappings.
the broad test steps are as follows:
Part 1 (on
main
branch)If you want to skip step 1 here is the computed template for
metrics-apm.app
from the APM integration:metrics-apm.app template
Part 2 (on this feature branch, clean install)
Upgrade testing
To test that the new component templates are created and applied on upgrade, we can take a 8.1.0 system, set up some integrations and then switch branch and restart. It is important to test that both registry and bundled packages are force reinstalled on first startup.
Detailed instructions:
yarn build
) locally