Skip to content

Commit

Permalink
[8.7] [Response Ops][Alerting] Delete unrecognized tasks when enabl…
Browse files Browse the repository at this point in the history 
…ing a rule (#152975) (#153152)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Response Ops][Alerting] Delete `unrecognized` tasks when enabling a
rule (#152975)](#152975)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Ying
Mao","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-03-11T03:22:19Z","message":"[Response
Ops][Alerting] Delete `unrecognized` tasks when enabling a rule
(#152975)","sha":"c875a284af465287dd3ba49b431086d4befac0e4","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Feature:Alerting","Team:ResponseOps","v8.7.0","v8.8.0"],"number":152975,"url":"https://github.com/elastic/kibana/pull/152975","mergeCommit":{"message":"[Response
Ops][Alerting] Delete `unrecognized` tasks when enabling a rule
(#152975)","sha":"c875a284af465287dd3ba49b431086d4befac0e4"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/152975","number":152975,"mergeCommit":{"message":"[Response
Ops][Alerting] Delete `unrecognized` tasks when enabling a rule
(#152975)","sha":"c875a284af465287dd3ba49b431086d4befac0e4"}}]}]
BACKPORT-->

Co-authored-by: Ying Mao <[email protected]>
  • Loading branch information
@kibanamachine @ymao1
kibanamachine and ymao1 authored Mar 11, 2023
1 parent 871292e commit fd217a3
Show file tree
Hide file tree
Showing 7 changed files with 435 additions and 8 deletions.
18 changes: 13 additions & 5 deletions x-pack/plugins/alerting/server/rules_client/methods/bulk_enable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { KueryNode, nodeBuilder } from '@kbn/es-query';
import { SavedObjectsBulkUpdateObject } from '@kbn/core/server';
import { withSpan } from '@kbn/apm-utils';
import { Logger } from '@kbn/core/server';
import { TaskManagerStartContract } from '@kbn/task-manager-plugin/server';
import { TaskManagerStartContract, TaskStatus } from '@kbn/task-manager-plugin/server';
import { RawRule, IntervalSchedule } from '../../types';
import { convertRuleIdsToKueryNode } from '../../lib';
import { ruleAuditEvent, RuleAuditAction } from '../common/audit_events';
Expand All @@ -36,10 +36,18 @@ const getShouldScheduleTask = async (
if (!scheduledTaskId) return true;
try {
// make sure scheduledTaskId exist
await withSpan({ name: 'getShouldScheduleTask', type: 'rules' }, () =>
context.taskManager.get(scheduledTaskId)
);
return false;
return await withSpan({ name: 'getShouldScheduleTask', type: 'rules' }, async () => {
const task = await context.taskManager.get(scheduledTaskId);

// Check whether task status is unrecognized. If so, we want to delete
// this task and create a fresh one
if (task.status === TaskStatus.Unrecognized) {
await context.taskManager.removeIfExists(scheduledTaskId);
return true;
}

return false;
});
} catch (err) {
return true;
}
Expand Down
10 changes: 9 additions & 1 deletion x-pack/plugins/alerting/server/rules_client/methods/enable.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { TaskStatus } from '@kbn/task-manager-plugin/server';
import { RawRule, IntervalSchedule } from '../../types';
import { resetMonitoringLastRun, getNextRun } from '../../lib';
import { WriteOperations, AlertingAuthorizationEntity } from '../../authorization';
Expand Down Expand Up @@ -110,7 +111,14 @@ async function enableWithOCC(context: RulesClientContext, { id }: { id: string }
if (attributes.scheduledTaskId) {
// If scheduledTaskId defined in rule SO, make sure it exists
try {
await context.taskManager.get(attributes.scheduledTaskId);
const task = await context.taskManager.get(attributes.scheduledTaskId);

// Check whether task status is unrecognized. If so, we want to delete
// this task and create a fresh one
if (task.status === TaskStatus.Unrecognized) {
await context.taskManager.removeIfExists(attributes.scheduledTaskId);
scheduledTaskIdToCreate = id;
}
} catch (err) {
scheduledTaskIdToCreate = id;
}
Expand Down
242 changes: 241 additions & 1 deletion x-pack/plugins/alerting/server/rules_client/tests/bulk_enable.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
returnedRule1,
returnedRule2,
} from './test_helpers';
import { TaskStatus } from '@kbn/task-manager-plugin/server';

jest.mock('../../invalidate_pending_api_keys/bulk_mark_api_keys_for_invalidation', () => ({
bulkMarkApiKeysForInvalidation: jest.fn(),
Expand Down Expand Up @@ -379,7 +380,7 @@ describe('bulkEnableRules', () => {
});

describe('taskManager', () => {
test('should return task id if deleting task failed', async () => {
test('should return task id if enabling task failed', async () => {
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRule1, enabledRule2],
});
Expand Down Expand Up @@ -487,6 +488,245 @@ describe('bulkEnableRules', () => {
);
expect(logger.error).toBeCalledTimes(0);
});

test('should schedule task when scheduledTaskId is defined but task with that ID does not', async () => {
// One rule gets the task successfully, one rule doesn't so only one task should be scheduled
taskManager.get.mockRejectedValueOnce(new Error('Failed to get task!'));
taskManager.schedule.mockResolvedValueOnce({
id: 'id1',
taskType: 'alerting:fakeType',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRule1, enabledRule2],
});

const result = await rulesClient.bulkEnableRules({ ids: ['id1', 'id2'] });

expect(taskManager.schedule).toHaveBeenCalledTimes(1);
expect(taskManager.schedule).toHaveBeenCalledWith({
id: 'id1',
taskType: `alerting:fakeType`,
params: {
alertId: 'id1',
spaceId: 'default',
consumer: 'fakeConsumer',
},
schedule: {
interval: '5m',
},
enabled: true,
state: {
alertInstances: {},
alertTypeState: {},
previousStartedAt: null,
},
scope: ['alerting'],
});

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: true,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: true,
}),
}),
]),
{ overwrite: true }
);

expect(result).toStrictEqual({
errors: [],
rules: [returnedRule1, returnedRule2],
total: 2,
taskIdsFailedToBeEnabled: [],
});
});

test('should schedule task when scheduledTaskId is not defined', async () => {
encryptedSavedObjects.createPointInTimeFinderDecryptedAsInternalUser = jest
.fn()
.mockResolvedValueOnce({
close: jest.fn(),
find: function* asyncGenerator() {
yield {
saved_objects: [
{
...disabledRule1,
attributes: { ...disabledRule1.attributes, scheduledTaskId: null },
},
disabledRule2,
],
};
},
});
taskManager.schedule.mockResolvedValueOnce({
id: 'id1',
taskType: 'alerting:fakeType',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRule1, enabledRule2],
});
const result = await rulesClient.bulkEnableRules({ ids: ['id1', 'id2'] });

expect(taskManager.schedule).toHaveBeenCalledTimes(1);
expect(taskManager.schedule).toHaveBeenCalledWith({
id: 'id1',
taskType: `alerting:fakeType`,
params: {
alertId: 'id1',
spaceId: 'default',
consumer: 'fakeConsumer',
},
schedule: {
interval: '5m',
},
enabled: true,
state: {
alertInstances: {},
alertTypeState: {},
previousStartedAt: null,
},
scope: ['alerting'],
});

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: true,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: true,
}),
}),
]),
{ overwrite: true }
);

expect(result).toStrictEqual({
errors: [],
rules: [returnedRule1, returnedRule2],
total: 2,
taskIdsFailedToBeEnabled: [],
});
});

test('should schedule task when task with scheduledTaskId exists but is unrecognized', async () => {
taskManager.get.mockResolvedValueOnce({
id: 'task-123',
taskType: 'alerting:123',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Unrecognized,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {
alertId: '1',
},
ownerId: null,
enabled: false,
});
taskManager.schedule.mockResolvedValueOnce({
id: 'id1',
taskType: 'alerting:fakeType',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});
unsecuredSavedObjectsClient.bulkCreate.mockResolvedValue({
saved_objects: [enabledRule1, enabledRule2],
});

const result = await rulesClient.bulkEnableRules({ ids: ['id1', 'id2'] });

expect(taskManager.removeIfExists).toHaveBeenCalledTimes(1);
expect(taskManager.removeIfExists).toHaveBeenCalledWith('id1');
expect(taskManager.schedule).toHaveBeenCalledTimes(1);
expect(taskManager.schedule).toHaveBeenCalledWith({
id: 'id1',
taskType: `alerting:fakeType`,
params: {
alertId: 'id1',
spaceId: 'default',
consumer: 'fakeConsumer',
},
schedule: {
interval: '5m',
},
enabled: true,
state: {
alertInstances: {},
alertTypeState: {},
previousStartedAt: null,
},
scope: ['alerting'],
});

expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.bulkCreate).toHaveBeenCalledWith(
expect.arrayContaining([
expect.objectContaining({
id: 'id1',
attributes: expect.objectContaining({
enabled: true,
}),
}),
expect.objectContaining({
id: 'id2',
attributes: expect.objectContaining({
enabled: true,
}),
}),
]),
{ overwrite: true }
);

expect(result).toStrictEqual({
errors: [],
rules: [returnedRule1, returnedRule2],
total: 2,
taskIdsFailedToBeEnabled: [],
});
});
});

describe('auditLogger', () => {
Expand Down
47 changes: 47 additions & 0 deletions x-pack/plugins/alerting/server/rules_client/tests/enable.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,53 @@ describe('enable()', () => {
});
});

test('schedules task when task with scheduledTaskId exists but is unrecognized', async () => {
taskManager.schedule.mockResolvedValueOnce({
id: '1',
taskType: 'alerting:123',
scheduledAt: new Date(),
attempts: 1,
status: TaskStatus.Idle,
runAt: new Date(),
startedAt: null,
retryAt: null,
state: {},
params: {},
ownerId: null,
});
taskManager.get.mockResolvedValue({ ...mockTask, status: TaskStatus.Unrecognized });
await rulesClient.enable({ id: '1' });
expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled();
expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', {
namespace: 'default',
});
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(2);
expect(taskManager.bulkEnable).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(taskManager.schedule).toHaveBeenCalledWith({
id: '1',
taskType: `alerting:myType`,
params: {
alertId: '1',
spaceId: 'default',
consumer: 'myApp',
},
schedule: {
interval: '10s',
},
enabled: true,
state: {
alertInstances: {},
alertTypeState: {},
previousStartedAt: null,
},
scope: ['alerting'],
});
expect(unsecuredSavedObjectsClient.update).toHaveBeenNthCalledWith(2, 'alert', '1', {
scheduledTaskId: '1',
});
});

test('throws error when scheduling task fails', async () => {
encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({
...existingRule,
Expand Down
Loading

0 comments on commit fd217a3

Please sign in to comment.