[Migrations] Only pickup updated SO types when performing a compatible migration #159962

gsoldevila · 2023-06-19T16:36:04Z

Summary

Tackles the first improvement described in #160038.

When "picking up" the updated mappings, we add a "query" in order to select and update only the SO types that have been updated, compared to the previous version.

We achieve this by comparing migrationMappingPropertyHashes; we compare the hashes stored in the <soIndex>.mapping._meta.migrationMappingPropertyHashes against the ones calculated from the definitions from the typeRegistry.

…fix'

elasticmachine · 2023-06-20T16:01:35Z

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

gsoldevila · 2023-06-20T16:01:56Z

@pgayvallet I believe it makes sense to add this enhancement to the ZDT as well, right?

rudolf

Left some nits, but biggest question is around getUpdatedHashes(...) when actual doesn't have a _meta

rudolf · 2023-06-22T11:15:23Z

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

@@ -35,7 +36,8 @@ export const pickupUpdatedMappings =
  (
    client: ElasticsearchClient,
    index: string,
-    batchSize: number
+    batchSize: number,


nit on line 23 the comment says:

Pickup updated mappings by performing an update by query operation on all documents in the index. Returns a task ID which can be tracked for progress.

maybe we can update that to be "on all documents matching the passed in query"

rudolf · 2023-06-23T11:54:58Z

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

+                ...stateP.logs,
+                {
+                  level: 'info',
+                  message: `Kibana is performing a compatible update and some root fields have been updated. All SO documents must be updated. Updated root fields: ${nonTypeFields}.`,


Suggested change

message: `Kibana is performing a compatible update and some root fields have been updated. All SO documents must be updated. Updated root fields: ${nonTypeFields}.`,

message: `Kibana is performing a compatible upgrade and the mappings of some root fields have been changed. For Elasticsearch to pickup these mappings, all saved objects need to be updated. Updated root fields: ${nonTypeFields}.`,

Thought it's useful to explain the "pickup updated mappings" part similar to what you did in the log message below.

rudolf · 2023-06-23T12:08:35Z

...-objects/core-saved-objects-migration-server-internal/src/core/build_active_mappings.test.ts

+      properties: {},
+    };
+
+    expect(getUpdatedHashes(actual, expected)).toBeUndefined();


I'm unsure about this. Probably an edge case but if an index doesn't have _meta can we safely conclude that all mappings have been picked up? Feels safer to assume all types' mappings have changed and need pickup?

Having getUpdatedHashes(...): string[] | undefined is also a slightly awkward API where undefined means something like "I don't know"...

Actually, when this method returns undefined we don't pass in a query, so we already update all types.
But you've got a point, this can be confusing.
I'll update the logic to not rely on this undefined.

rudolf · 2023-06-23T12:13:20Z

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

+          // some top-level properties have changed, e.g. 'dynamic' or '_meta' (see diffMappings())
+          return {
+            ...stateP,
+            controlState: 'UPDATE_TARGET_MAPPINGS_PROPERTIES',


If when actual._meta === undefined we say "all types have changed" instead of undefined (see my other comment) then we could only end up here if we change dynamic right? I'm wondering a bit if this scenario could ever happen in the wild?

I wasn't sure about the dynamic case, so I played it safe and updated everything in that case.

After some investigation, it seems that for target indices we have dynamic: 'strict' and for temporary (and cloned) indices we have dynamic: false.

So if a migrator fails before the UPDATE_TARGET_MAPPINGS_PROPERTIES step, the corresponding index would still have the dynamic: false. However it would be missing the hashes and the indexTypesMap as well, which already trigger a "full" pickup of documents.

TLDR: It can happen and we must pickup all documents in that case.

I improved the code with this commit, I think it is much more clear now.

rudolf

Left a nit on a comment that confused me, but implementation looks good 👍

rudolf · 2023-06-30T13:13:32Z

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

+            ],
+          };
+        } else {
+          // compatible migration, all updated hashes correspond to SO types


all updated hashes correspond to SO types

This sounds a lot to me like "The md5 of ALL mappings match, so there's no need to update target mappings" should this maybe be "some hashes are different because of changes in saved object type's mappings"

kibana-ci · 2023-06-30T14:40:41Z

💚 Build Succeeded

Buildkite Build
Commit: 7f69a58

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-migration-server-internal`	89	90	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-migration-server-internal`	123	124	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	413	417	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	15	17	+2
`securitySolution`	492	496	+4
total			+6

History

💚 Build #138906 succeeded 2e35927
💔 Build #138815 failed 60f3ae7
💚 Build #136828 succeeded 1ef9bab
💔 Build #136643 failed 84bd400
💔 Build #136630 failed a751935
💔 Build #136604 failed d870ffc

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

…e migration (elastic#159962) ## Summary Tackles the first improvement described in elastic#160038. When "picking up" the updated mappings, we add a "query" in order to select and update only the SO types that have been updated, compared to the previous version. We achieve this by comparing `migrationMappingPropertyHashes`; we compare the hashes stored in the `<soIndex>.mapping._meta.migrationMappingPropertyHashes` against the ones calculated from the definitions from the `typeRegistry`. --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 6f87e1d)

kibanamachine · 2023-06-30T14:46:53Z

💚 All backports created successfully

Status	Branch	Result
✅	8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…patible migration (#159962) (#161010) # Backport This will backport the following commits from `main` to `8.9`: - [[Migrations] Only pickup updated SO types when performing a compatible migration (#159962)](#159962)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Gerard Soldevila <[email protected]>

Part of #161067 Same idea as #159962, but applied to ZDT. When "picking up" the updated mappings, we add a "query" in order to select and update only the SO types that have been updated, compared to the previous version.

Pickup updated SO types only when performing a compatible migration

718b310

gsoldevila and others added 5 commits June 20, 2023 17:02

Do not filter if root fields have been updated

7d956e1

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

d870ffc

…fix'

Merge branch 'main' into kbn-pickup-updated-mappings-only

281f4c9

Use indexTypesMap to determine what fields are SO types

a751935

Remove deprecated dependency, add extra integration test

84bd400

gsoldevila marked this pull request as ready for review June 20, 2023 16:01

gsoldevila requested a review from a team as a code owner June 20, 2023 16:01

Update outdated test

1ef9bab

rudolf reviewed Jun 23, 2023

View reviewed changes

gsoldevila added 2 commits June 28, 2023 14:35

Address PR feedback

e51f28b

Merge branch 'main' into kbn-pickup-updated-mappings-only

60f3ae7

gsoldevila requested a review from rudolf June 28, 2023 12:36

Fix outdated test

2e35927

rudolf approved these changes Jun 30, 2023

View reviewed changes

Address PR nits

df9c880

gsoldevila added v8.10.0 and removed v8.8.2 labels Jun 30, 2023

Merge branch 'main' into kbn-pickup-updated-mappings-only

7f69a58

gsoldevila enabled auto-merge (squash) June 30, 2023 13:27

gsoldevila merged commit 6f87e1d into elastic:main Jun 30, 2023

kibanamachine mentioned this pull request Jun 30, 2023

[8.9] [Migrations] Only pickup updated SO types when performing a compatible migration (#159962) #161010

Merged

gsoldevila mentioned this pull request Jul 3, 2023

[ZDT] Pickup updated types only #161123

Merged

gsoldevila mentioned this pull request Jul 10, 2023

[Migrations] Enhance performance of the UPDATE_TARGET_MAPPINGS_PROPERTIES step #160038

Closed

rudolf mentioned this pull request Sep 23, 2023

Scale upgrade migrations to millions of saved objects #144035

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Migrations] Only pickup updated SO types when performing a compatible migration #159962

[Migrations] Only pickup updated SO types when performing a compatible migration #159962

gsoldevila commented Jun 19, 2023 •

edited by kibanamachine

Loading

elasticmachine commented Jun 20, 2023

gsoldevila commented Jun 20, 2023

rudolf left a comment

rudolf Jun 22, 2023

rudolf Jun 23, 2023

rudolf Jun 23, 2023

gsoldevila Jun 23, 2023

rudolf Jun 23, 2023

gsoldevila Jun 23, 2023

gsoldevila Jun 28, 2023

gsoldevila Jun 28, 2023 •

edited

Loading

rudolf left a comment

rudolf Jun 30, 2023

kibana-ci commented Jun 30, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Jun 30, 2023

	message: `Kibana is performing a compatible update and some root fields have been updated. All SO documents must be updated. Updated root fields: ${nonTypeFields}.`,
	message: `Kibana is performing a compatible upgrade and the mappings of some root fields have been changed. For Elasticsearch to pickup these mappings, all saved objects need to be updated. Updated root fields: ${nonTypeFields}.`,

[Migrations] Only pickup updated SO types when performing a compatible migration #159962

[Migrations] Only pickup updated SO types when performing a compatible migration #159962

Conversation

gsoldevila commented Jun 19, 2023 • edited by kibanamachine Loading

Summary

elasticmachine commented Jun 20, 2023

gsoldevila commented Jun 20, 2023

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsoldevila Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Jun 30, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Jun 30, 2023

💚 All backports created successfully

Questions ?

gsoldevila commented Jun 19, 2023 •

edited by kibanamachine

Loading

gsoldevila Jun 28, 2023 •

edited

Loading