From abc14f5d34b951c46d482c430d2f7fd983844c1b 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 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..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 @@ -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 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, }, });