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.

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

[RAM] refactors RulesClient.update, by using new Task Manager API bulkUpdateSchedules #134027

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
2759c2e
init taskManager bulk
vitaliidm May 20, 2022
d24c2f3
updates and fixes
vitaliidm May 23, 2022
ff55e01
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm May 23, 2022
6efc5fb
fix the rest of tests
vitaliidm May 23, 2022
655a753
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm May 23, 2022
74c5a7c
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm May 26, 2022
7e1e7c7
add unit tests
vitaliidm May 26, 2022
815854b
tests!!!
vitaliidm May 30, 2022
79fc692
refactor it
vitaliidm May 30, 2022
8197409
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm May 30, 2022
e9d4426
add test to rukes_client
vitaliidm May 31, 2022
51469cc
Merge branch 'task-manager-bulk-schedules' of https://github.com/vita…
vitaliidm May 31, 2022
0b96f46
tests, more tests
vitaliidm May 31, 2022
9f1ccf7
README, docs
vitaliidm May 31, 2022
edc7890
skip again
vitaliidm Jun 1, 2022
d20f2fd
add rest of ops
vitaliidm Jun 1, 2022
f572388
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm Jun 1, 2022
f2bc696
tests
vitaliidm Jun 1, 2022
0b44ef1
comments updates
vitaliidm Jun 6, 2022
cb5411a
JSDoc
vitaliidm Jun 6, 2022
bdeadf6
few perf improvements
vitaliidm Jun 6, 2022
0563f88
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm Jun 6, 2022
bd36d9a
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm Jun 8, 2022
64e7adb
CR: replace auditLogger with logger.error
vitaliidm Jun 8, 2022
6fb6bee
Merge branch 'main' into task-manager-bulk-schedules
vitaliidm Jun 9, 2022
f079cd7
[RAM] rules-client-update-move-to-new-taks-manager-api
vitaliidm Jun 9, 2022
24e9f45
fix failing unti tests
vitaliidm Jun 9, 2022
4136dd3
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jun 9, 2022
cb16f8c
Merge branch 'main' into repsponse-ops/rules-client-update-move-to-ne…
vitaliidm Jun 13, 2022
adcee94
Merge branch 'main' into repsponse-ops/rules-client-update-move-to-ne…
vitaliidm Jun 15, 2022
f216bbf
Update x-pack/plugins/alerting/server/rules_client/rules_client.ts
vitaliidm Jun 15, 2022
d59787d
Update x-pack/plugins/alerting/server/rules_client/rules_client.ts
vitaliidm Jun 15, 2022
834d7fb
Merge branch 'main' into repsponse-ops/rules-client-update-move-to-ne…
vitaliidm Jun 15, 2022
1b46b3a
CR: add runAt in
vitaliidm Jun 15, 2022
6bdcc9f
fix test
vitaliidm Jun 15, 2022
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
27 changes: 15 additions & 12 deletions x-pack/plugins/alerting/server/rules_client/rules_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1245,20 +1245,23 @@ export class RulesClient {
(async () => {
if (
updateResult.scheduledTaskId &&
updateResult.schedule &&
!isEqual(alertSavedObject.attributes.schedule, updateResult.schedule)
) {
this.taskManager
.runNow(updateResult.scheduledTaskId)
.then(() => {
this.logger.debug(
`Alert update has rescheduled the underlying task: ${updateResult.scheduledTaskId}`
);
})
.catch((err: Error) => {
this.logger.error(
`Alert update failed to run its underlying task. TaskManager runNow failed with Error: ${err.message}`
);
});
try {
const { tasks } = await this.taskManager.bulkUpdateSchedules(
[updateResult.scheduledTaskId],
updateResult.schedule
);

this.logger.debug(
`Rule update has rescheduled the underlying task: ${updateResult.scheduledTaskId} to run at: ${tasks?.[0]?.runAt}`
);
} catch (err) {
this.logger.error(
`Rule update failed to run its underlying task. TaskManager bulkUpdateSchedules failed with Error: ${err.message}`
);
}
}
})(),
]);
Expand Down
57 changes: 9 additions & 48 deletions x-pack/plugins/alerting/server/rules_client/tests/update.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { RecoveredActionGroup } from '../../../common';
import { encryptedSavedObjectsMock } from '@kbn/encrypted-saved-objects-plugin/server/mocks';
import { actionsAuthorizationMock } from '@kbn/actions-plugin/server/mocks';
import { AlertingAuthorization } from '../../authorization/alerting_authorization';
import { resolvable } from '../../test_utils';
import { ActionsAuthorization, ActionsClient } from '@kbn/actions-plugin/server';
import { TaskStatus } from '@kbn/task-manager-plugin/server';
import { auditLoggerMock } from '@kbn/security-plugin/server/audit/mocks';
Expand Down Expand Up @@ -1585,7 +1584,7 @@ describe('update()', () => {
taskManager.runNow.mockReturnValueOnce(Promise.resolve({ id: alertId }));
}

test('updating the alert schedule should rerun the task immediately', async () => {
test('updating the alert schedule should call taskManager.bulkUpdateSchedules', async () => {
const alertId = uuid.v4();
const taskId = uuid.v4();

Expand Down Expand Up @@ -1614,10 +1613,10 @@ describe('update()', () => {
},
});

expect(taskManager.runNow).toHaveBeenCalledWith(taskId);
expect(taskManager.bulkUpdateSchedules).toHaveBeenCalledWith([taskId], { interval: '1m' });
});

test('updating the alert without changing the schedule should not rerun the task', async () => {
test('updating the alert without changing the schedule should not call taskManager.bulkUpdateSchedules', async () => {
const alertId = uuid.v4();
const taskId = uuid.v4();

Expand Down Expand Up @@ -1646,55 +1645,17 @@ describe('update()', () => {
},
});

expect(taskManager.runNow).not.toHaveBeenCalled();
expect(taskManager.bulkUpdateSchedules).not.toHaveBeenCalled();
});

test('updating the alert should not wait for the rerun the task to complete', async () => {
Copy link
Contributor Author

@vitaliidm vitaliidm Jun 13, 2022

Choose a reason for hiding this comment

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

I believe we don't need this test anymore: we not running task anymore right after rule's SO updated, only doing REST API calls.
So, once user updates rule's schedule, there will be no time expensive run of rule, only underlying task update.
In that case, we can wait to this update finished and we won't need to leave unfinished HTTP requests once return response of RulesClient.update

test('logs when update of schedule of an alerts underlying task fails', async () => {
const alertId = uuid.v4();
const taskId = uuid.v4();

mockApiCalls(alertId, taskId, { interval: '1m' }, { interval: '30s' });

const resolveAfterAlertUpdatedCompletes = resolvable<{ id: string }>();

taskManager.runNow.mockReset();
taskManager.runNow.mockReturnValue(resolveAfterAlertUpdatedCompletes);

await rulesClient.update({
id: alertId,
data: {
schedule: { interval: '1m' },
name: 'abc',
tags: ['foo'],
params: {
bar: true,
},
throttle: null,
notifyWhen: null,
actions: [
{
group: 'default',
id: '1',
params: {
foo: true,
},
},
],
},
});

expect(taskManager.runNow).toHaveBeenCalled();
resolveAfterAlertUpdatedCompletes.resolve({ id: alertId });
});

test('logs when the rerun of an alerts underlying task fails', async () => {
const alertId = uuid.v4();
const taskId = uuid.v4();

mockApiCalls(alertId, taskId, { interval: '1m' }, { interval: '30s' });

taskManager.runNow.mockReset();
taskManager.runNow.mockRejectedValue(new Error('Failed to run alert'));
taskManager.bulkUpdateSchedules.mockReset();
taskManager.bulkUpdateSchedules.mockRejectedValue(new Error('Failed to run alert'));

await rulesClient.update({
id: alertId,
Expand All @@ -1719,10 +1680,10 @@ describe('update()', () => {
},
});

expect(taskManager.runNow).toHaveBeenCalled();
expect(taskManager.bulkUpdateSchedules).toHaveBeenCalled();

expect(rulesClientParams.logger.error).toHaveBeenCalledWith(
`Alert update failed to run its underlying task. TaskManager runNow failed with Error: Failed to run alert`
`Rule update failed to run its underlying task. TaskManager bulkUpdateSchedules failed with Error: Failed to run alert`
);
});
});
Expand Down