Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Migrations] Only pickup updated SO types when performing a compatible migration #159962

Merged
merged 12 commits into from
Jun 30, 2023
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const ROOT_FIELDS = [
'namespaces',
'type',
'references',
'migrationVersion',
'migrationVersion', // deprecated, see https://github.com/elastic/kibana/pull/150075
'coreMigrationVersion',
'typeMigrationVersion',
'managed',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,36 @@
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 { diffMappings, 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' },
type1: { type: 'integer' },
type2: { type: 'integer' },
},
_meta: {
migrationMappingPropertyHashes: {
type1: 'type1OldHash',
type2: 'type2OldHash',
},
},
};

const expectedMappings: IndexMapping = {
properties: {
field: { type: 'long' },
type1: { type: 'long' },
type2: { type: 'long' },
},
_meta: {
migrationMappingPropertyHashes: {
type1: 'type1NewHash',
type2: 'type2NewHash',
},
},
};

Expand Down Expand Up @@ -66,14 +81,15 @@ describe('checkTargetMappings', () => {
});

it('returns match=false if diffMappings() finds differences', async () => {
diffMappingsMock.mockReturnValueOnce({ changedProp: 'field' });
diffMappingsMock.mockReturnValueOnce({ changedProp: 'type1' });
getUpdatedHashesMock.mockReturnValueOnce(['type1', 'type2']);

const task = checkTargetMappings({
actualMappings,
expectedMappings,
});

const result = await task();
expect(result).toEqual(Either.right({ match: false }));
expect(result).toEqual(Either.right({ match: false, updatedHashes: ['type1', 'type2'] }));
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
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 { diffMappings, getUpdatedHashes } from '../core/build_active_mappings';

/** @internal */
export interface CheckTargetMappingsParams {
Expand All @@ -20,6 +20,7 @@ export interface CheckTargetMappingsParams {
/** @internal */
export interface TargetMappingsCompareResult {
match: boolean;
updatedHashes?: string[];
}

export const checkTargetMappings =
Expand All @@ -32,5 +33,6 @@ export const checkTargetMappings =
return Either.right({ match: false });
}
const diff = diffMappings(actualMappings, expectedMappings);
return Either.right({ match: !diff });
const updatedHashes = getUpdatedHashes(actualMappings, expectedMappings);
return Either.right({ match: !diff, updatedHashes });
};
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -35,7 +36,8 @@ export const pickupUpdatedMappings =
(
client: ElasticsearchClient,
index: string,
batchSize: number
batchSize: number,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit on line 23 the comment says:

Pickup updated mappings by performing an update by query operation on all documents in the index. Returns a task ID which can be tracked for progress.

maybe we can update that to be "on all documents matching the passed in query"

query?: QueryDslQueryContainer
): TaskEither.TaskEither<RetryableEsClientError, UpdateByQueryResponse> =>
() => {
return client
Expand All @@ -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!) });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,47 +11,76 @@ import { errors as EsErrors } from '@elastic/elasticsearch';
import { elasticsearchClientMock } from '@kbn/core-elasticsearch-client-server-mocks';
import { updateAndPickupMappings } from './update_and_pickup_mappings';
import { DEFAULT_TIMEOUT } from './constants';
import { pickupUpdatedMappings } from './pickup_updated_mappings';

jest.mock('./catch_retryable_es_client_errors');
jest.mock('./pickup_updated_mappings');

describe('updateAndPickupMappings', () => {
beforeEach(() => {
jest.clearAllMocks();
});

// Create a mock client that rejects all methods with a 503 status code
// response.
const retryableError = new EsErrors.ResponseError(
elasticsearchClientMock.createApiResponse({
statusCode: 503,
body: { error: { type: 'es_type', reason: 'es_reason' } },
})
);
const client = elasticsearchClientMock.createInternalClient(
elasticsearchClientMock.createErrorTransportRequestPromise(retryableError)
);
describe('putMappingTask', () => {
// Create a mock client that rejects all methods with a 503 status code
// response.
const retryableError = new EsErrors.ResponseError(
elasticsearchClientMock.createApiResponse({
statusCode: 503,
body: { error: { type: 'es_type', reason: 'es_reason' } },
})
);
const client = elasticsearchClientMock.createInternalClient(
elasticsearchClientMock.createErrorTransportRequestPromise(retryableError)
);

it('calls catchRetryableEsClientErrors when the promise rejects', async () => {
const task = updateAndPickupMappings({
client,
index: 'new_index',
mappings: { properties: {} },
batchSize: 1000,
it('calls catchRetryableEsClientErrors when the promise rejects', async () => {
const task = updateAndPickupMappings({
client,
index: 'new_index',
mappings: { properties: {} },
batchSize: 1000,
});
try {
await task();
} catch (e) {
/** ignore */
}

expect(catchRetryableEsClientErrors).toHaveBeenCalledWith(retryableError);
});
try {
await task();
} catch (e) {
/** ignore */
}

expect(catchRetryableEsClientErrors).toHaveBeenCalledWith(retryableError);
});
it('calls the indices.putMapping with the mapping properties as well as the _meta information', async () => {
const task = updateAndPickupMappings({
client,
index: 'new_index',
mappings: {
properties: {
'apm-indices': {
type: 'object',
dynamic: false,
},
},
_meta: {
migrationMappingPropertyHashes: {
references: '7997cf5a56cc02bdc9c93361bde732b0',
'epm-packages': '860e23f4404fa1c33f430e6dad5d8fa2',
'cases-connector-mappings': '17d2e9e0e170a21a471285a5d845353c',
},
},
},
batchSize: 1000,
});
try {
await task();
} catch (e) {
/** ignore */
}

it('calls the indices.putMapping with the mapping properties as well as the _meta information', async () => {
const task = updateAndPickupMappings({
client,
index: 'new_index',
mappings: {
expect(client.indices.putMapping).toHaveBeenCalledTimes(1);
expect(client.indices.putMapping).toHaveBeenCalledWith({
index: 'new_index',
timeout: DEFAULT_TIMEOUT,
properties: {
'apm-indices': {
type: 'object',
Expand All @@ -65,32 +94,47 @@ describe('updateAndPickupMappings', () => {
'cases-connector-mappings': '17d2e9e0e170a21a471285a5d845353c',
},
},
},
batchSize: 1000,
});
});
try {
await task();
} catch (e) {
/** ignore */
}
});

expect(client.indices.putMapping).toHaveBeenCalledTimes(1);
expect(client.indices.putMapping).toHaveBeenCalledWith({
index: 'new_index',
timeout: DEFAULT_TIMEOUT,
properties: {
'apm-indices': {
type: 'object',
dynamic: false,
},
},
_meta: {
migrationMappingPropertyHashes: {
references: '7997cf5a56cc02bdc9c93361bde732b0',
'epm-packages': '860e23f4404fa1c33f430e6dad5d8fa2',
'cases-connector-mappings': '17d2e9e0e170a21a471285a5d845353c',
describe('pickupUpdatedMappings', () => {
const client = elasticsearchClientMock.createInternalClient(
elasticsearchClientMock.createSuccessTransportRequestPromise({})
);

it('calls pickupUpdatedMappings with the right parameters', async () => {
const query = {
bool: {
should: [
{
term: {
type: 'type1',
},
},
{
term: {
type: 'type2',
},
},
],
},
},
};
const task = updateAndPickupMappings({
client,
index: 'new_index',
mappings: { properties: {} },
batchSize: 1000,
query,
});
try {
await task();
} catch (e) {
/** ignore */
}

expect(pickupUpdatedMappings).toHaveBeenCalledTimes(1);
expect(pickupUpdatedMappings).toHaveBeenCalledWith(client, 'new_index', 1000, query);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as TaskEither from 'fp-ts/lib/TaskEither';
import { pipe } from 'fp-ts/lib/pipeable';
import type { ElasticsearchClient } from '@kbn/core-elasticsearch-server';
import type { IndexMapping } from '@kbn/core-saved-objects-base-server-internal';
import type { QueryDslQueryContainer } from '@elastic/elasticsearch/lib/api/types';
import {
catchRetryableEsClientErrors,
type RetryableEsClientError,
Expand All @@ -29,6 +30,7 @@ export interface UpdateAndPickupMappingsParams {
index: string;
mappings: IndexMapping;
batchSize: number;
query?: QueryDslQueryContainer;
}
/**
* Updates an index's mappings and runs an pickupUpdatedMappings task so that the mapping
Expand All @@ -39,6 +41,7 @@ export const updateAndPickupMappings = ({
index,
mappings,
batchSize,
query,
}: UpdateAndPickupMappingsParams): TaskEither.TaskEither<
RetryableEsClientError,
UpdateAndPickupMappingsResponse
Expand Down Expand Up @@ -76,7 +79,7 @@ export const updateAndPickupMappings = ({
return pipe(
putMappingTask,
TaskEither.chain((res) => {
return pickupUpdatedMappings(client, index, batchSize);
return pickupUpdatedMappings(client, index, batchSize, query);
})
);
};
Loading