From 4caaf84ca6b8e2988932b7c8e7569af6df25dbec Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Thu, 22 Aug 2024 10:38:59 -0700 Subject: [PATCH 1/2] fixme comments for where report job cleanup needs to wait on report being finished and/or remove the report job from task manager --- .../reporting/server/routes/common/jobs/get_job_routes.ts | 2 ++ x-pack/test/accessibility/apps/group3/reporting.ts | 1 + x-pack/test/functional/apps/discover/reporting.ts | 2 +- .../reporting_and_security/datastream.ts | 3 +-- .../reporting_and_security/ilm_migration_apis.ts | 1 + .../reporting_without_security/csv/job_apis_csv.ts | 1 + x-pack/test/reporting_api_integration/services/scenarios.ts | 1 + .../reporting_functional/reporting_and_security/management.ts | 2 +- .../api_integration/test_suites/common/reporting/datastream.ts | 2 +- .../api_integration/test_suites/common/reporting/management.ts | 2 +- 10 files changed, 11 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts b/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts index dc01a29db780a..32fb37db53f56 100644 --- a/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts +++ b/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts @@ -108,6 +108,8 @@ export const commonJobsRouteHandlerFactory = ( counters, { isInternal }, async (doc) => { + // FIXME look for potential report job task in task manager and remove that too + const docIndex = doc.index; const stream = await getContentStream(reporting, { id: docId, index: docIndex }); const reportingSetup = reporting.getPluginSetupDeps(); diff --git a/x-pack/test/accessibility/apps/group3/reporting.ts b/x-pack/test/accessibility/apps/group3/reporting.ts index 45959e42b383a..3d2f39f83c94a 100644 --- a/x-pack/test/accessibility/apps/group3/reporting.ts +++ b/x-pack/test/accessibility/apps/group3/reporting.ts @@ -36,6 +36,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { }); after(async () => { + // FIXME: wait for queued report to finish before clean up await reporting.teardownLogs(); await deleteReportingUser(); }); diff --git a/x-pack/test/functional/apps/discover/reporting.ts b/x-pack/test/functional/apps/discover/reporting.ts index 41882ef130790..887e083db490d 100644 --- a/x-pack/test/functional/apps/discover/reporting.ts +++ b/x-pack/test/functional/apps/discover/reporting.ts @@ -103,7 +103,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await PageObjects.discover.selectIndexPattern('ecommerce'); }); - it('generates a report with single timefilter', async () => { + it('exposes a POST URL for a report with single timefilter', async () => { await PageObjects.discover.clickNewSearchButton(); await PageObjects.timePicker.setCommonlyUsedTime('Last_24 hours'); await PageObjects.discover.saveSearch('single-timefilter-search'); diff --git a/x-pack/test/reporting_api_integration/reporting_and_security/datastream.ts b/x-pack/test/reporting_api_integration/reporting_and_security/datastream.ts index f116110db78f1..2b723c0f92c63 100644 --- a/x-pack/test/reporting_api_integration/reporting_and_security/datastream.ts +++ b/x-pack/test/reporting_api_integration/reporting_and_security/datastream.ts @@ -16,14 +16,13 @@ export default function ({ getService }: FtrProviderContext) { describe('Data Stream', () => { before(async () => { await reportingAPI.initEcommerce(); - - // for this test, we don't need to wait for the job to finish or verify the result await reportingAPI.postJob( `/api/reporting/generate/csv_searchsource?jobParams=%28browserTimezone%3AUTC%2Ccolumns%3A%21%28%29%2CobjectType%3Asearch%2CsearchSource%3A%28fields%3A%21%28%28field%3A%27%2A%27%2Cinclude_unmapped%3Atrue%29%29%2Cfilter%3A%21%28%28meta%3A%28field%3A%27%40timestamp%27%2Cindex%3A%27logstash-%2A%27%2Cparams%3A%28%29%29%2Cquery%3A%28range%3A%28%27%40timestamp%27%3A%28format%3Astrict_date_optional_time%2Cgte%3A%272015-09-22T09%3A17%3A53.728Z%27%2Clte%3A%272015-09-22T09%3A30%3A50.786Z%27%29%29%29%29%2C%28%27%24state%27%3A%28store%3AappState%29%2Cmeta%3A%28alias%3A%21n%2Cdisabled%3A%21f%2Cindex%3A%27logstash-%2A%27%2Ckey%3Aquery%2Cnegate%3A%21f%2Ctype%3Acustom%2Cvalue%3A%27%7B%22bool%22%3A%7B%22minimum_should_match%22%3A1%2C%22should%22%3A%5B%7B%22match_phrase%22%3A%7B%22%40tags%22%3A%22info%22%7D%7D%5D%7D%7D%27%29%2Cquery%3A%28bool%3A%28minimum_should_match%3A1%2Cshould%3A%21%28%28match_phrase%3A%28%27%40tags%27%3Ainfo%29%29%29%29%29%29%29%2Cindex%3A%27logstash-%2A%27%2Cquery%3A%28language%3Akuery%2Cquery%3A%27%27%29%2Csort%3A%21%28%28%27%40timestamp%27%3A%28format%3Astrict_date_optional_time%2Corder%3Adesc%29%29%29%29%2Ctitle%3A%27A%20saved%20search%20with%20match_phrase%20filter%20and%20no%20columns%20selected%27%2Cversion%3A%278.15.0%27%29` ); }); after(async () => { + // FIXME wait for report to complete before cleanup await reportingAPI.deleteAllReports(); await reportingAPI.teardownEcommerce(); }); diff --git a/x-pack/test/reporting_api_integration/reporting_and_security/ilm_migration_apis.ts b/x-pack/test/reporting_api_integration/reporting_and_security/ilm_migration_apis.ts index 56009ccdd8b3d..c30ff3982fcf0 100644 --- a/x-pack/test/reporting_api_integration/reporting_and_security/ilm_migration_apis.ts +++ b/x-pack/test/reporting_api_integration/reporting_and_security/ilm_migration_apis.ts @@ -72,6 +72,7 @@ export default function ({ getService }: FtrProviderContext) { }); after(async () => { + // FIXME wait for reports to complete before teardown await reportingAPI.teardownLogs(); await reportingAPI.makeAllReportingIndicesUnmanaged(); // ensure that a delete phase does not remove the index while future tests are running }); diff --git a/x-pack/test/reporting_api_integration/reporting_without_security/csv/job_apis_csv.ts b/x-pack/test/reporting_api_integration/reporting_without_security/csv/job_apis_csv.ts index ade8efab7166c..10bef6c82b854 100644 --- a/x-pack/test/reporting_api_integration/reporting_without_security/csv/job_apis_csv.ts +++ b/x-pack/test/reporting_api_integration/reporting_without_security/csv/job_apis_csv.ts @@ -55,6 +55,7 @@ export default function ({ getService }: FtrProviderContext) { }); after(async () => { + // FIXME wait for reports to complete before teardown await reportingAPI.teardownLogs(); await esArchiver.unload('x-pack/test/functional/es_archives/logstash_functional'); }); diff --git a/x-pack/test/reporting_api_integration/services/scenarios.ts b/x-pack/test/reporting_api_integration/services/scenarios.ts index ed542cc74e44e..bebb4e6ef59db 100644 --- a/x-pack/test/reporting_api_integration/services/scenarios.ts +++ b/x-pack/test/reporting_api_integration/services/scenarios.ts @@ -208,6 +208,7 @@ export function createScenarios({ getService }: Pick { log.debug('ReportingAPI.deleteAllReports'); diff --git a/x-pack/test/reporting_functional/reporting_and_security/management.ts b/x-pack/test/reporting_functional/reporting_and_security/management.ts index e1d124ede931e..f8f45b8043701 100644 --- a/x-pack/test/reporting_functional/reporting_and_security/management.ts +++ b/x-pack/test/reporting_functional/reporting_and_security/management.ts @@ -21,6 +21,7 @@ export default ({ getService, getPageObjects }: FtrProviderContext) => { await reportingFunctional.initEcommerce(); }); after(async () => { + // FIXME: wait for the report job to finish before clean up await reportingFunctional.teardownEcommerce(); }); @@ -47,7 +48,6 @@ export default ({ getService, getPageObjects }: FtrProviderContext) => { await PageObjects.common.navigateToApp('reporting'); await PageObjects.common.sleep(3000); // Wait an amount of time for auto-polling to refresh the jobs - // We do not need to wait for the report to finish generating await (await testSubjects.find('euiCollapsedItemActionsButton')).click(); await (await testSubjects.find('reportOpenInKibanaApp')).click(); diff --git a/x-pack/test_serverless/api_integration/test_suites/common/reporting/datastream.ts b/x-pack/test_serverless/api_integration/test_suites/common/reporting/datastream.ts index 0541ff426e605..fe2ab2f255b1f 100644 --- a/x-pack/test_serverless/api_integration/test_suites/common/reporting/datastream.ts +++ b/x-pack/test_serverless/api_integration/test_suites/common/reporting/datastream.ts @@ -34,7 +34,6 @@ export default function ({ getService }: FtrProviderContext) { await esArchiver.load(archives.ecommerce.data); await kibanaServer.importExport.load(archives.ecommerce.savedObjects); - // for this test, we don't need to wait for the job to finish or verify the result await reportingAPI.createReportJobInternal( 'csv_searchsource', { @@ -54,6 +53,7 @@ export default function ({ getService }: FtrProviderContext) { }); after(async () => { + // FIXME wait for job to finish before clean up await reportingAPI.deleteAllReports(roleAuthc, internalReqHeader); await esArchiver.unload(archives.ecommerce.data); await kibanaServer.importExport.unload(archives.ecommerce.savedObjects); diff --git a/x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts b/x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts index 62e9f4eaf8acd..11f0d32dc78f2 100644 --- a/x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts +++ b/x-pack/test_serverless/api_integration/test_suites/common/reporting/management.ts @@ -30,6 +30,7 @@ export default ({ getService }: FtrProviderContext) => { internalReqHeader = svlCommonApi.getInternalRequestHeader(); }); after(async () => { + // FIXME wait for job to finish before invalidating user await svlUserManager.invalidateM2mApiKeyWithRoleScope(adminUser); }); @@ -63,7 +64,6 @@ export default ({ getService }: FtrProviderContext) => { }); it(`user can delete a report they've created`, async () => { - // for this test, we don't need to wait for the job to finish or verify the result const response = await supertestWithoutAuth .delete(`${INTERNAL_ROUTES.JOBS.DELETE_PREFIX}/${reportJob.id}`) .set(...API_HEADER) From d407145e1a2fcdf3903a3532b272cbe6cefeb55e Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Mon, 9 Sep 2024 15:45:56 -0700 Subject: [PATCH 2/2] [Reporting] Delete the task from Task Manager when deleting a report --- .../routes/common/jobs/get_job_routes.ts | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts b/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts index 32fb37db53f56..13d6fdfa1068e 100644 --- a/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts +++ b/x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts @@ -108,13 +108,13 @@ export const commonJobsRouteHandlerFactory = ( counters, { isInternal }, async (doc) => { - // FIXME look for potential report job task in task manager and remove that too - const docIndex = doc.index; const stream = await getContentStream(reporting, { id: docId, index: docIndex }); const reportingSetup = reporting.getPluginSetupDeps(); const logger = reportingSetup.logger.get('delete-report'); + logger.debug(`Deleting report ${docId}`); + // An "error" event is emitted if an error is // passed to the `stream.end` callback from // the _final method of the ContentStream. @@ -123,6 +123,48 @@ export const commonJobsRouteHandlerFactory = ( logger.error(err); }); + // 1. Look for a task in task manager associated with the report job + try { + let taskId: string | undefined; + const { taskManager } = await reporting.getPluginStartDeps(); + const result = await taskManager.fetch({ + query: { + match: { + 'task.taskType': 'report:execute', + }, + }, + size: 1000, // NOTE: this is an arbitrary size that is likely to include all running and pending reporting tasks in most deployments + }); + + if (result.docs.length > 0) { + // The task params are stored as a string of JSON. In order to find the task that corresponds to + // the report to delete, we need to check each task's params, look for the report id, and see if it + // matches our docId to delete. + for (const task of result.docs) { + const { params } = task; + if (params.id === docId) { + // found the matching task + taskId = task.id; + logger.debug( + `Found a Task Manager task associated with the report being deleted: ${taskId}. Task status: ${task.status}.` + ); + break; + } + } + if (taskId) { + // remove the task that was found + await taskManager.remove(taskId); + logger.debug(`Deleted Task Manager task ${taskId}.`); + } + } + } catch (error) { + logger.error( + 'Encountered an error in finding a task associated with the report being deleted' + ); + logger.error(error); + } + + // 2. Remove the report document try { // Overwriting existing content with an // empty buffer to remove all the chunks.