Implement the second part of the ZDT migration algorithm #153031

pgayvallet · 2023-03-09T16:10:42Z

Summary

Part of #150309
Follow-up of #152219

Implement the second part of the zero-downtime migration algorithm: the document conversion.

Schema

because a schema is worth a thousand words:

TODO / notepad

~~check that all types have model versions in INIT~~ will do later when we'll start have real types using MVs
Optimize to skip document migration when creating new index
documentsUpdateInit: extract remaining logic to utilities
outdatedDocumentsSearchRead: cleanup corrupted doc logic
outdatedDocumentsSearchTransform: cleanup corrupted doc logic
tests for /zdt/actions/wait_for_delay.ts ?
~~support for coreMigrationVersion~~ added as a follow-up in the parent issue
init -> equal -> check if aliasActions is empty

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

…-part-2

elasticmachine · 2023-03-23T11:47:28Z

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

jloleysens

Great job @pgayvallet ! This is looking really solid. I left a few comments, no blockers from my side, I am guessing more reviews are in flight 👀 .

One thought is that it would be nice to avoid deleting unknown documents because it should be safe to do leveraging model versions and on-read-migration. I understand maintaining parity with the previous algo is probably still safest but my gut feeling is that discarding data unless absolutely necessary should be avoided.

jloleysens · 2023-03-24T11:41:40Z

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

+  // couldn't find a way to infer the type of the state depending on the state of the handler
+  // even if they are directly coupled, so had to force-cast to this ugly any instead.
+  return stageHandler(current as any, response as any, context);


Yeah, that is really annoying. One idea, if you specifically want to avoid using any:

type AnyModelStageHandler = ( state: State, response: Either.Either<unknown, unknown>, ctx: MigratorContext ) => State;

then later

const stageHandler = modelStageMap[current.controlState] as AnyModelStageHandler;

Still uses type cast. Happy to stick with what you have + comment.

...cts-migration-server-internal/src/zdt/model/stages/cleanup_unknown_and_excluded_docs.test.ts

jloleysens · 2023-03-24T11:44:37Z

...-objects-migration-server-internal/src/zdt/model/stages/cleanup_unknown_and_excluded_docs.ts

❤️ these small files

jloleysens · 2023-03-24T14:28:17Z

...cts/core-saved-objects-migration-server-internal/src/zdt/model/stages/create_target_index.ts

@@ -48,10 +47,11 @@ export const createTargetIndex: ModelStage<'CREATE_TARGET_INDEX', 'UPDATE_ALIASE

  return {
    ...state,
-    controlState: 'UPDATE_ALIASES',
+    controlState: aliasActions.length ? 'UPDATE_ALIASES' : 'INDEX_STATE_UPDATE_DONE',


Forced optimisation :P

.../server/integration_tests/saved_objects/migrations/zero_downtime/conversion_failures.test.ts

jloleysens · 2023-03-24T15:09:14Z

...e/server/integration_tests/saved_objects/migrations/zero_downtime/rerun_same_version.test.ts

+    await runMigrations();
+  };
+
+  it('should perform a no-op upgrade', async () => {


To check my understanding: does the ZDT algo actually re-run queries to check for outdated documents?

It currently does. it's probably not necessary if we were to check the docVersions property, but I didn't want to optimize too soon (and checking for docs everytime is safest anyway)

afharo · 2023-03-24T18:11:54Z

ACK! I'll take a look on Monday

…-part-2

afharo

LGTM! Just added a few minor comments

afharo · 2023-03-27T18:05:54Z

packages/core/saved-objects/core-saved-objects-base-server-internal/src/mappings/types.ts

+  /**
+   * Indicates that the algorithm is currently converting the documents.
+   */
+  convertingDocuments: boolean;


++ to keeping it simple for now. In the future we might want to include a migration_stage field that claims what step it is. I think it might help with troubleshooting.

afharo · 2023-03-27T18:20:19Z

...s/core/saved-objects/core-saved-objects-migration-server-internal/src/common/redact_state.ts

+ */
+
+import { BulkOperationContainer } from '@elastic/elasticsearch/lib/api/types';
+import type { BulkOperation } from '../model/create_batches';


nit: importing non-common types from the common dir. We should try to avoid that 😇

Yeah... I'm planning on doing some 'move all the things' refactor, but I was planning on waiting until the algorithm is more polished before doing so

afharo · 2023-03-27T18:28:00Z

...-objects/core-saved-objects-migration-server-internal/src/zdt/actions/wait_for_delay.test.ts

+const nextTick = () => new Promise<void>((resolve) => resolve());
+const aFewTicks = () => nextTick().then(nextTick).then(nextTick);


hahaha! the .then sorting in NodeJS is interesting...

I think the following works and completes immediately because of the fake timers:

it('resolves after the specified amount of time', async () => { const handler = jest.fn(); const promise = waitForDelay({ delayInSec: 5 })().then(handler); expect(handler).not.toHaveBeenCalled(); jest.advanceTimersByTime(5000); await promise; expect(handler).toHaveBeenCalledTimes(1); });

…-part-2

pgayvallet · 2023-03-28T06:59:38Z

@elasticmachine merge upstream

pgayvallet · 2023-03-28T07:48:58Z

@elasticmachine merge upstream

rudolf · 2023-03-27T14:27:07Z

...cts/core-saved-objects-migration-server-internal/src/zdt/model/stages/create_target_index.ts

@@ -48,10 +47,11 @@ export const createTargetIndex: ModelStage<'CREATE_TARGET_INDEX', 'UPDATE_ALIASE

  return {
    ...state,
-    controlState: 'UPDATE_ALIASES',
+    controlState: aliasActions.length ? 'UPDATE_ALIASES' : 'INDEX_STATE_UPDATE_DONE',


if we create a new index then alias actions should always be empty?

But I wonder why we add aliases as a second step instead of creating the index with the aliases already set (createIndex accepts aliases param)?

rudolf · 2023-03-27T14:27:22Z

...core-saved-objects-migration-server-internal/src/zdt/model/stages/index_state_update_done.ts

+  if (state.newIndexCreation) {
+    return {
+      ...state,
+      controlState: 'DONE',
+    };
+  } else {


We have tsdocs on state.newIndexCreation but it would be useful to add (another) comment here to explain why we can skip to done

rudolf · 2023-03-28T10:02:03Z

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

+        meta: setMetaDocMigrationStarted({
+          meta: state.currentIndexMeta,
+        }),


This is sometimes awkward in the v2 migration model too :( it's not great that the previous stage needs to know how to prepare the state for the next stage. Almost feels like we want to have two callbacks for each stage in the model, one when entering that state and another when we get an action response. But even if we try this, it feels like the kind of thing we don't want to do inside another PR anyway.

The downside of adding it here is that there's business logic outside of the model and outside the actions making it harder to understand and test. I think we should either

add more test coverage to next.ts

Move this into a dedicated Actions.setDocMigrationStarted and test it there

I'd slightly lean towards (2) but this does also mean we're putting more business logic into actions them 🤷

...saved-objects/core-saved-objects-migration-server-internal/src/zdt/model/stages/init.test.ts

packages/core/saved-objects/core-saved-objects-migration-server-internal/src/zdt/state/types.ts

…-part-2

kibana-ci · 2023-03-28T18:39:45Z

💚 Build Succeeded

Buildkite Build
Commit: d44a692

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-base-server-internal`	56	52	-4

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	7	8	+1

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	76	72	-4

ESLint disabled line counts

id	before	after	diff
`@kbn/core`	7	8	+1
`securitySolution`	433	436	+3
total			+4

Total ESLint disabled count

id	before	after	diff
`@kbn/core`	8	9	+1
`securitySolution`	513	516	+3
total			+4

History

💔 Build #116119 failed d95d022
💔 Build #116069 failed fd41656
💔 Build #115908 failed 848bbd5a8a52a15a1b7516af05dfd91b2b9efd03
💔 Build #115900 failed c106456
💔 Build #115896 failed 599ec85

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

creating the states and stubing next and model

33a2ba1

pgayvallet mentioned this pull request Mar 9, 2023

Implement zero downtime migration algorithm #150309

Closed

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Migrations Epic:ZDTmigrations Zero downtime migrations labels Mar 9, 2023

pgayvallet and others added 5 commits March 11, 2023 13:14

work in progress

61c42f3

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

387d2bd

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

trying to remember where I was

5d49c69

wire doc transform

8e6d4a7

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

1646817

…-part-2

pgayvallet force-pushed the kbn-150309-zdt-algo-part-2 branch from e2d36fe to 1646817 Compare March 20, 2023 12:01

pgayvallet added 10 commits March 20, 2023 13:05

fix lint error

f245b9c

fix a few things

b3bfe52

some fixes on the workflow

68dade9

no refresh if no deleted docs

b776f66

fixing / adapting existing integration tests

1c1b74c

fix unit test

97eb83b

add more tests

fa76e2e

fix scenario with unknown types

e43e9c0

fix existing stages unit tests

a26c61d

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

2c69ff8

…-part-2

pgayvallet added release_note:skip Skip the PR/issue when compiling release notes v8.8.0 labels Mar 22, 2023

pgayvallet added 7 commits March 22, 2023 09:36

start adding new unit tests for stages

0fc0049

add remaining new unit tests for stages

dda8500

fix another unit test

be6fb44

enable state redaction for ZDT

e1edadc

more tests

98a56c8

last util tests

721efbe

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

9f7ef0e

…-part-2

jloleysens approved these changes Mar 24, 2023

View reviewed changes

pgayvallet added 5 commits March 27, 2023 15:37

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

3d0a514

…-part-2

update query

064b111

NITs

ecf654d

fix integration test version

3ed7d70

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

dcdcd4a

…-part-2

afharo approved these changes Mar 27, 2023

View reviewed changes

pgayvallet added 4 commits March 28, 2023 07:46

fix default version, again

b0a75c3

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

40a62a3

…-part-2

use import type

15ad0a4

use better types for stage handler in model

599ec85

Merge branch 'main' into kbn-150309-zdt-algo-part-2

c106456

rudolf reviewed Mar 28, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

359c64c

…-part-2

pgayvallet force-pushed the kbn-150309-zdt-algo-part-2 branch from 848bbd5 to 359c64c Compare March 28, 2023 13:13

pgayvallet added 5 commits March 28, 2023 15:18

use same transition format for stage test name

4623c8a

improve tsdoc

051cdb6

add comment

fd41656

add basic test coverage on next actionMap

daf6bc3

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

d95d022

…-part-2

rudolf approved these changes Mar 28, 2023

View reviewed changes

pgayvallet enabled auto-merge (squash) March 28, 2023 15:13

Merge remote-tracking branch 'upstream/main' into kbn-150309-zdt-algo…

d44a692

…-part-2

pgayvallet merged commit 3ff906d into elastic:main Mar 28, 2023

kibanamachine added the backport:skip This commit does not require backporting label Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the second part of the ZDT migration algorithm #153031

Implement the second part of the ZDT migration algorithm #153031

pgayvallet commented Mar 9, 2023 •

edited

Loading

elasticmachine commented Mar 23, 2023

jloleysens left a comment

jloleysens Mar 24, 2023

jloleysens Mar 24, 2023

jloleysens Mar 24, 2023

jloleysens Mar 24, 2023

pgayvallet Mar 27, 2023

afharo commented Mar 24, 2023

afharo left a comment

afharo Mar 27, 2023

afharo Mar 27, 2023

pgayvallet Mar 28, 2023

afharo Mar 27, 2023

pgayvallet commented Mar 28, 2023

pgayvallet commented Mar 28, 2023

rudolf Mar 27, 2023

rudolf Mar 27, 2023 •

edited

Loading

rudolf Mar 28, 2023 •

edited

Loading

kibana-ci commented Mar 28, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

		const nextTick = () => new Promise<void>((resolve) => resolve());
		const aFewTicks = () => nextTick().then(nextTick).then(nextTick);

Implement the second part of the ZDT migration algorithm #153031

Implement the second part of the ZDT migration algorithm #153031

Conversation

pgayvallet commented Mar 9, 2023 • edited Loading

Summary

Schema

TODO / notepad

elasticmachine commented Mar 23, 2023

jloleysens 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

afharo commented Mar 24, 2023

afharo 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

pgayvallet commented Mar 28, 2023

pgayvallet commented Mar 28, 2023

Choose a reason for hiding this comment

rudolf Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

rudolf Mar 28, 2023 • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Mar 28, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Public APIs missing exports

API count

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet commented Mar 9, 2023 •

edited

Loading

rudolf Mar 27, 2023 •

edited

Loading

rudolf Mar 28, 2023 •

edited

Loading