[Ingest Manager] Fix package upgrade breaking after first rollover before new data has arrived

[neptunian]

Summary

Resolves #79370

If the user has rolled over a datastream in the past and tries to upgrade a package before any new data has arrived in the current write index, a failure will occur because the data_stream constant field mappings do not yet have values which are relied on to create the data stream name.

Change

Instead of relying on constant values of the datastream mappings in the current write index to compose the name of the datastream, query for the data_stream constant values amongst all indices

[elasticmachine]

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

[ruflin]

Is there also an option for us to ensure the values are always there / already set?

[jfsiii]

If I understand correctly indexName is a only a string vs some other type. Can we at least add some assertions to guard against undefined values or anything to give us more confidence it's what we expect?

[jonathan-buttner]

Yeah, it's probably worth checking. If the backing index prefix changes to something like .ds_logs-... we'd also have an invalid type. Maybe we should check that the type is either logs or metrics? I'm not sure if it's valid for it to be anything else. Or maybe we can use the enums that we use to create the kibana index patterns and make sure it at least is one of those that way we'd only have to change a single place to add new valid types.

[jonathan-buttner]

Is there also an option for us to ensure the values are always there / already set?

@ruflin do you mean avoid performing an upgrade until those values are present aka data has come in to populate those fields? For indices that don't receive data very often (metrics-endpoint.metadata-default only gets events like once a day per host I think), it could take a while before we'd allow an upgrade.

[neptunian]

A bit more complicated but we should be able update the component template {type-templateName}-mappings for eg logs-endpoint.events.library-mappings right before rolling over. This would require 2 requests to get the mappings of the current write index which has these values, update the component template with the constants' values, and then trigger the rollover.

[jonathan-buttner]

A bit more complicated but we should be able update the component template {type-templateName}-mappings for eg logs-endpoint.events.library-mappings right before rolling over. This would require 2 requests to get the mappings of the current write index which has these values, update the component template with the constants' values, and then trigger the rollover.

Oh interesting. What about the scenario where the ILM did the rollover so the empty backing index already exists and then we try to do an upgrade?

[neptunian]

Oh interesting. What about the scenario where the ILM did the rollover so the empty backing index already exists and then we try to do an upgrade?

Good point. Could always fetch the first backing index or perform a query on it:

GET .ds-metrics-system.socket_summary-default-000001/_search
{
  "size": 1,
  "query": {
    "bool": {
      "must": [
        {
          "exists": {
            "field": "data_stream.namespace"
          }
        }
      ]
    }
  }
}

or use a wildcard if we don't want to assume the name GET *metrics-system.socket_summary*/_search. And get the namespace this way. I think this is a good alternative if we're worried about using the index name.

[jonathan-buttner]

Oh interesting. What about the scenario where the ILM did the rollover so the empty backing index already exists and then we try to do an upgrade?

Good point. Could always fetch the first backing index or perform a query on it:

GET .ds-metrics-system.socket_summary-default-000001/_search
{
  "size": 1,
  "query": {
    "bool": {
      "must": [
        {
          "exists": {
            "field": "data_stream.namespace"
          }
        }
      ]
    }
  }
}

or use a wildcard if we don't want to assume the name GET *metrics-system.socket_summary*/_search. And get the namespace this way. I think this is a good alternative if we're worried about using the index name.

Yeah I think you can just search the data stream name: GET metrics-system.socket_summary*/_search instead of the backing index right?

[neptunian]

Yeah I think you can just search the data stream name: GET metrics-system.socket_summary*/_search instead of the backing index right?

Yes, you're right.

[jonathan-buttner]

nit: do we want to check that type is a valid DataType? I suppose if they ingested data initially with an invalid data type then we should just continue doing that 🤷

[neptunian]

Yea, i'm not sure this is the place to be validating that and whether we'd have to throw an error and stop the upgrade because of that.

[jfsiii]

Can we add any tests?

[neptunian]

Can we add any tests?

These existing tests would cover that it was rolled over with the expected name https://github.com/elastic/kibana/blob/master/x-pack/test/ingest_manager_api_integration/apis/epm/data_stream.ts

[jfsiii]

But didn't our existing tests not identify the original issue? Can we add a test which prevents a regression?

[neptunian]

But didn't our existing tests not identify the original issue? Can we add a test which prevents a regression?

It's true our existing test did not identify the condition that caused this bug, but the condition no longer exists where the data_stream constants would not exist in a particular index because we are no longer getting it from the current index, but checking any index in the data stream. The data stream would not exist if at least one document didn't exist and every document sends the value. So the current integration tests cover the case because it could not have rolled over if i didn't have the constants. Testing for the previous scenario would not cover the case, currently. I'd have to perform a 2nd rollover to see if it has the constants in the current write index but we no longer depend on subsequent rollovers to carry constants over.

[jonathan-buttner]

But didn't our existing tests not identify the original issue? Can we add a test which prevents a regression?

It's true our existing test did not identify the condition that caused this bug, but the condition no longer exists where the data_stream constants would not exist in a particular index because we are no longer getting it from the current index, but checking any index in the data stream. The data stream would not exist if at least one document didn't exist and every document sends the value. So the current integration tests cover the case because it could not have rolled over if i didn't have the constants. Testing for the previous scenario would not cover the case, currently. I'd have to perform a 2nd rollover to see if it has the constants in the current write index but we no longer depend on subsequent rollovers to carry constants over.

It'd probably still be worth creating a test that does:

1. Install a package
2. Add a document that populates the data_stream fields
3. Do a manual roll over
4. Ensure that an upgrade of the package succeeds

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 7ed638f

Metrics [docs]

page load bundle size

id	before	after	diff
ingestManager	391.5KB	391.8KB	+327.0B

History

• 💚 Build #81144 succeeded f0b9702
• 💚 Build #80847 succeeded edb3cc3
• 💚 Build #80532 succeeded 8773a00

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