From dfee92802d386a9d1d64e62bd363695a2b860b31 Mon Sep 17 00:00:00 2001 From: Tim Sullivan Date: Wed, 2 Oct 2024 12:04:39 -0700 Subject: [PATCH] [Reporting] Delete the task from Task Manager when deleting a report (#192417) ## Summary Closes https://github.com/elastic/kibana/issues/191676 This PR ensures that when a user deletes a report, if there is a backing Task Manager task that references the report job, that task is deleted as well. With these changes, the report job will truly be "cancelled" when a report is deleted. The risk of not cancelling the Task Manager task when deleting a report is evident in server logs and in task monitoring, when automated tests are run on the reporting features. ### How to test Primarily, the API Integration test for the Reporting Datastream exercises the changes in this PR. ### Checklist - [x] Ensure the API/Functional tests are using the delete report endpoint rather than directly deleting reports from the ES store. * **To prevent this PR from becoming very large, this assurance is only needed for serverless tests, and for tests that do not wait for the report job to be completed before deleting the report document.** * Those constrictions narrow the changes down to just the test on the Reporting Datastream - [x] Handle scenario of report document deleted before task begins or completes - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../routes/common/jobs/get_job_routes.ts | 44 +++++++++++++++++++ .../common/reporting/datastream.ts | 11 +++-- .../common/reporting/generate_csv_discover.ts | 1 - .../shared/services/svl_reporting.ts | 29 +++++++----- x-pack/test_serverless/tsconfig.json | 1 - 5 files changed, 69 insertions(+), 17 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..dab96944ea6e8 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 @@ -113,6 +113,8 @@ export const commonJobsRouteHandlerFactory = ( 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. @@ -121,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: { + term: { + 'task.taskType': { value: '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. 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..ce9fe313ecf88 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 @@ -27,6 +27,7 @@ export default function ({ getService }: FtrProviderContext) { }; describe('Data Stream', function () { + const generatedReports = new Set(); before(async () => { roleAuthc = await svlUserManager.createM2mApiKeyWithRoleScope('admin'); internalReqHeader = svlCommonApi.getInternalRequestHeader(); @@ -34,8 +35,7 @@ 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( + const { job } = await reportingAPI.createReportJobInternal( 'csv_searchsource', { browserTimezone: 'UTC', @@ -51,10 +51,15 @@ export default function ({ getService }: FtrProviderContext) { roleAuthc, internalReqHeader ); + + generatedReports.add(job.id); }); after(async () => { - await reportingAPI.deleteAllReports(roleAuthc, internalReqHeader); + for (const reportId of generatedReports) { + await reportingAPI.deleteReport(reportId, roleAuthc, internalReqHeader); + } + await esArchiver.unload(archives.ecommerce.data); await kibanaServer.importExport.unload(archives.ecommerce.savedObjects); await svlUserManager.invalidateM2mApiKeyWithRoleScope(roleAuthc); diff --git a/x-pack/test_serverless/api_integration/test_suites/common/reporting/generate_csv_discover.ts b/x-pack/test_serverless/api_integration/test_suites/common/reporting/generate_csv_discover.ts index 740e106affa62..dd070d9a84aa2 100644 --- a/x-pack/test_serverless/api_integration/test_suites/common/reporting/generate_csv_discover.ts +++ b/x-pack/test_serverless/api_integration/test_suites/common/reporting/generate_csv_discover.ts @@ -101,7 +101,6 @@ export default function ({ getService }: FtrProviderContext) { }); after(async () => { - 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/shared/services/svl_reporting.ts b/x-pack/test_serverless/shared/services/svl_reporting.ts index 9d3d7941ec503..f056543e72e23 100644 --- a/x-pack/test_serverless/shared/services/svl_reporting.ts +++ b/x-pack/test_serverless/shared/services/svl_reporting.ts @@ -8,7 +8,6 @@ import expect from '@kbn/expect'; import { INTERNAL_ROUTES } from '@kbn/reporting-common'; import type { ReportingJobResponse } from '@kbn/reporting-plugin/server/types'; -import { REPORTING_DATA_STREAM_WILDCARD_WITH_LEGACY } from '@kbn/reporting-server'; import rison from '@kbn/rison'; import { FtrProviderContext } from '../../functional/ftr_provider_context'; import { RoleCredentials } from '.'; @@ -111,17 +110,23 @@ export function SvlReportingServiceProvider({ getService }: FtrProviderContext) .set(roleAuthc.apiKeyHeader); return response.text as unknown; }, - async deleteAllReports(roleAuthc: RoleCredentials, internalReqHeader: InternalRequestHeader) { - log.debug('ReportingAPI.deleteAllReports'); - - // ignores 409 errs and keeps retrying - await retry.tryForTime(5000, async () => { - await supertestWithoutAuth - .post(`/${REPORTING_DATA_STREAM_WILDCARD_WITH_LEGACY}/_delete_by_query`) - .set(internalReqHeader) - .set(roleAuthc.apiKeyHeader) - .send({ query: { match_all: {} } }); - }); + + /* + * Ensures reports are cleaned up through the delete report API + */ + async deleteReport( + reportId: string, + roleAuthc: RoleCredentials, + internalReqHeader: InternalRequestHeader + ) { + log.debug(`ReportingAPI.deleteReport ${INTERNAL_ROUTES.JOBS.DELETE_PREFIX}/${reportId}`); + const response = await supertestWithoutAuth + .delete(INTERNAL_ROUTES.JOBS.DELETE_PREFIX + `/${reportId}`) + .set(internalReqHeader) + .set(roleAuthc.apiKeyHeader) + .set('kbn-xsrf', 'xxx') + .expect(200); + return response.text as unknown; }, }; } diff --git a/x-pack/test_serverless/tsconfig.json b/x-pack/test_serverless/tsconfig.json index c27f5ea4fdda2..12ec49106c944 100644 --- a/x-pack/test_serverless/tsconfig.json +++ b/x-pack/test_serverless/tsconfig.json @@ -90,7 +90,6 @@ "@kbn/dataset-quality-plugin", "@kbn/alerting-comparators", "@kbn/search-types", - "@kbn/reporting-server", "@kbn/config-schema", "@kbn/features-plugin", "@kbn/observability-ai-assistant-plugin",