[Migrations] Only pickup updated SO types when performing a compatibl…

…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]>
kibanamachine · Jun 30, 2023 · 6f87e1d · 6f87e1d
1 parent cca2be4
commit 6f87e1d
Show file tree

Hide file tree

Showing 18 changed files with 607 additions and 129 deletions.
diff --git a/...ore/saved-objects/core-saved-objects-api-server-internal/src/lib/utils/included_fields.ts b/...ore/saved-objects/core-saved-objects-api-server-internal/src/lib/utils/included_fields.ts
@@ -11,7 +11,7 @@ const ROOT_FIELDS = [
   'namespaces',
   'type',
   'references',
-  'migrationVersion',
+  'migrationVersion', // deprecated, see https://github.com/elastic/kibana/pull/150075
   'coreMigrationVersion',
   'typeMigrationVersion',
   'managed',

diff --git a/...ts/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts b/...ts/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.test.ts
@@ -8,22 +8,35 @@
 
 import * as Either from 'fp-ts/lib/Either';
 import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal';
-import { checkTargetMappings } from './check_target_mappings';
-import { diffMappings } from '../core/build_active_mappings';
+import type { SavedObjectsMappingProperties } from '@kbn/core-saved-objects-server';
+import {
+  checkTargetMappings,
+  type ComparedMappingsChanged,
+  type ComparedMappingsMatch,
+} from './check_target_mappings';
+import { getUpdatedHashes } from '../core/build_active_mappings';
 
 jest.mock('../core/build_active_mappings');
 
-const diffMappingsMock = diffMappings as jest.MockedFn<typeof diffMappings>;
+const getUpdatedHashesMock = getUpdatedHashes as jest.MockedFn<typeof getUpdatedHashes>;
 
-const actualMappings: IndexMapping = {
-  properties: {
-    field: { type: 'integer' },
-  },
+const indexTypes = ['type1', 'type2'];
+
+const properties: SavedObjectsMappingProperties = {
+  type1: { type: 'long' },
+  type2: { type: 'long' },
+};
+
+const migrationMappingPropertyHashes = {
+  type1: 'type1Hash',
+  type2: 'type2Hash',
 };
 
 const expectedMappings: IndexMapping = {
-  properties: {
-    field: { type: 'long' },
+  properties,
+  dynamic: 'strict',
+  _meta: {
+    migrationMappingPropertyHashes,
   },
 };
 
@@ -32,48 +45,99 @@ describe('checkTargetMappings', () => {
     jest.clearAllMocks();
   });
 
-  it('returns match=false if source mappings are not defined', async () => {
-    const task = checkTargetMappings({
-      expectedMappings,
-    });
-
-    const result = await task();
-    expect(diffMappings).not.toHaveBeenCalled();
-    expect(result).toEqual(Either.right({ match: false }));
-  });
+  describe('when actual mappings are incomplete', () => {
+    it("returns 'actual_mappings_incomplete' if actual mappings are not defined", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+      });
 
-  it('calls diffMappings() with the source and target mappings', async () => {
-    const task = checkTargetMappings({
-      actualMappings,
-      expectedMappings,
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
     });
 
-    await task();
-    expect(diffMappings).toHaveBeenCalledTimes(1);
-    expect(diffMappings).toHaveBeenCalledWith(actualMappings, expectedMappings);
-  });
-
-  it('returns match=true if diffMappings() match', async () => {
-    diffMappingsMock.mockReturnValueOnce(undefined);
+    it("returns 'actual_mappings_incomplete' if actual mappings do not define _meta", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+        actualMappings: {
+          properties,
+          dynamic: 'strict',
+        },
+      });
+
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
+    });
 
-    const task = checkTargetMappings({
-      actualMappings,
-      expectedMappings,
+    it("returns 'actual_mappings_incomplete' if actual mappings do not define migrationMappingPropertyHashes", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+        actualMappings: {
+          properties,
+          dynamic: 'strict',
+          _meta: {},
+        },
+      });
+
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
     });
 
-    const result = await task();
-    expect(result).toEqual(Either.right({ match: true }));
+    it("returns 'actual_mappings_incomplete' if actual mappings define a different value for 'dynamic' property", async () => {
+      const task = checkTargetMappings({
+        indexTypes,
+        expectedMappings,
+        actualMappings: {
+          properties,
+          dynamic: false,
+          _meta: { migrationMappingPropertyHashes },
+        },
+      });
+
+      const result = await task();
+      expect(result).toEqual(Either.left({ type: 'actual_mappings_incomplete' as const }));
+    });
   });
 
-  it('returns match=false if diffMappings() finds differences', async () => {
-    diffMappingsMock.mockReturnValueOnce({ changedProp: 'field' });
-
-    const task = checkTargetMappings({
-      actualMappings,
-      expectedMappings,
+  describe('when actual mappings are complete', () => {
+    describe('and mappings do not match', () => {
+      it('returns the lists of changed root fields and types', async () => {
+        const task = checkTargetMappings({
+          indexTypes,
+          expectedMappings,
+          actualMappings: expectedMappings,
+        });
+
+        getUpdatedHashesMock.mockReturnValueOnce(['type1', 'type2', 'someRootField']);
+
+        const result = await task();
+        const expected: ComparedMappingsChanged = {
+          type: 'compared_mappings_changed' as const,
+          updatedRootFields: ['someRootField'],
+          updatedTypes: ['type1', 'type2'],
+        };
+        expect(result).toEqual(Either.left(expected));
+      });
     });
 
-    const result = await task();
-    expect(result).toEqual(Either.right({ match: false }));
+    describe('and mappings match', () => {
+      it('returns a compared_mappings_match response', async () => {
+        const task = checkTargetMappings({
+          indexTypes,
+          expectedMappings,
+          actualMappings: expectedMappings,
+        });
+
+        getUpdatedHashesMock.mockReturnValueOnce([]);
+
+        const result = await task();
+        const expected: ComparedMappingsMatch = {
+          type: 'compared_mappings_match' as const,
+        };
+        expect(result).toEqual(Either.right(expected));
+      });
+    });
   });
 });
diff --git a/...objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts b/...objects/core-saved-objects-migration-server-internal/src/actions/check_target_mappings.ts
@@ -8,29 +8,62 @@
 import * as Either from 'fp-ts/lib/Either';
 import * as TaskEither from 'fp-ts/lib/TaskEither';
 
-import { IndexMapping } from '@kbn/core-saved-objects-base-server-internal';
-import { diffMappings } from '../core/build_active_mappings';
+import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal';
+import { getUpdatedHashes } from '../core/build_active_mappings';
 
 /** @internal */
 export interface CheckTargetMappingsParams {
+  indexTypes: string[];
   actualMappings?: IndexMapping;
   expectedMappings: IndexMapping;
 }
 
 /** @internal */
-export interface TargetMappingsCompareResult {
-  match: boolean;
+export interface ComparedMappingsMatch {
+  type: 'compared_mappings_match';
+}
+
+export interface ActualMappingsIncomplete {
+  type: 'actual_mappings_incomplete';
+}
+
+export interface ComparedMappingsChanged {
+  type: 'compared_mappings_changed';
+  updatedRootFields: string[];
+  updatedTypes: string[];
 }
 
 export const checkTargetMappings =
   ({
+    indexTypes,
     actualMappings,
     expectedMappings,
-  }: CheckTargetMappingsParams): TaskEither.TaskEither<never, TargetMappingsCompareResult> =>
+  }: CheckTargetMappingsParams): TaskEither.TaskEither<
+    ActualMappingsIncomplete | ComparedMappingsChanged,
+    ComparedMappingsMatch
+  > =>
   async () => {
-    if (!actualMappings) {
-      return Either.right({ match: false });
+    if (
+      !actualMappings?._meta?.migrationMappingPropertyHashes ||
+      actualMappings.dynamic !== expectedMappings.dynamic
+    ) {
+      return Either.left({ type: 'actual_mappings_incomplete' as const });
+    }
+
+    const updatedHashes = getUpdatedHashes({
+      actual: actualMappings,
+      expected: expectedMappings,
+    });
+
+    if (updatedHashes.length) {
+      const updatedTypes = updatedHashes.filter((field) => indexTypes.includes(field));
+      const updatedRootFields = updatedHashes.filter((field) => !indexTypes.includes(field));
+      return Either.left({
+        type: 'compared_mappings_changed' as const,
+        updatedRootFields,
+        updatedTypes,
+      });
+    } else {
+      return Either.right({ type: 'compared_mappings_match' as const });
     }
-    const diff = diffMappings(actualMappings, expectedMappings);
-    return Either.right({ match: !diff });
   };
diff --git a/...ages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts b/...ages/core/saved-objects/core-saved-objects-migration-server-internal/src/actions/index.ts
@@ -108,6 +108,7 @@ import type { UnknownDocsFound } from './check_for_unknown_docs';
 import type { IncompatibleClusterRoutingAllocation } from './initialize_action';
 import type { ClusterShardLimitExceeded } from './create_index';
 import type { SynchronizationFailed } from './synchronize_migrators';
+import type { ActualMappingsIncomplete, ComparedMappingsChanged } from './check_target_mappings';
 
 export type {
   CheckForUnknownDocsParams,
@@ -176,6 +177,8 @@ export interface ActionErrorTypeMap {
   cluster_shard_limit_exceeded: ClusterShardLimitExceeded;
   es_response_too_large: EsResponseTooLargeError;
   synchronization_failed: SynchronizationFailed;
+  actual_mappings_incomplete: ActualMappingsIncomplete;
+  compared_mappings_changed: ComparedMappingsChanged;
 }
 
 /**

diff --git a/...jects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts b/...jects/core-saved-objects-migration-server-internal/src/actions/pickup_updated_mappings.ts
@@ -9,6 +9,7 @@
 import * as Either from 'fp-ts/lib/Either';
 import * as TaskEither from 'fp-ts/lib/TaskEither';
 import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server';
+import type { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types';
 import {
   catchRetryableEsClientErrors,
   type RetryableEsClientError,
@@ -20,7 +21,7 @@ export interface UpdateByQueryResponse {
 
 /**
  * Pickup updated mappings by performing an update by query operation on all
- * documents in the index. Returns a task ID which can be
+ * documents matching the passed in query. Returns a task ID which can be
  * tracked for progress.
  *
  * @remarks When mappings are updated to add a field which previously wasn't
@@ -35,7 +36,8 @@ export const pickupUpdatedMappings =
   (
     client: ElasticsearchClient,
     index: string,
-    batchSize: number
+    batchSize: number,
+    query?: QueryDslQueryContainer
   ): TaskEither.TaskEither<RetryableEsClientError, UpdateByQueryResponse> =>
   () => {
     return client
@@ -52,6 +54,8 @@ export const pickupUpdatedMappings =
         refresh: true,
         // Create a task and return task id instead of blocking until complete
         wait_for_completion: false,
+        // Only update the documents that match the provided query
+        query,
       })
       .then(({ task: taskId }) => {
         return Either.right({ taskId: String(taskId!) });