[Ingest Manager] rollover data stream when index template mappings are not compatible

[neptunian]

#68381

During an update, updating the current write index's mappings could be incompatible if a mapping type has changed, at which point a rollover of the index should take place.

It's not possible to easily test without having packages to update to in the registry that have incompatible mappings. Otherwise code changes would be necessary, so this should perhaps wait to be merged until those test packages are ready and I can include an integration test (#67372)

[elasticmachine]

Pinging @elastic/ingest-management (Team:Ingest Management)

[ruflin]

If the code is decoupled from specific packages (which I think it mostly is) you could also test without a package at the moment. What I had in mind:

Flow with compatible mapping

• Create template with foo: keyword in data stream
• Create data stream matching template
• Update data stream with mappings foo: keyword, bar: keyword
• Make sure no errors happen
• Make sure data stream mapping contains now both parts
• Make sure no rollover happened

Flow with incompatible mapping

• Create template with foo: keyword in data stream
• Create data stream matching template
• Update data stream with mappings foo.bar: keyword
• Make sure errors happen and rollver is triggered
• Make sure data stream mapping contains now foo.bar: keyword

The above does not mean we should not test with packages too in the future, but I think we can decouple this at the moment to have more local tests.

[neptunian]

What I was thinking is this is only called within the installPackage handler from the install endpoint eg: /packages/system-0.1.0. The test cases would fall under the api integration test of this install endpoint. It seems like it would be redundant to test some functions within the endpoint as integration tests. As of now, we don't have integration tests for anything other than APIs.

[ruflin]

@neptunian Not sure I follow what this means for the proposed tests. Does it mean we can't do it at the moment or that we should add it directly to the integrations tests?

[neptunian]

Looking closer after our conversation, I don't think I can test these functions outside of the API endpoint. They all require an argument for the ES callCluster for the current user which is passed to the request handlers when making the request to the endpoint through the API. I think this is why you had mentioned needing to create an endpoint for integration tests with ES.

[ruflin]

@elasticmachine merge upstream

[ruflin]

@neptunian I suggest we get this PR merged rather soonish and can still follow up with tests. This will also allow us to test it manually.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 04cbcdf

Build metrics

✅ unchanged

History

• 💚 Build #54121 succeeded d956de6
• 💚 Build #54113 succeeded 3b73d5c

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

[skh]

What do you think about adding the error message that comes from elasticsearch to the new error message? This can be helpful for troubleshooting.

[skh]

At some point, when we touch code, I think it would be a good idea to implement error handling as outlined in #66688 (as in: use IngestManagerError and adjust route handlers to react to it accordingly) but this can be added later too.

[skh]

I haven't tested whether the actual rollover happens, as you already agreed to add tests in a follow-up PR. Code looks good, and nothing breaks in local testing, so 👍!