Allow for additive mappings update without creating a new version index #149326

gsoldevila · 2023-01-23T13:59:19Z

Based on the same principle as #147371, the goal of this PR is to avoid reindexing if possible.
This time, the idea is to check whether the new mappings are still compatible with the ones stored in ES.
To to so, we attempt to update the mappings in place in the existing index, introducing a new CHECK_COMPATIBLE_MAPPINGS step:

If the update operation fails, we assume the mappings are NOT compatible, and we continue with the normal reindexing flow.
If the update operation succeeds, we assume the mappings ARE compatible, and we skip reindexing, just like #147371 does.

elasticmachine · 2023-01-23T13:59:23Z

Pinging @elastic/kibana-core (Team:Core)

gsoldevila · 2023-01-23T14:02:56Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts

@@ -81,8 +79,6 @@ export { createIndex } from './create_index';

 export { checkTargetMappings } from './check_target_mappings';

-export { updateTargetMappingsMeta } from './update_target_mappings_meta';


I generalised updateTargetMappingsMeta and renamed it as updateMappings.

gsoldevila · 2023-01-23T14:03:34Z

...jects/core-saved-objects-migration-server-internal/src/actions/update_and_pickup_mappings.ts

@@ -45,13 +45,11 @@ export const updateAndPickupMappings = ({
    RetryableEsClientError,
    'update_mappings_succeeded'
  > = () => {
-    // ._meta property will be updated on a later step
-    const { _meta, ...mappingsWithoutMeta } = mappings;


This shouldn't be this function's responsibility, it has been moved to next.ts.

gsoldevila · 2023-01-23T14:04:43Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts

@@ -488,7 +489,7 @@ export const model = (currentState: State, resW: ResponseType<AllActionStates>):
        // the mappings have changed, but changes might still be compatible
        return {
          ...stateP,
-          controlState: 'CHECK_UNKNOWN_DOCUMENTS',
+          controlState: 'CHECK_COMPATIBLE_MAPPINGS',


That's the root change of the PR; introduction of a new intermediate step that will attempt to update the mappings (excluding the _meta).

gsoldevila · 2023-01-23T14:34:20Z

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/next.ts

@@ -69,6 +71,12 @@ export const nextActionMap = (client: ElasticsearchClient, transformRawDocs: Tra
      Actions.fetchIndices({ client, indices: [state.currentAlias, state.versionAlias] }),
    WAIT_FOR_YELLOW_SOURCE: (state: WaitForYellowSourceState) =>
      Actions.waitForIndexStatus({ client, index: state.sourceIndex.value, status: 'yellow' }),
+    CHECK_COMPATIBLE_MAPPINGS: (state: CheckCompatibleMappingsState) =>
+      Actions.updateMappings({


We're effectively updating the index mappings earlier in the flow (if they are actually compatible).
With this change, if the mappings are compatible, the OUTDATED_DOCUMENTS _SEARCH + _TRANSFORM will be performed after updating the mappings (not the _meta).

This shouldn't be a problem cause the mappings are compatible, and we're going to perform an updateAndPickupMappings afterwards anyway.

jloleysens

I checked this out locally and confirmed that the flows described in the diagram are reproducible (tested with same mappings + new version and new compat. mappings and new version).

Great work @gsoldevila!

jloleysens · 2023-01-23T16:12:52Z

...re/saved-objects/core-saved-objects-migration-server-internal/src/actions/update_mappings.ts

+      })
+      .then(() => Either.right('update_mappings_succeeded' as const))
+      .catch(catchRetryableEsClientErrors)
+      .catch(() => Either.left('incompatible_mapping_exception'));


Just for my learning (and not part of this PRs changes):

is it possible that ES returns some other kind of error code here that we are mapping to incompatible_mapping_exception. I see the following for an incompatible mapping on a recent snapshot:

{ "error": { "root_cause": [ { "type": "illegal_argument_exception", "reason": "mapper [file.Status] cannot be changed from type [keyword] to [long]" } ], "type": "illegal_argument_exception", "reason": "mapper [file.Status] cannot be changed from type [keyword] to [long]" }, "status": 400 }

I'm not sure if there are others, didn't spot 'em. Might be more defensive to check for the type: 'illegal_argument_exception' and just throw for an unexpected condition.

I was fixing the CI errors and came to the same conclusion, this will be part of my next push.
TY!

…-ref HEAD~1..HEAD --fix'

gsoldevila · 2023-01-24T05:42:31Z

~~The current CI failures are integrations tests that use 7.13 archives to test legacy migrations.~~

~~Is it possible that the mappings are still compatible and that we're not creating the v2 style indices? I will try to confirm that.~~

~~I propose to leave this check here and only apply the enhancement for >= 7.14 migrations (aka v2 migrator)~~.
UPDATE: I was incorrectly assuming legacy migrations were those <= 7.14, and that the legacyIndex field of the state was an indicator of whether the migration was from legacy index or not.

From code comments, state.legacyIndex is:

The name of the concrete legacy index (if it exists) e.g. `.kibana` for <6.5 or `.kibana_task_manager` for < 7.4

This breaks my theory of the 7.13 archives having a legacy structure. I'll need to dig a bit deeper.

UPDATE: Adding a stateP.legacyIndex === undefined condition to our recent enhancements fixes all the 7.13 integration tests. But they just work cause I effectively disabled the enhancement by checking that property, which is always present 🤦🏼‍♂️ .

jloleysens

I propose to leave this check here and only apply the enhancement for >= 7.14 migrations (aka v2 migrator). WDYT?

This makes sense to me, I'd like to hear what @rudolf thinks! Do we specifically want this enhancement for users who are < 7.14?

rudolf · 2023-01-25T07:56:07Z

It's not important that we optimise the downtime from a legacy upgrade (< 7.3 for task manager, < 6.5.0 for .kibana!). But by the time we're done with the LEGACY_REINDEX_DELETE steps a legacy index looks like a v1/v2 index. That is it has a .kibana alias pointing to it and I don't think we should need to handle it very differently.

I think the reason the tests fail is because when we don't reindex we're unable to apply the excludeOnUpgradeQuery. We would need to have a kind of cleanup step that does a delete by query matching on the inverse of the excludeOnUpgradeQuery.

In the context of migrations, #147371 avoids reindexing during an upgrade, provided that `diffMappings === false`. This _alternative path_ skips some key steps that are performed before reindexing: * `CHECK_UNKNOWN_DOCUMENTS` * `CALCULATE_EXCLUDE_FILTERS` These steps enrich a search query that is used during reindexing, effectively filtering out undesired documents. If the mappings [match](#147371) (or they are [compatible](#149326)) and we _no longer reindex_, this cleanup operation does not happen, leaving undesired documents in our system indices. The goal of this PR is to add an extra step in the state machine (`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system index if we're going the _skip reindexing_ path. ![image](https://user-images.githubusercontent.com/25349407/216979691-fef40638-f990-4850-bac8-ee3e58330a7f.png) Fixes #150299 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

…ve-changes

In the context of migrations, elastic#147371 avoids reindexing during an upgrade, provided that `diffMappings === false`. This _alternative path_ skips some key steps that are performed before reindexing: * `CHECK_UNKNOWN_DOCUMENTS` * `CALCULATE_EXCLUDE_FILTERS` These steps enrich a search query that is used during reindexing, effectively filtering out undesired documents. If the mappings [match](elastic#147371) (or they are [compatible](elastic#149326)) and we _no longer reindex_, this cleanup operation does not happen, leaving undesired documents in our system indices. The goal of this PR is to add an extra step in the state machine (`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system index if we're going the _skip reindexing_ path. ![image](https://user-images.githubusercontent.com/25349407/216979691-fef40638-f990-4850-bac8-ee3e58330a7f.png) Fixes elastic#150299 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 754e868)

jloleysens

Great work @gsoldevila 👍🏻, this looks pretty comprehensive with the addition of the tests using the Kibana migrator kit. I left a couple of comments, mostly for my own understanding.

I tested the new upgrade locally by:

Introducing a compatible change to file SO (added a new field)
Deleted the existing Kibana .kibana_8.8.0 alias pointing to .kibana_8.8.0_001 to pretend we are doing a version upgrade

Confirmed that I saw the expected logs and Kibana works as expected. Did not do further testing.

Would be great to get a second (or third) pair of eyes before merging. Cheers!

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts

src/core/server/integration_tests/saved_objects/migrations/group2/outdated_docs.test.ts

# Backport This will backport the following commits from `main` to `8.7`: - [Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step (#149931)](#149931)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

…nt. tests

kibana-ci · 2023-02-28T16:17:27Z

💚 Build Succeeded

Buildkite Build
Commit: a53cedb

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	428	430	+2

Total ESLint disabled count

id	before	after	diff
`securitySolution`	506	508	+2

History

💔 Build #111053 failed 1b8e5c2
💔 Build #111052 failed 2eef9b5
💔 Build #110762 failed e9af690
💔 Build #110592 failed daf9636
💚 Build #108491 succeeded d7511c6

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

cc @gsoldevila

dokmic

LGTM! Great job 👍

In the context of migrations, elastic#147371 avoids reindexing during an upgrade, provided that `diffMappings === false`. This _alternative path_ skips some key steps that are performed before reindexing: * `CHECK_UNKNOWN_DOCUMENTS` * `CALCULATE_EXCLUDE_FILTERS` These steps enrich a search query that is used during reindexing, effectively filtering out undesired documents. If the mappings [match](elastic#147371) (or they are [compatible](elastic#149326)) and we _no longer reindex_, this cleanup operation does not happen, leaving undesired documents in our system indices. The goal of this PR is to add an extra step in the state machine (`CLEANUP_UNKNOWN_AND_EXCLUDED`), which will actively cleanup a system index if we're going the _skip reindexing_ path. ![image](https://user-images.githubusercontent.com/25349407/216979691-fef40638-f990-4850-bac8-ee3e58330a7f.png) Fixes elastic#150299 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

…ex (elastic#149326) Fixes [elastic#147237](elastic#147237) Based on the same principle as [elastic#147371](elastic#147371), the goal of this PR is to **avoid reindexing if possible**. This time, the idea is to check whether the new mappings are still compatible with the ones stored in ES. To to so, we attempt to update the mappings in place in the existing index, introducing a new `CHECK_COMPATIBLE_MAPPINGS` step: * If the update operation fails, we assume the mappings are NOT compatible, and we continue with the normal reindexing flow. * If the update operation succeeds, we assume the mappings ARE compatible, and we skip reindexing, just like [elastic#147371](elastic#147371) does. ![image](https://user-images.githubusercontent.com/25349407/216979882-9fe9f034-b521-4171-b85d-50be6a13e179.png) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

gsoldevila added 2 commits January 23, 2023 14:34

Try to update mappings to find out if they are compatible

12a635e

Merge branch 'main' into kbn-147237-allow-additive-changes

0b35f91

gsoldevila requested a review from a team as a code owner January 23, 2023 13:59

gsoldevila commented Jan 23, 2023

View reviewed changes

jloleysens approved these changes Jan 23, 2023

View reviewed changes

gsoldevila and others added 2 commits January 23, 2023 18:39

Fix some of the CI errors

b348929

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

0cd479e

…-ref HEAD~1..HEAD --fix'

gsoldevila added 2 commits January 24, 2023 11:09

Do not skip reindexing if we are migrating from v1 state

5ad4637

Merge branch 'main' into kbn-147237-allow-additive-changes

c89fa4d

jloleysens approved these changes Jan 24, 2023

View reviewed changes

Revert attempt to NOT skip reindexing for legacy migrations

385f1ed

jloleysens self-requested a review January 26, 2023 16:52

rudolf assigned gsoldevila Jan 27, 2023

Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step

85217b9

gsoldevila mentioned this pull request Jan 31, 2023

Introduce CLEANUP_UNKNOWN_AND_EXCLUDED step #149931

Merged

gsoldevila added 3 commits January 31, 2023 13:19

Fix UTs

5920fed

Add REMOVED_TYPES to the deleteQuery

67728f2

Perform active cleanup before updating aliases

dbff9a8

gsoldevila added 6 commits February 21, 2023 15:40

Merge branch 'main' into kbn-147237-active-cleanup

f170cb6

Add retry mechanism to reduce number of FATALs

887ce7d

Misc enhancements

c4345b4

Fix linting and failing tests

485940a

Merge branch 'main' into kbn-147237-active-cleanup

6aeee3b

Merge branch 'main' into kbn-147237-active-cleanup

561febb

gsoldevila added 2 commits February 27, 2023 11:57

Merge branch 'kbn-147237-active-cleanup' into kbn-147237-allow-additi…

5849c72

…ve-changes

Merge branch 'main' into kbn-147237-allow-additive-changes

daf9636

jloleysens approved these changes Feb 27, 2023

View reviewed changes

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/model/model.ts Show resolved Hide resolved

src/core/server/integration_tests/saved_objects/migrations/group2/outdated_docs.test.ts Outdated Show resolved Hide resolved

gsoldevila added 3 commits February 27, 2023 18:03

Address PR comments

7d90ab8

Merge branch 'main' into kbn-147237-allow-additive-changes

bbac6db

Reorder code for clarity

e9af690

gsoldevila added 5 commits February 28, 2023 14:51

Update IncompatibleMappingException Left return type

454d717

Merge branch 'main' into kbn-147237-allow-additive-changes

8e48295

Update corresponding UT

2eef9b5

Fix model.test.ts

1b8e5c2

Rename CheckCompatibleMappings => UpdateSourceMappings. Fix failing i…

a53cedb

…nt. tests

dokmic approved these changes Mar 1, 2023

View reviewed changes

gsoldevila merged commit 5dd8742 into elastic:main Mar 1, 2023

rudolf added the Epic:ScaleMigrations Scale upgrade migrations to millions of saved objects label Jun 2, 2023

rudolf mentioned this pull request Sep 23, 2023

Scale upgrade migrations to millions of saved objects #144035

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for additive mappings update without creating a new version index #149326

Allow for additive mappings update without creating a new version index #149326

gsoldevila commented Jan 23, 2023 •

edited

Loading

elasticmachine commented Jan 23, 2023

gsoldevila Jan 23, 2023

gsoldevila Jan 23, 2023

gsoldevila Jan 23, 2023 •

edited

Loading

gsoldevila Jan 23, 2023 •

edited

Loading

jloleysens left a comment

jloleysens Jan 23, 2023

gsoldevila Jan 23, 2023

gsoldevila commented Jan 24, 2023 •

edited

Loading

jloleysens left a comment

rudolf commented Jan 25, 2023

jloleysens left a comment

kibana-ci commented Feb 28, 2023

ESLint disabled line counts

Total ESLint disabled count

dokmic left a comment

		@@ -81,8 +79,6 @@ export { createIndex } from './create_index';

		export { checkTargetMappings } from './check_target_mappings';

		export { updateTargetMappingsMeta } from './update_target_mappings_meta';

Allow for additive mappings update without creating a new version index #149326

Allow for additive mappings update without creating a new version index #149326

Conversation

gsoldevila commented Jan 23, 2023 • edited Loading

elasticmachine commented Jan 23, 2023

gsoldevila Jan 23, 2023

Choose a reason for hiding this comment

gsoldevila Jan 23, 2023

Choose a reason for hiding this comment

gsoldevila Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

gsoldevila Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

jloleysens left a comment

Choose a reason for hiding this comment

jloleysens Jan 23, 2023

Choose a reason for hiding this comment

gsoldevila Jan 23, 2023

Choose a reason for hiding this comment

gsoldevila commented Jan 24, 2023 • edited Loading

jloleysens left a comment

Choose a reason for hiding this comment

rudolf commented Jan 25, 2023

jloleysens left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 28, 2023

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

dokmic left a comment

Choose a reason for hiding this comment

gsoldevila commented Jan 23, 2023 •

edited

Loading

gsoldevila Jan 23, 2023 •

edited

Loading

gsoldevila Jan 23, 2023 •

edited

Loading

gsoldevila commented Jan 24, 2023 •

edited

Loading