Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Reporting] Delete the task from Task Manager when deleting a report #192417

Merged
merged 4 commits into from
Oct 2, 2024
Merged

[Reporting] Delete the task from Task Manager when deleting a report #192417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0441201
[Reporting] Delete the task from Task Manager when deleting a report
tsullivan Aug 22, 2024
fdb6e72
update tests
tsullivan Sep 17, 2024
a3ca7a7
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Oct 2, 2024
538d6f2
Merge branch 'main' into reporting/delete-task-associated-with-report…
tsullivan Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions x-pack/plugins/reporting/server/routes/common/jobs/get_job_routes.ts
View file
Open in desktop
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' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, just wanted to double check that we can't query by id here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The report ID is part of the task payload, which is stored in the attributes.params data of the task document. The format of this field is a string of JSON, which needs to be parsed before it is sent to the consumer.

Searching for an ID substring in the JSON string field is possible, using a wildcard or regexp query. But I don't recommend we do this:

  1. We would probably want to have the loop anyway, to ensure accuracy.
  2. These are expensive query types that can be disabled in cluster settings or elasticsearch.yml.

},
},
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
11 changes: 8 additions & 3 deletions x-pack/test_serverless/api_integration/test_suites/common/reporting/datastream.ts
View file
Open in desktop
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the line that causes "failures" in monitoring deployments that are used by automated tests.

  1. Stages a report job, just for the side-effect of initializing the datastream
  2. Deletes the report job when the test is finished, but a reference to the job is still encoded in the pending report job task
  3. Deletes the saved object which is used in the report.
  4. Invalidates the user credentials which are encoded in the pending report job task

Result: when it tries to execute the report job task, it will fail.

  • If the task begins after the report job is deleted, it would fail because the task runner's job is to query Reporting storage for the report job data
  • If the task begins before the report job is deleted and somehow accesses the report job data, it would fail because the saved object or user credentials were deleted

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
1 change: 0 additions & 1 deletion x-pack/test_serverless/api_integration/test_suites/common/reporting/generate_csv_discover.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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