From 9e3673feaae644dbe35b99f394f8937deab5fa42 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Wed, 11 Mar 2020 13:11:32 +0100 Subject: [PATCH 1/3] Refactor: Move checking of closed index to single point We should rather only check if an index is currently closed the moment before starting to reindex. We still store a flag to indicate that we opened an index that was closed, but this should not be set from the reindex handlers because the reindex task may only start some time later in which case the closed index could have been opened and our reindex job will open it and close it again. --- .../server/lib/es_indices_state_check.ts | 15 +++++------ .../server/lib/es_migration_apis.ts | 5 +++- .../lib/reindexing/reindex_service.test.ts | 6 +++-- .../server/lib/reindexing/reindex_service.ts | 25 ++++++++++++++++--- .../routes/reindex_indices/reindex_handler.ts | 2 -- .../reindex_indices/reindex_indices.test.ts | 13 ++-------- .../routes/reindex_indices/reindex_indices.ts | 5 ---- 7 files changed, 38 insertions(+), 33 deletions(-) diff --git a/x-pack/plugins/upgrade_assistant/server/lib/es_indices_state_check.ts b/x-pack/plugins/upgrade_assistant/server/lib/es_indices_state_check.ts index 9931abf7f416c..eb20b7d6cde9c 100644 --- a/x-pack/plugins/upgrade_assistant/server/lib/es_indices_state_check.ts +++ b/x-pack/plugins/upgrade_assistant/server/lib/es_indices_state_check.ts @@ -4,27 +4,24 @@ * you may not use this file except in compliance with the Elastic License. */ -import { IScopedClusterClient } from 'kibana/server'; +import { APICaller } from 'kibana/server'; import { getIndexStateFromClusterState } from '../../common/get_index_state_from_cluster_state'; import { ClusterStateAPIResponse } from '../../common/types'; type StatusCheckResult = Record; export const esIndicesStateCheck = async ( - dataClient: IScopedClusterClient, + callAsUser: APICaller, indices: string[] ): Promise => { // According to https://www.elastic.co/guide/en/elasticsearch/reference/7.6/cluster-state.html // The response from this call is considered internal and subject to change. We have an API // integration test for asserting that the current ES version still returns what we expect. // This lives in x-pack/test/upgrade_assistant_integration - const clusterState: ClusterStateAPIResponse = await dataClient.callAsCurrentUser( - 'cluster.state', - { - index: indices, - metric: 'metadata', - } - ); + const clusterState: ClusterStateAPIResponse = await callAsUser('cluster.state', { + index: indices, + metric: 'metadata', + }); const result: StatusCheckResult = {}; diff --git a/x-pack/plugins/upgrade_assistant/server/lib/es_migration_apis.ts b/x-pack/plugins/upgrade_assistant/server/lib/es_migration_apis.ts index 3381e5506f39a..bace32f95c021 100644 --- a/x-pack/plugins/upgrade_assistant/server/lib/es_migration_apis.ts +++ b/x-pack/plugins/upgrade_assistant/server/lib/es_migration_apis.ts @@ -27,7 +27,10 @@ export async function getUpgradeAssistantStatus( // If we have found deprecation information for index/indices check whether the index is // open or closed. if (indexNames.length) { - const indexStates = await esIndicesStateCheck(dataClient, indexNames); + const indexStates = await esIndicesStateCheck( + dataClient.callAsCurrentUser.bind(dataClient), + indexNames + ); indices.forEach(indexData => { indexData.blockerForReindexing = diff --git a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.test.ts b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.test.ts index beb7b28e05e97..9c2593ee79f93 100644 --- a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.test.ts +++ b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.test.ts @@ -3,7 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ - +jest.mock('../es_indices_state_check', () => ({ esIndicesStateCheck: jest.fn() })); import { BehaviorSubject } from 'rxjs'; import { Logger } from 'src/core/server'; import { loggingServiceMock } from 'src/core/server/mocks'; @@ -19,6 +19,8 @@ import { CURRENT_MAJOR_VERSION, PREV_MAJOR_VERSION } from '../../../common/versi import { licensingMock } from '../../../../licensing/server/mocks'; import { LicensingPluginSetup } from '../../../../licensing/server'; +import { esIndicesStateCheck } from '../es_indices_state_check'; + import { isMlIndex, isWatcherIndex, @@ -43,6 +45,7 @@ describe('reindexService', () => { Promise.reject(`Mock function ${name} was not implemented!`); beforeEach(() => { + (esIndicesStateCheck as jest.Mock).mockResolvedValue({}); actions = { createReindexOp: jest.fn(unimplemented('createReindexOp')), deleteReindexOp: jest.fn(unimplemented('deleteReindexOp')), @@ -844,7 +847,6 @@ describe('reindexService', () => { attributes: { ...defaultAttributes, lastCompletedStep: ReindexStep.newIndexCreated, - reindexOptions: { openAndClose: false }, }, } as ReindexSavedObject; diff --git a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts index 4cc465e1f10b9..b14ebe912960e 100644 --- a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts +++ b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts @@ -6,6 +6,8 @@ import { APICaller, Logger } from 'src/core/server'; import { first } from 'rxjs/operators'; +import { LicensingPluginSetup } from '../../../../licensing/server'; + import { IndexGroup, ReindexOptions, @@ -15,14 +17,16 @@ import { ReindexWarning, } from '../../../common/types'; +import { esIndicesStateCheck } from '../es_indices_state_check'; + import { generateNewIndexName, getReindexWarnings, sourceNameForIndex, transformFlatSettings, } from './index_settings'; + import { ReindexActions } from './reindex_actions'; -import { LicensingPluginSetup } from '../../../../licensing/server'; import { error } from './error'; @@ -317,7 +321,9 @@ export const reindexServiceFactory = ( const startReindexing = async (reindexOp: ReindexSavedObject) => { const { indexName, reindexOptions } = reindexOp.attributes; - if (reindexOptions?.openAndClose === true) { + const indicesState = await esIndicesStateCheck(callAsUser, [indexName]); + const openAndClose = indicesState[indexName] === 'close'; + if (indicesState[indexName] === 'close') { await callAsUser('indices.open', { index: indexName }); } @@ -334,6 +340,12 @@ export const reindexServiceFactory = ( lastCompletedStep: ReindexStep.reindexStarted, reindexTaskId: startReindex.task, reindexTaskPercComplete: 0, + reindexOptions: { + ...(reindexOptions ?? {}), + // Indicate to downstream states whether we opened a closed index that should be + // closed again. + openAndClose, + }, }); }; @@ -654,9 +666,16 @@ export const reindexServiceFactory = ( throw new Error(`Reindex operation must be paused in order to be resumed.`); } + const reindexOptions: ReindexOptions | undefined = opts + ? { + ...(op.attributes.reindexOptions ?? {}), + ...opts, + } + : undefined; + return actions.updateReindexOp(op, { status: ReindexStatus.inProgress, - reindexOptions: opts ?? op.attributes.reindexOptions, + reindexOptions, }); }); }, diff --git a/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_handler.ts b/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_handler.ts index b7569d8679590..e640d03791cce 100644 --- a/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_handler.ts +++ b/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_handler.ts @@ -24,7 +24,6 @@ interface ReindexHandlerArgs { headers: Record; credentialStore: CredentialStore; reindexOptions?: { - openAndClose?: boolean; enqueue?: boolean; }; } @@ -56,7 +55,6 @@ export const reindexHandler = async ({ const opts: ReindexOptions | undefined = reindexOptions ? { - openAndClose: reindexOptions.openAndClose, queueSettings: reindexOptions.enqueue ? { queuedAt: Date.now() } : undefined, } : undefined; diff --git a/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.test.ts b/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.test.ts index dc1516ad76560..df8b2fa80a25a 100644 --- a/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.test.ts +++ b/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.test.ts @@ -20,7 +20,6 @@ const mockReindexService = { resumeReindexOperation: jest.fn(), cancelReindexing: jest.fn(), }; -jest.mock('../../lib/es_indices_state_check', () => ({ esIndicesStateCheck: jest.fn() })); jest.mock('../../lib/es_version_precheck', () => ({ versionCheckHandlerWrapper: (a: any) => a, })); @@ -39,7 +38,6 @@ import { } from '../../../common/types'; import { credentialStoreFactory } from '../../lib/reindexing/credential_store'; import { registerReindexIndicesRoutes } from './reindex_indices'; -import { esIndicesStateCheck } from '../../lib/es_indices_state_check'; /** * Since these route callbacks are so thin, these serve simply as integration tests @@ -57,7 +55,6 @@ describe('reindex API', () => { } as any; beforeEach(() => { - (esIndicesStateCheck as jest.Mock).mockResolvedValue({}); mockRouter = createMockRouter(); routeDependencies = { credentialStore, @@ -168,9 +165,7 @@ describe('reindex API', () => { ); // It called create correctly - expect(mockReindexService.createReindexOperation).toHaveBeenCalledWith('theIndex', { - openAndClose: false, - }); + expect(mockReindexService.createReindexOperation).toHaveBeenCalledWith('theIndex', undefined); // It returned the right results expect(resp.status).toEqual(200); @@ -237,10 +232,7 @@ describe('reindex API', () => { kibanaResponseFactory ); // It called resume correctly - expect(mockReindexService.resumeReindexOperation).toHaveBeenCalledWith('theIndex', { - openAndClose: false, - queueSettings: undefined, - }); + expect(mockReindexService.resumeReindexOperation).toHaveBeenCalledWith('theIndex', undefined); expect(mockReindexService.createReindexOperation).not.toHaveBeenCalled(); // It returned the right results @@ -269,7 +261,6 @@ describe('reindex API', () => { describe('POST /api/upgrade_assistant/reindex/batch', () => { const queueSettingsArg = { - openAndClose: false, queueSettings: { queuedAt: expect.any(Number) }, }; it('creates a collection of index operations', async () => { diff --git a/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.ts b/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.ts index 0846e6c0d31d3..686f93b771e62 100644 --- a/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.ts +++ b/x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.ts @@ -16,7 +16,6 @@ import { LicensingPluginSetup } from '../../../../licensing/server'; import { ReindexStatus } from '../../../common/types'; import { versionCheckHandlerWrapper } from '../../lib/es_version_precheck'; -import { esIndicesStateCheck } from '../../lib/es_indices_state_check'; import { reindexServiceFactory, ReindexWorker } from '../../lib/reindexing'; import { CredentialStore } from '../../lib/reindexing/credential_store'; import { reindexActionsFactory } from '../../lib/reindexing/reindex_actions'; @@ -108,7 +107,6 @@ export function registerReindexIndicesRoutes( response ) => { const { indexName } = request.params; - const indexStates = await esIndicesStateCheck(dataClient, [indexName]); try { const result = await reindexHandler({ savedObjects: savedObjectsClient, @@ -118,7 +116,6 @@ export function registerReindexIndicesRoutes( licensing, headers: request.headers, credentialStore, - reindexOptions: { openAndClose: indexStates[indexName] === 'close' }, }); // Kick the worker on this node to immediately pickup the new reindex operation. @@ -190,7 +187,6 @@ export function registerReindexIndicesRoutes( response ) => { const { indexNames } = request.body; - const indexStates = await esIndicesStateCheck(dataClient, indexNames); const results: PostBatchResponse = { enqueued: [], errors: [], @@ -206,7 +202,6 @@ export function registerReindexIndicesRoutes( headers: request.headers, credentialStore, reindexOptions: { - openAndClose: indexStates[indexName] === 'close', enqueue: true, }, }); From 14236e83401295b7c06cc29517d65f0a5edf59db Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Wed, 11 Mar 2020 13:32:07 +0100 Subject: [PATCH 2/3] Added debug log --- .../upgrade_assistant/server/lib/reindexing/reindex_service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts index b14ebe912960e..a6a1152317a0a 100644 --- a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts +++ b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts @@ -324,6 +324,7 @@ export const reindexServiceFactory = ( const indicesState = await esIndicesStateCheck(callAsUser, [indexName]); const openAndClose = indicesState[indexName] === 'close'; if (indicesState[indexName] === 'close') { + log.debug(`Detected closed index ${indexName}, opening...`); await callAsUser('indices.open', { index: indexName }); } From bf624248ba338ee7e143429874536c35de85c42a Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Thu, 12 Mar 2020 09:56:51 +0100 Subject: [PATCH 3/3] Added comment --- .../upgrade_assistant/server/lib/reindexing/reindex_service.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts index a6a1152317a0a..b270998658db8 100644 --- a/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts +++ b/x-pack/plugins/upgrade_assistant/server/lib/reindexing/reindex_service.ts @@ -321,6 +321,8 @@ export const reindexServiceFactory = ( const startReindexing = async (reindexOp: ReindexSavedObject) => { const { indexName, reindexOptions } = reindexOp.attributes; + // Where possible, derive reindex options at the last moment before reindexing + // to prevent them from becoming stale as they wait in the queue. const indicesState = await esIndicesStateCheck(callAsUser, [indexName]); const openAndClose = indicesState[indexName] === 'close'; if (indicesState[indexName] === 'close') {