Skip to content

Commit

Permalink
[UA] Remove noisey warn when deleting from .tasks (#204720)
Browse files Browse the repository at this point in the history
## Summary

Reduce the number of `warn` logs reindexing will create due to failed
attempts to delete from `.tasks`. In this PR we only log for responses
that do not have status code 403 or 401.


### Checklist

- [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
  • Loading branch information
jloleysens authored Dec 18, 2024
1 parent 53748fd commit ff3a600
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { esIndicesStateCheck } from '../es_indices_state_check';
import { versionService } from '../version';

import { ReindexService, reindexServiceFactory } from './reindex_service';
import { error } from './error';

const asApiResponse = <T>(body: T): TransportResult<T> =>
({
Expand Down Expand Up @@ -638,12 +637,7 @@ describe('reindexService', () => {
index: '.tasks',
id: 'xyz',
});
expect(log.warn).toHaveBeenCalledTimes(1);
expect(log.warn).toHaveBeenCalledWith(
error.reindexTaskCannotBeDeleted(
`Could not delete reindexing task xyz, got response "!?"`
)
);
expect(log.warn).toHaveBeenCalledTimes(0); // Do not log anything in this case
});

it('does not throw if task doc deletion throws', async () => {
Expand Down Expand Up @@ -672,6 +666,32 @@ describe('reindexService', () => {
expect(log.warn).toHaveBeenCalledTimes(1);
expect(log.warn).toHaveBeenCalledWith(new Error('FAILED!'));
});

it.each([401, 403])(
'does not throw if task doc deletion throws AND does not log due to missing access permission: %d',
async (statusCode) => {
clusterClient.asCurrentUser.tasks.get.mockResponseOnce({
completed: true,
// @ts-expect-error not full interface
task: { status: { created: 100, total: 100 } },
});

clusterClient.asCurrentUser.count.mockResponseOnce(
// @ts-expect-error not full interface
{
count: 100,
}
);
const e = new Error();
Object.defineProperty(e, 'statusCode', { value: statusCode });
clusterClient.asCurrentUser.delete.mockRejectedValue(e);

const updatedOp = await service.processNextStep(reindexOp);
expect(updatedOp.attributes.lastCompletedStep).toEqual(ReindexStep.reindexCompleted);
expect(updatedOp.attributes.reindexTaskPercComplete).toEqual(1);
expect(log.warn).toHaveBeenCalledTimes(0);
}
);
});

describe('reindex task is cancelled', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,20 +309,17 @@ export const reindexServiceFactory = (
}

try {
// Delete the task from ES .tasks index
const deleteTaskResp = await esClient.delete({
// Best effort, delete the task from ES .tasks index...
await esClient.delete({
index: '.tasks',
id: taskId,
});
if (deleteTaskResp.result !== 'deleted') {
log.warn(
error.reindexTaskCannotBeDeleted(
`Could not delete reindexing task ${taskId}, got response "${deleteTaskResp.result}"`
)
);
}
} catch (e) {
log.warn(e);
// We explicitly ignore authz related error codes bc we expect this to be
// very common when deleting from .tasks
if (e?.statusCode !== 401 && e?.statusCode !== 403) {
log.warn(e);
}
}

return reindexOp;
Expand Down

0 comments on commit ff3a600

Please sign in to comment.