[ZDT migration] Don't run document migration on non-migrator nodes #156345

pgayvallet · 2023-05-02T10:21:48Z

Summary

Part of #150309
A few enhancements to the ZDT migration algorithm.

1. Run the 'expand' phase (and only this one) on non-migrator nodes

Given our latests changes to the way we want the algo to function, the non-migrator nodes will have to run the 'expand' (schema expansion) phase. However, the document migration phase will have to be run by the migrator node exclusively.

Note: because it was required for integration tests, a new migration.zdt.runOnNonMigratorNodes option was introduced to change this behavor and have non-migrator nodes ignore this limitation.

2. Don't terminate during `INIT` if higher mapping versions are found

Any mapping changes are upward compatible, meaning that we can safely no-op instead of failing of the mapping version check result is lesser. This change is required now that mapping updates will be performed before all nodes of the previous version are shut down (and is also required for rollbacks)

3. Perform a version check during `DOCUMENTS_UPDATE_INIT`

We were always executing the full doc update cycle when entering this stage. We're now performing a version check similar to what was done during INIT.

If the check result returns:

greater: we perform the document migration (as it was done before this change)
equal: we skip the document migration
lesser: we skip the document migration (NOTE: this may change later depending on how we handle rollbacks)
conflict: we terminate with a failure, as done during INIT

…fix'

…55981) ## Summary Adapt the `zdt` migration algorithm to run the v2 migrations in addition to the model version transformations. The intent is to be able to use the current migration system in a zero-downtime upgrade friendly-ish way ### Technicals elastic#148656 was a precondition, as the zdt algo requires that mapping changes are compatible (given we keep the same index). Technically, it means we're now storing the `virtualVersion` instead of the `modelVersion` in the index's meta (`mappingVersions` and `docVersions`), to allow keeping track of mixed stack and model versions per type. Note that switching to model versioning is still a one way, non-revertible action. Once using model versioning (by specifying `switchToModelVersionAt` on your type), there is no going back.

…gration-for-non-mig-nodes

…-fix'

…gration-for-non-mig-nodes

pgayvallet

Self review

pgayvallet · 2023-05-03T13:00:05Z

...ges/core/saved-objects/core-saved-objects-migration-server-internal/src/common/utils/logs.ts

+  if (logger.isLevelEnabled('debug')) {
+    logger.debug<StateTransitionLogMeta>(logMessage, {


Just a small improvement, since we implemented Logger.isLevelEnabled a while ago. Only log one message depending on the log level.

pgayvallet · 2023-05-03T13:02:19Z

src/core/server/integration_tests/saved_objects/migrations/jest_matchers.ts

+const toContainLogEntries: MatcherFunction<[entries: string[], options: { ordered?: boolean }]> = (
+  actual,
+  entries,
+  options = {}
+) => {


I finally added it!

pgayvallet · 2023-05-03T13:06:53Z

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

+    // app version is lower than the index mapping version.
+    // Should only occur in case of rollback.
+    // For now, we don't do anything.
+    case 'lesser':
+      return {
+        ...state,
+        logs,
+        controlState: 'DONE',
+      };


For now, we just perform a noop for downgrades and go straight to DONE. It may change in the future, depending on how we want to manage that, but it probably makes sense to just have a defined behavior for now, to be able to add tests around it.

pgayvallet · 2023-05-03T13:10:19Z

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

+  /**
+   * When true, will fully skip document migration, and will transition directly to DONE
+   * after the INDEX_STATE_UPDATE_DONE stage.
+   *
+   * This flag is set to `true` in the following scenarios:
+   * - on nodes without the `migrator` role, the flag will always be `true`.
+   * - if the migrator create the index, the workflow will set the flag to `true` given there is nothing to migrate.
+   */
+  readonly skipDocumentMigration: boolean;


As documented - this flag replaces newIndexCreation

pgayvallet · 2023-05-03T13:12:53Z

...d-objects/core-saved-objects-migration-server-internal/src/zdt/state/create_initial_state.ts

+  const runDocumentMigration =
+    context.nodeRoles.migrator || context.migrationConfig.zdt.runOnNonMigratorNodes;


As explained on the PR's description: this zdt.runOnNonMigratorNodes was added to force non-migrator nodes to run the document migration. The only usage for now is integration tests, but we may want to use this in single-node dev / test environments.

pgayvallet · 2023-05-03T13:15:16Z

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

    // app version is lower than the index mapping version.
-    // likely a rollback scenario - unsupported for the initial implementation
+    // either a rollback scenario, or an old node rebooting during the cohabitation period
+    // in that case, we simply no-op the expand phase.
    case 'lesser':
      return {
        ...state,
-        controlState: 'FATAL',
-        reason: 'Downgrading model version is currently unsupported',
-        logs,
+        controlState: 'INDEX_STATE_UPDATE_DONE',
+        ...commonState,


Change of behavior - encountering higher versions of the mappings in the index's meta now just skip the schema expansion phase

pgayvallet · 2023-05-03T13:16:24Z

src/core/server/integration_tests/saved_objects/migrations/zdt_1/basic_downgrade.test.ts

+describe('ZDT upgrades - basic downgrade', () => {
+  let esServer: TestElasticsearchUtils['es'];
+
+  const startElasticsearch = async () => {
+    const { startES } = createTestServers({
+      adjustTimeout: (t: number) => jest.setTimeout(t),
+      settings: {
+        es: {
+          license: 'basic',
+        },
+      },
+    });
+    return await startES();
+  };


Just a basic test asserting of the current behavior of the algo in case of 'downgrade' (booting with a lower version of the SO types)

…gration-for-non-mig-nodes

elasticmachine · 2023-05-03T13:17:06Z

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

gsoldevila · 2023-05-04T09:02:06Z

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

@@ -98,7 +99,7 @@ describe('migrationsStateActionMachine', () => {
      abort,
    });
    const logs = loggingSystemMock.collect(mockLogger);
-    const doneLog = logs.info.splice(8, 1)[0][0];
+    const doneLog = logs.info.splice(4, 1)[0][0];


NIT could you add a little comment describing what you're doing with this obscure line?

I'm doing nothing, just adapting someone else's hack

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

gsoldevila · 2023-05-04T09:22:57Z

src/core/server/integration_tests/saved_objects/migrations/zdt_1/basic_downgrade.test.ts

+describe('ZDT upgrades - basic downgrade', () => {
+  let esServer: TestElasticsearchUtils['es'];
+
+  const startElasticsearch = async () => {


NIT Since this function is very common, I made it available directly in the kibana_migrator_test_kit.ts (under the same name).

Oh, great. Wasn't aware of that and TBH I thought about extracting it in this PR. Will adapt accordingly

jloleysens

Overall lgtm. Left a q about node roles. Did not test locally but test coverage looks great!

jloleysens · 2023-05-04T09:14:25Z

...e/saved-objects/core-saved-objects-migration-server-internal/src/zdt/test_helpers/context.ts

    elasticsearchClient: elasticsearchClientMock.createElasticsearchClient(),
    maxRetryAttempts: 15,
    migrationDocLinks: docLinksServiceMock.createSetupContract().links.kibanaUpgradeSavedObjects,
    typeRegistry,
    serializer: serializerMock.create(),
    deletedTypes: ['deleted-type'],
    discardCorruptObjects: false,
+    nodeRoles: { migrator: true, ui: true, backgroundTasks: true },


Our current config validation prohibits migrator being combined with other roles. I'm guessing we are just doing all roles true in this mock for convenience?

Yeah, pretty much. If we want to be more consistent, I can adapt that to { migrator: true, ui: false, backgroundTasks: false }, it shouldn't cause any test to fail. Will try

gsoldevila · 2023-05-04T11:17:38Z

src/core/server/integration_tests/saved_objects/migrations/zdt_1/basic_downgrade.test.ts

+
+    // check that the mappingVersions are still v2
+    expect(mappingMeta.mappingVersions).toEqual({
+      sample_type: '10.2.0',


Ignorant question: How / where is this mappingVersions 10.2.0 defined?

Model version 2 = virtual version 10.2.0.

…gration-for-non-mig-nodes

gsoldevila

LGTM with few nits and comments!

gsoldevila · 2023-05-04T12:33:53Z

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

@@ -18,7 +18,7 @@ export const indexStateUpdateDone: ModelStage<
    throwBadResponse(state, res as never);
  }

-  if (state.newIndexCreation) {
+  if (state.skipDocumentMigration) {
    // we created the index, so we can safely skip the whole document migration


NIT outdated comment

// this migrator is not configured to update documents (or its index has just been created), skip the whole document migration

Would it make sense to move this condition inside the documentsUpdateInit()?

Otherwise, if in the future we have other paths that lead to the DOCUMENTS_UPDATE_INIT we'll have to add the if(state.skipDocumentMigration) condition in each of them. WDYT?

My reasoning was that INDEX_STATE_UPDATE_DONE was a mandatory step (as skipping the index update still leads to this stage), so we should be good keeping it there for now.

gsoldevila · 2023-05-04T13:01:24Z

...e/saved-objects/core-saved-objects-migration-server-internal/src/zdt/test_helpers/context.ts

-import { SavedObjectTypeRegistry } from '@kbn/core-saved-objects-base-server-internal';
+import {
+  SavedObjectTypeRegistry,
+  SavedObjectsMigrationConfigType,


NIT can be imported as a type

…gration-for-non-mig-nodes

kibana-ci · 2023-05-04T14:26:07Z

💚 Build Succeeded

Buildkite Build
Commit: 5ab4aaf

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`	87	88	+1

Unknown metric groups

API count

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

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	398	401	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	478	481	+3
total			+5

History

💚 Build #125477 succeeded a4ad083
💚 Build #125152 succeeded 2928b14
💛 Build #125050 was flaky 0772f9c
💔 Build #125030 failed 43620e3
💛 Build #124834 was flaky e6062f4
💔 Build #124803 failed c8af214

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

Start wiring the logic

9466974

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes Feature:Migrations v8.9.0 labels May 2, 2023

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

2a1cbbf

…fix'

pgayvallet force-pushed the kbn-xxx-skip-doc-migration-for-non-mig-nodes branch from 117af1f to fd844be Compare May 2, 2023 13:20

pgayvallet added 2 commits May 2, 2023 15:21

adapting usages

487bc49

pgayvallet force-pushed the kbn-xxx-skip-doc-migration-for-non-mig-nodes branch from fd844be to 487bc49 Compare May 2, 2023 13:21

pgayvallet and others added 13 commits May 2, 2023 15:22

Merge remote-tracking branch 'upstream/main' into kbn-xxx-skip-doc-mi…

c8af214

…gration-for-non-mig-nodes

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

9be1c91

…-fix'

add the runOnNonMigratorNodes migration option

e6062f4

don't fail when encountering higher versions in the mappings

e0c6634

use logger.isLevelEnabled

64f5ad9

update DOCUMENTS_UPDATE_INIT logic

5dc3307

Merge remote-tracking branch 'upstream/main' into kbn-xxx-skip-doc-mi…

aa03c85

…gration-for-non-mig-nodes

update test assertion

43620e3

add basic downgrade test

964fe52

fix snapshots

0772f9c

self review NITs

527db33

self review NITs

5e5c05f

self review NITs

c33f043

pgayvallet commented May 3, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into kbn-xxx-skip-doc-mi…

2928b14

…gration-for-non-mig-nodes

pgayvallet marked this pull request as ready for review May 3, 2023 13:17

pgayvallet requested a review from a team as a code owner May 3, 2023 13:17

gsoldevila reviewed May 4, 2023

View reviewed changes

...saved-objects-migration-server-internal/src/zdt/model/stages/index_state_update_done.test.ts Outdated Show resolved Hide resolved

gsoldevila reviewed May 4, 2023

View reviewed changes

jloleysens approved these changes May 4, 2023

View reviewed changes

gsoldevila reviewed May 4, 2023

View reviewed changes

pgayvallet added 2 commits May 4, 2023 13:19

Merge remote-tracking branch 'upstream/main' into kbn-xxx-skip-doc-mi…

2d8f513

…gration-for-non-mig-nodes

review comments

a4ad083

gsoldevila approved these changes May 4, 2023

View reviewed changes

pgayvallet added 2 commits May 4, 2023 15:24

import as type

dc95338

Merge remote-tracking branch 'upstream/main' into kbn-xxx-skip-doc-mi…

5ab4aaf

…gration-for-non-mig-nodes

pgayvallet enabled auto-merge (squash) May 4, 2023 13:24

pgayvallet merged commit 8f34b96 into elastic:main May 4, 2023

kibanamachine added the backport:skip This commit does not require backporting label May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZDT migration] Don't run document migration on non-migrator nodes #156345

[ZDT migration] Don't run document migration on non-migrator nodes #156345

pgayvallet commented May 2, 2023 •

edited

Loading

pgayvallet left a comment

pgayvallet May 3, 2023

pgayvallet May 3, 2023

pgayvallet May 3, 2023

pgayvallet May 3, 2023

pgayvallet May 3, 2023

pgayvallet May 3, 2023

pgayvallet May 3, 2023

elasticmachine commented May 3, 2023

gsoldevila May 4, 2023

pgayvallet May 4, 2023

gsoldevila May 4, 2023

pgayvallet May 4, 2023

jloleysens left a comment

jloleysens May 4, 2023

pgayvallet May 4, 2023

gsoldevila May 4, 2023

pgayvallet May 4, 2023

gsoldevila left a comment

gsoldevila May 4, 2023

gsoldevila May 4, 2023

pgayvallet May 4, 2023

gsoldevila May 4, 2023

kibana-ci commented May 4, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

		if (logger.isLevelEnabled('debug')) {
		logger.debug<StateTransitionLogMeta>(logMessage, {

		const runDocumentMigration =
		context.nodeRoles.migrator \|\| context.migrationConfig.zdt.runOnNonMigratorNodes;

[ZDT migration] Don't run document migration on non-migrator nodes #156345

[ZDT migration] Don't run document migration on non-migrator nodes #156345

Conversation

pgayvallet commented May 2, 2023 • edited Loading

Summary

1. Run the 'expand' phase (and only this one) on non-migrator nodes

2. Don't terminate during INIT if higher mapping versions are found

3. Perform a version check during DOCUMENTS_UPDATE_INIT

pgayvallet 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

elasticmachine commented May 3, 2023

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

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

gsoldevila 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

kibana-ci commented May 4, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

ESLint disabled line counts

Total ESLint disabled count

History

pgayvallet commented May 2, 2023 •

edited

Loading

2. Don't terminate during `INIT` if higher mapping versions are found

3. Perform a version check during `DOCUMENTS_UPDATE_INIT`