[SoMigV2] Fail fast if unknown document types are present in the source index #103341

pgayvallet · 2021-06-24T18:49:05Z

Summary

Fix #101351

Add a new CHECK_UNKNOWN_DOCUMENTS stage between WAIT_FOR_YELLOW_SOURCE and SET_SOURCE_WRITE_BLOCK to check for unknown documents (and fail if encountered) before starting the migration or enable the write block on the source index
Split the model.ts monolith into multiple files in its own module for maintainability.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…fail-unknown-docs

pgayvallet · 2021-06-28T06:37:52Z

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

+  return {
+    bool: {
+      must: unusedTypesQuery,
+      must_not: knownTypes.map((type) => ({
+        term: {
+          type,
+        },
+      })),
+    },
+  };


I included the unusedTypesQuery in the unknown doc queries, are we're not migrating them to the temp index, and as it would allow to 'remove' old types by unregistering them while adding them to the unused query.

Does it mean that users must remove them to continue the migration? If so, it might be considered as a breaking change even though we just fixed a bug.

No, I mean the opposite actually: I'm excluding the objects matching the unusedTypesQuery, to avoid failing if any is encountered given than we don't migrate them anyway.

ok, must usage is a bit misleading here because unusedTypesQuery contains another must_not inside.

Yea, the whole unusedTypesQuery naming is misleading because it's actually an excludeUnusedTypesQuery

pgayvallet · 2021-06-28T06:39:29Z

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

+export const createInitialState = ({
+  kibanaVersion,
+  targetMappings,
+  preMigrationScript,


Extracted this from model.ts to its own file for maintainability.

Only addition is the new typeRegistry parameter, and adding the knownTypes to the default/initial state.

This PR is still relatively small so I guess it's ok to split files here too.
For larger efforts, it might make it harder for reviewers to figure out what has changed vs what code was moved around.

pgayvallet · 2021-06-28T06:42:37Z

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

+    coreSetup.savedObjects.registerType({
+      name: 'foo',
+      hidden: false,
+      mappings: {
+        properties: {},
+      },
+      namespaceType: 'agnostic',


The test suite was triggering a failure by having some index-pattern docs registered with the foo type

{ type: 'foo', id: 'index-pattern:some-id' }

Adding this new unknown doc preflight check was causing the migration to fail too early, failing this test suite.

I'm now registering the foo type in the suite, to let it fails at the exact same step it was failing before that PR.

pgayvallet · 2021-06-28T06:44:05Z

...ver/saved_objects/migrationsv2/integration_tests/migration_7_13_0_transform_failures.test.ts

+    coreSetup.savedObjects.registerType({
+      name: 'space',
+      hidden: false,
+      mappings: {
+        properties: {},
+      },
+      namespaceType: 'single',
+      migrations: {
+        '7.14.0': (doc) => {
+          doc.attributes.foo.bar = 12;
+          return doc;
+        },


Same than for the cleanup suite. This new preflight check was causing the suite to fail too early. I'm now registering the space suite and registering a failing migration function instead.

Maybe we should update test fixtures instead? Spaces plugin SO are not relevant for this test anyway.

pgayvallet · 2021-06-28T06:45:15Z

...e/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts

+      const { body: response } = await client.indices.getSettings({ index: '.kibana_7.13.0_001' });
+      const settings = response['.kibana_7.13.0_001']
+        .settings as estypes.IndicesIndexStatePrefixedSettings;
+      expect(settings.index).not.toBeUndefined();
+      expect(settings.index!.blocks?.write).not.toEqual('true');


I manually modified the test locally to set the write block before these assertions to be sure that it was failing when the write block was present

pgayvallet · 2021-06-28T06:48:03Z

src/core/server/saved_objects/migrationsv2/model/extract_errors.test.ts

+    ).toMatchInlineSnapshot(`
+      "Migration failed because documents from unknown types were found. To proceed with the migration, please delete these documents from the \\".kibana_15\\" index.
+      The unknown documents were:
+      - \\"unknownType:12\\" (type: \\"unknownType\\")
+      - \\"anotherUnknownType:42\\" (type: \\"anotherUnknownType\\")
+      You can delete them using the following command:
+      curl -X POST \\"{elasticsearch}/.kibana_15/_bulk?pretty\\" -H 'Content-Type: application/json' -d'
+      { \\"delete\\" : { \\"_id\\" : \\"unknownType:12\\" } }
+      { \\"delete\\" : { \\"_id\\" : \\"anotherUnknownType:42\\" } }
+      '"
+    `);


Added curl command to remove the documents as requested in #101351 (comment)

Still not sure this is really helping as

we don't have the ES connection string in the migrator, so end user still have to manually replace it before executing the command

if security is enabled, user will also still have to add the proper auth headers for the query to succeed.

Do they have curl on windows ?

Wondering if we should really keep this @joshdover?

Personally, I think solving (1) may be worth the effort for clarity. I realize it would mean passing the config down several layers, but how feasible is this? Can be done as a follow up as well.

We should make this message a little more cautious though and recommend backing them up first (either with a snapshot or a curl bulk get)

pgayvallet · 2021-06-28T06:49:24Z

src/core/server/saved_objects/migrationsv2/model/extract_errors.ts

+export function extractTransformFailuresReason(
+  corruptDocumentIds: string[],
+  transformErrors: TransformErrorObjects[]
+): string {
+  const corruptDocumentIdReason =
+    corruptDocumentIds.length > 0
+      ? ` ${


Extracted from model.ts

pgayvallet · 2021-06-28T06:49:49Z

src/core/server/saved_objects/migrationsv2/model/helpers.ts

+/**
+ * A helper function/type for ensuring that all control state's are handled.
+ */
+export function throwBadControlState(p: never): never;
+export function throwBadControlState(controlState: any) {
+  throw new Error('Unexpected control state: ' + controlState);


All the file was extracted from model.ts

Same comment warning about doing too much in one PR but this is really great!

pgayvallet · 2021-06-28T06:50:46Z

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

+import * as Either from 'fp-ts/Either';
+import { RetryableEsClientError } from '../actions';
+
+export type ExcludeRetryableEsError<Response> = Exclude<


File extracted from model.ts

pgayvallet · 2021-06-28T06:51:14Z

src/core/server/saved_objects/migrationsv2/model/retry_state.ts

+export const delayRetryState = <S extends State>(
+  state: S,
+  errorMessage: string,
+  /** How many times to retry a step that fails */
+  maxRetryAttempts: number


File extracted from model.ts

Is there a test file for this helper?

No, there was no unit tests for the part of the code I extracted to this file. Can add some.

elasticmachine · 2021-06-28T06:53:49Z

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

…fail-unknown-docs

joshdover · 2021-06-28T16:44:32Z

...ver/saved_objects/migrationsv2/integration_tests/migration_7_13_0_transform_failures.test.ts

@@ -122,11 +140,11 @@ describe('migration v2', () => {
      }

      expect(errorMessage.includes('7 transformation errors were encountered:')).toBeTruthy();
-      expect(
-        errorMessage.includes(
-          'space:default: Error: Document "default" has property "space" which belongs to a more recent version of Kibana [6.6.0]. The last known version is [undefined]'


To be clear, this case is no longer possible in v2 migrations now, correct?

Correct, The last known version is [undefined] scenario is no longer possible during the migration as it would now fail before trying to transform the documents

joshdover · 2021-06-28T16:47:23Z

...e/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts

+          license: 'basic',
+          // dataset contains 2 type of unknown docs
+          // `foo` documents
+          // `space` documents (to mimic a migration with disabled plugins)


Trying to make sure I understand why we don't need to set xpack.spaces.enabled: false. Is this because createTestServers only loads OSS plugins by default?

Yea, it seems the test servers are only loading OSS by default (which made sense before the license changes)

FWIW, I had to double check that again against what's included under the default distribution. Maybe we should start referring to SSPL rather than OSS?

joshdover · 2021-06-28T16:48:47Z

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

+  unknownDocs: CheckForUnknownDocsResponseDoc[];
+}
+
+export const checkForUnknownDocs = ({


Should this file get an explicit unit test?

TinaHeiligers · 2021-06-28T22:53:16Z

...ver/saved_objects/migrationsv2/integration_tests/migration_7_13_0_transform_failures.test.ts

-          'space:default: Error: Document "default" has property "space" which belongs to a more recent version of Kibana [6.6.0]. The last known version is [undefined]'
+      expect(errorMessage).toEqual(
+        expect.stringContaining(
+          'space:sixth: Error: Migration function for version 7.14.0 threw an error'


I know we're only asserting agains the error message here but it would be great if we could also assert that the original stack trace from the failed transform is available too.

We're already doing that in a dedicated test suite (/type_migration_failure.test.ts), and such assertions are quite tedious, which is why I thought it wasn't necessary to do it here too.

kibana/src/core/server/saved_objects/migrationsv2/integration_tests/type_migration_failure.test.ts

Lines 133 to 136 in 0561648

{

mode: 'equal',

value: 'Error: "number" attribute should be present',

},

TinaHeiligers · 2021-06-28T23:06:06Z

...e/server/saved_objects/migrationsv2/integration_tests/migration_7_13_0_unknown_types.test.ts

+      expect(
+        errorMessage.startsWith(
+          'Unable to complete saved object migrations for the [.kibana] index: Migration failed because documents ' +
+            'from unknown types were found. To proceed with the migration, please delete these documents from the ' +


Out of context of saved objects, if a user sees this message, they might not know what the 'type' is here. Maybe we can improve the message string to make it clear what the 'type' refers to?

TinaHeiligers · 2021-06-28T23:12:37Z

src/core/server/saved_objects/migrationsv2/model/extract_errors.ts

+  sourceIndex: string
+): string {
+  return (
+    `Migration failed because documents from unknown types were found. ` +


I think we can be clearer with the failure message here. e.g.

Suggested change

`Migration failed because documents from unknown types were found. ` +

`Migration failed because documents were found for unknown saved object types. ` +

If we want to be really verbose, we could hint at the cause too:

The plugin or feature that was used to create these documents is no longer available.

That might be taking it a bit far though. We could add a possible reason for these unknown type errors to the documentation instead but I'm concerned about users who might already too frustrated to 'go and read the docs'? WDYT?

The plugin or feature that was used to create these documents is no longer available.

We can't really know for sure why and how the type is no longer registered, which is why I think we should avoid taking such guesses in the outputted logs?

e.g

The registering plugin could just be disabled

The document could be corrupted and just not have any type at all

???

It could probably be summarized as: the plugin that created this document is either missing or disabled. Any other case is likely a bug (either in our plugin code or a 3rd party plugin). We need to have an explicit API for registering types that can be safely deleted.

src/core/server/saved_objects/migrationsv2/model/extract_errors.ts

TinaHeiligers · 2021-06-28T23:28:48Z

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

      };
    } else {
      return throwBadResponse(stateP, res);
    }
+  } else if (stateP.controlState === 'CHECK_UNKNOWN_DOCUMENTS') {
+    const res = resW as ExcludeRetryableEsError<ResponseType<typeof stateP.controlState>>;
+    if (Either.isRight(res)) {


I'm torn here between returning a Either.Right or Either.Left for returning something that will ultimately cause the migrations to fail. Generally, the 'happy path' for migrations is indicated by an Either.Right res but, in this case, our success result is when we have found documents with unknown types.

I guess it's not critical to the implementation, it just feels inconsistent with what I'd have expected, especially since we're transitioning directly to the FATAL state from here if docs are found. See for example how we handle the 'ultimately will fail migrations' response for the OUTDATED_DOCUMENTS_TRANSFORM step

Yea, you're right, using left when unknown doc types are found makes sense. Will adapt.

TinaHeiligers

Overall implementation looks ok. I've made some suggestions and recommendations for improvement. If needed, those can be handled in a follow up.

…fail-unknown-docs

joshdover

LGTM

TinaHeiligers

LGTM!

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

mshustov · 2021-06-29T13:34:54Z

...ver/saved_objects/migrationsv2/integration_tests/migration_7_13_0_transform_failures.test.ts

+    coreSetup.savedObjects.registerType({
+      name: 'space',
+      hidden: false,
+      mappings: {
+        properties: {},
+      },
+      namespaceType: 'single',
+      migrations: {
+        '7.14.0': (doc) => {
+          doc.attributes.foo.bar = 12;
+          return doc;
+        },


Maybe we should update test fixtures instead? Spaces plugin SO are not relevant for this test anyway.

mshustov · 2021-06-29T13:45:44Z

src/core/server/saved_objects/migrationsv2/model/retry_state.ts

+export const delayRetryState = <S extends State>(
+  state: S,
+  errorMessage: string,
+  /** How many times to retry a step that fails */
+  maxRetryAttempts: number


Is there a test file for this helper?

mshustov · 2021-06-29T13:49:49Z

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

+  return {
+    bool: {
+      must: unusedTypesQuery,
+      must_not: knownTypes.map((type) => ({
+        term: {
+          type,
+        },
+      })),
+    },
+  };


Does it mean that users must remove them to continue the migration? If so, it might be considered as a breaking change even though we just fixed a bug.

mshustov · 2021-06-29T13:50:54Z

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

+        };
+      } else {
+        return throwBadResponse(stateP, res.left);
+      }
    }
  } else if (stateP.controlState === 'SET_SOURCE_WRITE_BLOCK') {


are you going to clean up the model from the unnecessary steps and update RFC in a follow-up PR?

are you going to clean up the model from the unnecessary steps

The PR did not 'deprecate' any step?

and update the RFC

Forgot about that, will do in current PR.

The PR did not 'deprecate' any step?

Do we need to keep OUTDATED_DOCUMENTS_* steps with the logic introduces in the current PR?

I don't think we do, but still not 100% sure tbh. Also, this PR did not really change anything functional in the v2 algorithm, it just fails earlier.

Will open a specific issue to discuss about the OUTDATED_DOCUMENTS_* steps removal

Created #103731

kibanamachine · 2021-06-29T17:19:48Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #134966 succeeded 8011281
💚 Build #134461 succeeded 26c36e9
💚 Build #134333 succeeded 1fdbf28
💔 Build #134329 failed 270f9b4
💔 Build #134202 failed 927f8ec

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

…ce index (elastic#103341) * initial draft * fix some tests * fix additional unit tests * move all the things * create error generation fn * add correct error message * add unknown types to log message * fix types * fix existing test suites * add IT test * review comments * add tests + use unknown instead of undefined for empty types * update RFC with new step # Conflicts: # rfcs/text/0013_saved_object_migrations.md

…ce index (#103341) (#103733) * initial draft * fix some tests * fix additional unit tests * move all the things * create error generation fn * add correct error message * add unknown types to log message * fix types * fix existing test suites * add IT test * review comments * add tests + use unknown instead of undefined for empty types * update RFC with new step # Conflicts: # rfcs/text/0013_saved_object_migrations.md

pgayvallet added 11 commits June 24, 2021 20:43

initial draft

927f8ec

fix some tests

cb5559c

fix additional unit tests

fb89a59

move all the things

fbb7959

create error generation fn

d41fec4

add correct error message

19bcb03

Merge remote-tracking branch 'upstream/master' into kbn-101351-somig-…

c367717

…fail-unknown-docs

add unknown types to log message

270f9b4

fix types

81de68a

fix existing test suites

f566e17

add IT test

1fdbf28

pgayvallet commented Jun 28, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review June 28, 2021 06:53

pgayvallet requested a review from a team as a code owner June 28, 2021 06:53

Merge remote-tracking branch 'upstream/master' into kbn-101351-somig-…

26c36e9

…fail-unknown-docs

joshdover self-requested a review June 28, 2021 10:29

joshdover reviewed Jun 28, 2021

View reviewed changes

TinaHeiligers reviewed Jun 28, 2021

View reviewed changes

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

TinaHeiligers reviewed Jun 28, 2021

View reviewed changes

pgayvallet added 2 commits June 29, 2021 07:46

Merge remote-tracking branch 'upstream/master' into kbn-101351-somig-…

b9375f9

…fail-unknown-docs

review comments

8011281

pgayvallet requested a review from joshdover June 29, 2021 10:28

joshdover approved these changes Jun 29, 2021

View reviewed changes

TinaHeiligers self-requested a review June 29, 2021 13:49

TinaHeiligers approved these changes Jun 29, 2021

View reviewed changes

mshustov reviewed Jun 29, 2021

View reviewed changes

pgayvallet added 2 commits June 29, 2021 17:10

add tests + use unknown instead of undefined for empty types

2095a60

update RFC with new step

774d58a

pgayvallet added v7.14.0 and removed v7.15.0 labels Jun 29, 2021

pgayvallet merged commit 5d24b23 into elastic:master Jun 29, 2021

pgayvallet mentioned this pull request Jun 29, 2021

[7.x] [SoMigV2] Fail fast if unknown document types are present in the source index (#103341) #103733

Merged

pgayvallet mentioned this pull request Jul 2, 2021

[SO Migration] improve the overall log output #104245

Closed

joshdover mentioned this pull request Jul 7, 2021

Filter out documents of unknown types during migrations #104690

Closed

pgayvallet added v7.15.0 v8.0.0 labels Jul 7, 2021

	{
	mode: 'equal',
	value: 'Error: "number" attribute should be present',
	},

	`Migration failed because documents from unknown types were found. ` +
	`Migration failed because documents were found for unknown saved object types. ` +

[SoMigV2] Fail fast if unknown document types are present in the source index #103341

[SoMigV2] Fail fast if unknown document types are present in the source index #103341

Conversation

pgayvallet commented Jun 24, 2021 • edited Loading

Summary

Checklist

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

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

TinaHeiligers Jun 28, 2021 • edited Loading

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 Jun 28, 2021

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

TinaHeiligers Jun 28, 2021 • edited Loading

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

TinaHeiligers Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

pgayvallet Jun 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment • edited Loading

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

TinaHeiligers 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

Choose a reason for hiding this comment

kibanamachine commented Jun 29, 2021

💚 Build Succeeded

Metrics [docs]

History

pgayvallet commented Jun 24, 2021 •

edited

Loading

TinaHeiligers Jun 28, 2021 •

edited

Loading

TinaHeiligers Jun 28, 2021 •

edited

Loading

TinaHeiligers Jun 28, 2021 •

edited

Loading

pgayvallet Jun 29, 2021 •

edited

Loading

TinaHeiligers Jun 28, 2021 •

edited

Loading

TinaHeiligers left a comment •

edited

Loading