Rewriting SO id during migration by mshustov · Pull Request #97222 · elastic/kibana

mshustov · 2021-04-15T10:20:53Z

Summary

Implementation is based on algorithm changes described in #93002
TLDR: Kibana doesn't reindex SO in ES, but instead loads them from Elasticsearch server, transforms, rewrites id for SO registered with namespace: multiple and namespace: multiple-isolated, writes SO back to Elasticsearch server.

Performance concerns.

Tested on the migration of 100 000 SavedObjects https://github.com/elastic/kibana/blob/6712bc07279c9582094d31eb22ebf72bdfaba2f7/src/core/server/saved_objects/migrationsv2/integration_tests/migration_7.7.2_xpack_100k.test.ts

Performance seems to be better in the worst-case scenario when we have a lot of outdated documents, as readWithPit seems to be more performant than searchForOutdatedDocuments. See logs:

Master

batchSize 1_000: 154sec
migration_test_kibana.log

batchSize 10_000: 104sec

Branch

batchSize 1_000: 97sec
migration_test_kibana.log
batchSize 10_000: 105sec

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

required to enforce idempotent id generation for SO

mshustov · 2021-04-15T11:37:41Z

src/core/server/saved_objects/migrations/kibana/kibana_migrator.test.ts

        jest.clearAllMocks();
      });

-      it('creates a V2 migrator that initializes a new index and migrates an existing index', async () => {


the implementation is tested in the integration tests

mshustov · 2021-04-15T11:38:45Z

src/core/server/saved_objects/migrationsv2/actions/index.ts

+  transformRawDocs: TransformRawDocs,
+  outdatedDocuments: SavedObjectsRawDoc[],
+  index: string,
+  refresh: estypes.Refresh


I'd move to named arguments instead, but let's do it in a separate PR for all the actions at once

src/core/server/saved_objects/migrationsv2/types.ts

rudolf

Did a quick first pass and the implementation looks very solid!

src/core/server/saved_objects/migrationsv2/actions/index.ts

src/core/server/saved_objects/migrationsv2/migrations_state_action_machine.ts

src/core/server/saved_objects/migrationsv2/next.ts

mshustov · 2021-04-20T09:48:47Z

src/core/server/saved_objects/migrations/core/elastic_index.ts

 import _ from 'lodash';
 import { estypes } from '@elastic/elasticsearch';
 import { MigrationEsClient } from './migration_es_client';
-import { CountResponse, SearchResponse } from '../../../elasticsearch';


no changes in the file, a simple cleanup

src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts

mshustov · 2021-04-20T09:53:04Z

src/core/server/saved_objects/migrationsv2/integration_tests/rewriting_id.test.ts

+    await new Promise((resolve) => setTimeout(resolve, 10000));
+  });
+
+  it('rewrites id deterministically for SO with namespaceType: "multiple" and "multiple-isolated"', async () => {


Inspired by https://github.com/elastic/kibana/blob/238791b9421cda571011fd416ae0ffd090b1e94d/test/api_integration/apis/saved_objects/migrations.ts

mshustov · 2021-04-20T10:00:49Z

test/functional/fixtures/es_archiver/visualize/data.json

        "title": "test_index*"
      },
-      "type": "test_index*"
+      "type": "index-pattern"


invalid type that used to fail migration.

…-id-gen

…st test file during cleanup phase

… the first test file during cleanup phase" This reverts commit a128d7b.

jportner

Sorry 😅

src/core/server/saved_objects/migrations/core/document_migrator.ts

This reverts commit 97315b6.

mshustov · 2021-04-26T07:14:27Z

@jportner reverted, I will skip the test on 7.x branch then

jportner

👍

kibanamachine · 2021-04-26T13:34:10Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #122232 failed c6ff34b
💚 Build #122205 succeeded 97315b6
💔 Build #122203 failed 44f7974
💔 Build #122202 failed a128d7b
💚 Build #122023 succeeded 8d17ce0

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

* some typos * implement an alternative client-side migration algorithm required to enforce idempotent id generation for SO * update tests * lol * remove unnecessary param from request generic * remove unused parameter * optimize search when quierying SO for migration * fix wrong type in fixtures * try shard_doc asc * add an integration test * cleanup * track_total_hits: false to improve perf * add happy path test for transformDocs action * remove unused types * fix wrong typing * add cleanup phase * add an integration test for cleanup phase * add unit-tests for cleanup function * address comments * Fix functional test * set defaultIndex before each test. otherwise it is deleted in the first test file during cleanup phase * sourceIndex: Option.some<> for consistency * Revert "set defaultIndex before each test. otherwise it is deleted in the first test file during cleanup phase" This reverts commit a128d7b. * address comments from Pierre * fix test * Revert "fix test" This reverts commit 97315b6. * revert min convert version back to 8.0 Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>

* Rewriting SO id during migration (#97222) * some typos * implement an alternative client-side migration algorithm required to enforce idempotent id generation for SO * update tests * lol * remove unnecessary param from request generic * remove unused parameter * optimize search when quierying SO for migration * fix wrong type in fixtures * try shard_doc asc * add an integration test * cleanup * track_total_hits: false to improve perf * add happy path test for transformDocs action * remove unused types * fix wrong typing * add cleanup phase * add an integration test for cleanup phase * add unit-tests for cleanup function * address comments * Fix functional test * set defaultIndex before each test. otherwise it is deleted in the first test file during cleanup phase * sourceIndex: Option.some<> for consistency * Revert "set defaultIndex before each test. otherwise it is deleted in the first test file during cleanup phase" This reverts commit a128d7b. * address comments from Pierre * fix test * Revert "fix test" This reverts commit 97315b6. * revert min convert version back to 8.0 Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co> * skip id rewriting test Co-authored-by: Matthias Wilhelm <matthias.wilhelm@elastic.co>

mshustov added 6 commits April 13, 2021 09:42

some typos

19b94c8

implement an alternative client-side migration algorithm

59a3fed

required to enforce idempotent id generation for SO

update tests

8c54a03

lol

2cf6d41

remove unnecessary param from request generic

4afb285

remove unused parameter

6841668

mshustov added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Apr 15, 2021

Merge branch 'master' into migration-id-gen

f988f23

mshustov commented Apr 15, 2021

View reviewed changes

src/core/server/saved_objects/migrationsv2/types.ts Outdated Show resolved Hide resolved

rudolf reviewed Apr 15, 2021

View reviewed changes

src/core/server/saved_objects/migrationsv2/actions/index.ts Outdated Show resolved Hide resolved

src/core/server/saved_objects/migrationsv2/migrations_state_action_machine.ts Show resolved Hide resolved

src/core/server/saved_objects/migrationsv2/next.ts Show resolved Hide resolved

mshustov added 4 commits April 16, 2021 10:59

Merge branch 'master' into migration-id-gen

10fdb61

optimize search when quierying SO for migration

d3a2dd1

fix wrong type in fixtures

be9438e

try shard_doc asc

4ebf73b

mshustov mentioned this pull request Apr 16, 2021

Refactor SO tests to reduce flakiness #97374

Merged

mshustov added 6 commits April 16, 2021 20:34

Merge branch 'master' into migration-id-gen

8983403

Merge branch 'master' into migration-id-gen

fdd18e4

add an integration test

e40c824

cleanup

8c03eaf

Merge branch 'master' into migration-id-gen

dfdb8e8

Merge branch 'master' into migration-id-gen

61ef354

mshustov commented Apr 20, 2021

View reviewed changes

src/core/server/saved_objects/migrations/core/migrate_raw_docs.ts Show resolved Hide resolved

mshustov commented Apr 20, 2021

View reviewed changes

Merge branch 'master' into migration-id-gen

2fa72b9

mshustov commented Apr 20, 2021

View reviewed changes

mshustov mentioned this pull request Apr 23, 2021

Update dependency @elastic/elasticsearch to v7.13.0-canary.4 (7.x) #97926

Closed

1 task

mshustov added 7 commits April 25, 2021 12:16

Merge remote-tracking branch 'origin/migration-id-gen' into migration…

abe3fff

…-id-gen

Merge branch 'master' into migration-id-gen

1332961

set defaultIndex before each test. otherwise it is deleted in the fir…

a128d7b

…st test file during cleanup phase

sourceIndex: Option.some<> for consistency

585bdd9

Revert "set defaultIndex before each test. otherwise it is deleted in…

145244a

… the first test file during cleanup phase" This reverts commit a128d7b.

address comments from Pierre

44f7974

fix test

97315b6

jportner suggested changes Apr 25, 2021

View reviewed changes

src/core/server/saved_objects/migrations/core/document_migrator.ts Outdated Show resolved Hide resolved

mshustov added 2 commits April 26, 2021 09:05

Revert "fix test"

f7b4d7d

This reverts commit 97315b6.

revert min convert version back to 8.0

c6ff34b

mshustov requested a review from jportner April 26, 2021 07:14

Merge branch 'master' into migration-id-gen

94ea2cd

jportner approved these changes Apr 26, 2021

View reviewed changes

mshustov merged commit e6ba8cc into elastic:master Apr 26, 2021

mshustov deleted the migration-id-gen branch April 26, 2021 13:57

mshustov mentioned this pull request Apr 26, 2021

[7.x] Rewriting SO id during migration (#97222) #98300

Merged

mshustov mentioned this pull request Apr 26, 2021

Provide API to inspect FTR state #98311

Open

This was referenced Apr 29, 2021

v2 migrations needs to support re-writing document _id's to support saved-objects in multiple spaces #86247

Closed

Use PiT for outdated document search #98797

Merged

mshustov mentioned this pull request May 26, 2021

Add an explicit CLEANUP step to migrations v2 #100650

Closed

This was referenced Jun 29, 2021

[SO Migration v2] Remove the OUTDATED_DOCUMENTS_* steps #103731

Closed

[SO Migration] increasing performance by getting rid of the temporary index #104080

Closed

gsoldevila mentioned this pull request Apr 14, 2022

Investigate the need for 10 seconds delays after multiple saved_objects tests #130240

Closed

rudolf mentioned this pull request Jul 13, 2022

migrations Actions.readWithPit must use allow_partial_search_results=false #136271

Closed

Comments

Conversation

mshustov commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Performance concerns.

Master

Branch

Checklist

Uh oh!

mshustov Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

mshustov Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rudolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mshustov Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshustov Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

mshustov Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mshustov commented Apr 26, 2021

Uh oh!

jportner left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 26, 2021

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mshustov commented Apr 15, 2021 •

edited

Loading