Skip to content

Commit

Permalink
[8.x] [Reporting] Delete the task from Task Manager when deleting a r…
Browse files Browse the repository at this point in the history
…eport (#192417) (#194758)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Reporting] Delete the task from Task Manager when deleting a report
(#192417)](#192417)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Tim
Sullivan","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-02T19:04:39Z","message":"[Reporting]
Delete the task from Task Manager when deleting a report (#192417)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191676\r\n\r\nThis PR ensures
that when a user deletes a report, if there is a backing\r\nTask Manager
task that references the report job, that task is deleted\r\nas well.
With these changes, the report job will truly be \"cancelled\"\r\nwhen a
report is deleted.\r\n\r\nThe risk of not cancelling the Task Manager
task when deleting a report\r\nis evident in server logs and in task
monitoring, when automated tests\r\nare run on the reporting
features.\r\n\r\n### How to test\r\nPrimarily, the API Integration test
for the Reporting Datastream\r\nexercises the changes in this
PR.\r\n\r\n### Checklist\r\n\r\n- [x] Ensure the API/Functional tests
are using the delete report\r\nendpoint rather than directly deleting
reports from the ES store.\r\n* **To prevent this PR from becoming very
large, this assurance is only\r\nneeded for serverless tests, and for
tests that do not wait for the\r\nreport job to be completed before
deleting the report document.**\r\n* Those constrictions narrow the
changes down to just the test on the\r\nReporting Datastream\r\n- [x]
Handle scenario of report document deleted before task begins
or\r\ncompletes\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"dfee92802d386a9d1d64e62bd363695a2b860b31","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:SharedUX","v8.16.0","Feature:Reporting:Framework","backport:version"],"title":"[Reporting]
Delete the task from Task Manager when deleting a
report","number":192417,"url":"https://github.com/elastic/kibana/pull/192417","mergeCommit":{"message":"[Reporting]
Delete the task from Task Manager when deleting a report (#192417)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191676\r\n\r\nThis PR ensures
that when a user deletes a report, if there is a backing\r\nTask Manager
task that references the report job, that task is deleted\r\nas well.
With these changes, the report job will truly be \"cancelled\"\r\nwhen a
report is deleted.\r\n\r\nThe risk of not cancelling the Task Manager
task when deleting a report\r\nis evident in server logs and in task
monitoring, when automated tests\r\nare run on the reporting
features.\r\n\r\n### How to test\r\nPrimarily, the API Integration test
for the Reporting Datastream\r\nexercises the changes in this
PR.\r\n\r\n### Checklist\r\n\r\n- [x] Ensure the API/Functional tests
are using the delete report\r\nendpoint rather than directly deleting
reports from the ES store.\r\n* **To prevent this PR from becoming very
large, this assurance is only\r\nneeded for serverless tests, and for
tests that do not wait for the\r\nreport job to be completed before
deleting the report document.**\r\n* Those constrictions narrow the
changes down to just the test on the\r\nReporting Datastream\r\n- [x]
Handle scenario of report document deleted before task begins
or\r\ncompletes\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"dfee92802d386a9d1d64e62bd363695a2b860b31"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192417","number":192417,"mergeCommit":{"message":"[Reporting]
Delete the task from Task Manager when deleting a report (#192417)\n\n##
Summary\r\n\r\nCloses
https://github.com/elastic/kibana/issues/191676\r\n\r\nThis PR ensures
that when a user deletes a report, if there is a backing\r\nTask Manager
task that references the report job, that task is deleted\r\nas well.
With these changes, the report job will truly be \"cancelled\"\r\nwhen a
report is deleted.\r\n\r\nThe risk of not cancelling the Task Manager
task when deleting a report\r\nis evident in server logs and in task
monitoring, when automated tests\r\nare run on the reporting
features.\r\n\r\n### How to test\r\nPrimarily, the API Integration test
for the Reporting Datastream\r\nexercises the changes in this
PR.\r\n\r\n### Checklist\r\n\r\n- [x] Ensure the API/Functional tests
are using the delete report\r\nendpoint rather than directly deleting
reports from the ES store.\r\n* **To prevent this PR from becoming very
large, this assurance is only\r\nneeded for serverless tests, and for
tests that do not wait for the\r\nreport job to be completed before
deleting the report document.**\r\n* Those constrictions narrow the
changes down to just the test on the\r\nReporting Datastream\r\n- [x]
Handle scenario of report document deleted before task begins
or\r\ncompletes\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"dfee92802d386a9d1d64e62bd363695a2b860b31"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Tim Sullivan <[email protected]>
  • Loading branch information
kibanamachine and tsullivan authored Oct 2, 2024
1 parent 246f25d commit 69f3b54
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ export default function ({ getService }: FtrProviderContext) {
};

describe('Data Stream', function () {
const generatedReports = new Set<string>();
before(async () => {
roleAuthc = await svlUserManager.createM2mApiKeyWithRoleScope('admin');
internalReqHeader = svlCommonApi.getInternalRequestHeader();

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',
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down
29 changes: 17 additions & 12 deletions x-pack/test_serverless/shared/services/svl_reporting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 '.';
Expand Down Expand Up @@ -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;
},
};
}
1 change: 0 additions & 1 deletion x-pack/test_serverless/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 69f3b54

Please sign in to comment.