Check diffMappings after WAIT_FOR_YELLOW_SOURCE

[gsoldevila]

Fixes a bug introduced by #147371.

The goal of the aforementioned PR was to avoid reindexing if the SO mappings didn't change.
But in that scenario we are still going to perform some operations on the index, and thus we must await for the index to be ready (aka WAIT_FOR_YELLOW_SOURCE).

This PR moves the diffMappings check (and the corresponding "shortcircuit") after the WAIT_FOR_YELLOW_SOURCE state + operation.

[jloleysens]

Great work @gsoldevila ! This logic makes sense to me! I added one comment/question that is not a blocker.

I expect we will also need to update src/core/server/integration_tests/saved_objects/migrations/skip_reindex.test.ts. It would be nice to also assert the full expected flow of INIT -> WAIT... -> PREPARE... there.

[jloleysens]

Just wanted to get your thoughts:

By passing aliases here it seems more likely that this object could be stale if other Kibana's see yellow source before us. We would reach the PREPARE_COMPATIBLE_MIGRATION step, get an alias_not_found exception and still go to OUTDATED_DOCUMENTS_SEARCH_OPEN_PIT. So all good?

[gsoldevila]

Fair point!

The only way I see to get fresher aliases information would be to perform a GET .kibana again (like the INIT step does), and even in that case, there's no way to guarantee that another instance will not delete the aliases before us, so I'm not sure it is worth it.

• If the other instance is the same version, and it performs the same cleanup of aliases before us, it's alright to proceed to OUTDATED_DOCUMENTS_SEARCH_OPEN_PIT on alias_not_found. As you said, odds of this happening are increasing with this fix, but the behavior / handling is still correct IMHO.

• If the other instance is a more recent version, it will obtain the aliases in the INIT step:

  • if it obtains them before we update / delete them in the PREPARE_COMPATIBLE_MIGRATION step, it will proceed to CHECK_UNKNOWN_DOCUMENTS + all the reindex steps, which don't use the aliases anymore.
  • if it obtains them after we update / delete them in the PREPARE_COMPATIBLE_MIGRATION step, it will reindex the source index into a new index without using the alias for anything. Then, it would likely override our .kibana alias and make it point to the newly created index. In this case we'd have some garbage version aliases pointing to an older index version, but that's already the case without this fix.

[rudolf]

++ there's always a potential race condition between the time we get the fetchAliases respose and performing the preTransformDocsActions. Before it was probably in the order of < 100ms and now it could potentially be many minutes. So this makes any potential race conditions more likely to occur but doesn't introduce anything new.

Like you both said, any race conditions should be handled in PREPARE_COMPATIBLE_MIGRATION already.

[gsoldevila]

Great work @gsoldevila ! This logic makes sense to me! I added one comment/question that is not a blocker.

I expect we will also need to update src/core/server/integration_tests/saved_objects/migrations/skip_reindex.test.ts. It would be nice to also assert the full expected flow of INIT -> WAIT... -> PREPARE... there.

I was waiting for CI feedback to see which integration tests need to be updated, thanks for the heads up!

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 738b8e4

Metrics [docs]

✅ unchanged

History

• 💔 Build #100504 failed 4488dcc

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

[rudolf]

++ there's always a potential race condition between the time we get the fetchAliases respose and performing the preTransformDocsActions. Before it was probably in the order of < 100ms and now it could potentially be many minutes. So this makes any potential race conditions more likely to occur but doesn't introduce anything new.

Like you both said, any race conditions should be handled in PREPARE_COMPATIBLE_MIGRATION already.