From 55dcbcf2f1f392060effc518dae5e2b0261d115b Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Thu, 12 Mar 2020 11:39:33 +0100 Subject: [PATCH] [Upgrade Assistant] Open And Close Slight Refactor (#59890) * 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. * Added debug log * Added comment Co-authored-by: Elastic Machine --- .../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 | 28 +++++++++++++++++-- .../routes/reindex_indices/reindex_handler.ts | 2 -- .../reindex_indices/reindex_indices.test.ts | 13 ++------- .../routes/reindex_indices/reindex_indices.ts | 5 ---- 7 files changed, 41 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 9931abf7f416c41..eb20b7d6cde9c24 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 3381e5506f39a6a..bace32f95c02155 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 beb7b28e05e97b1..9c2593ee79f9373 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 4cc465e1f10b9ce..b270998658db8a1 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,12 @@ export const reindexServiceFactory = ( const startReindexing = async (reindexOp: ReindexSavedObject) => { const { indexName, reindexOptions } = reindexOp.attributes; - if (reindexOptions?.openAndClose === true) { + // 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') { + log.debug(`Detected closed index ${indexName}, opening...`); await callAsUser('indices.open', { index: indexName }); } @@ -334,6 +343,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 +669,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 b7569d86795909d..e640d03791cce55 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 dc1516ad765609a..df8b2fa80a25add 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 0846e6c0d31d3b0..686f93b771e6289 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, }, });