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

Schedule session index cleanup task only once. #75661

Merged
merged 3 commits into from
Sep 16, 2020
Merged

Schedule session index cleanup task only once. #75661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a1e1c1
Schedule session index cleanup task only once.
azasypkin Aug 21, 2020
b379065
Merge branch 'master' into issue-xxx-task-manager-workaround
azasypkin Sep 16, 2020
b2ea0bb
Merge branch 'master' into issue-xxx-task-manager-workaround
elasticmachine Sep 16, 2020
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
23 changes: 9 additions & 14 deletions x-pack/plugins/security/server/session_management/session_management_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ describe('SessionManagementService', () => {
mockStatusSubject.next({ scheduleRetry: mockScheduleRetry });
await nextTick();
expect(mockSessionIndexInitialize).toHaveBeenCalledTimes(2);
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(2);

// Session index task shouldn't be scheduled twice due to TM issue.
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);

expect(mockScheduleRetry).not.toHaveBeenCalled();
});
Expand Down Expand Up @@ -199,16 +201,8 @@ describe('SessionManagementService', () => {
expect(mockTaskManager.get).toHaveBeenCalledWith(SESSION_INDEX_CLEANUP_TASK_NAME);

expect(mockTaskManager.remove).not.toHaveBeenCalled();

expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledWith({
id: SESSION_INDEX_CLEANUP_TASK_NAME,
taskType: SESSION_INDEX_CLEANUP_TASK_NAME,
scope: ['security'],
schedule: { interval: '3600s' },
params: {},
state: {},
});
// No need to schedule a task if Task Manager says it's already scheduled.
expect(mockTaskManager.ensureScheduled).not.toHaveBeenCalled();
});

it('schedules retry if index initialization fails', async () => {
Expand All @@ -224,11 +218,12 @@ describe('SessionManagementService', () => {
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
expect(mockScheduleRetry).toHaveBeenCalledTimes(1);

// Still fails.
// Still fails, but cleanup task is scheduled already
mockTaskManager.get.mockResolvedValue({ schedule: { interval: '3600s' } } as any);
mockStatusSubject.next({ scheduleRetry: mockScheduleRetry });
await nextTick();
expect(mockSessionIndexInitialize).toHaveBeenCalledTimes(2);
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(2);
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
expect(mockScheduleRetry).toHaveBeenCalledTimes(2);

// And finally succeeds, retry is not scheduled.
Expand All @@ -237,7 +232,7 @@ describe('SessionManagementService', () => {
mockStatusSubject.next({ scheduleRetry: mockScheduleRetry });
await nextTick();
expect(mockSessionIndexInitialize).toHaveBeenCalledTimes(3);
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(3);
expect(mockTaskManager.ensureScheduled).toHaveBeenCalledTimes(1);
expect(mockScheduleRetry).toHaveBeenCalledTimes(2);
});

Expand Down
18 changes: 16 additions & 2 deletions x-pack/plugins/security/server/session_management/session_management_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export class SessionManagementService {
private statusSubscription?: Subscription;
private sessionIndex!: SessionIndex;
private config!: ConfigType;
private isCleanupTaskScheduled = false;

constructor(private readonly logger: Logger) {}

Expand Down Expand Up @@ -124,7 +125,12 @@ export class SessionManagementService {

// Check if currently scheduled task is scheduled with the correct interval.
const cleanupInterval = `${this.config.session.cleanupInterval.asSeconds()}s`;
if (currentTask && currentTask.schedule?.interval !== cleanupInterval) {
if (currentTask) {
if (currentTask.schedule?.interval === cleanupInterval) {
this.logger.debug('Session index cleanup task is already scheduled.');
return;
}

this.logger.debug(
'Session index cleanup interval has changed, the cleanup task will be rescheduled.'
);
Expand All @@ -139,6 +145,13 @@ export class SessionManagementService {
throw err;
}
}
} else if (this.isCleanupTaskScheduled) {
// WORKAROUND: This is a workaround for the Task Manager issue: https://github.com/elastic/kibana/issues/75501
// and should be removed as soon as this issue is resolved.
this.logger.error(
'Session index cleanup task has been already scheduled, but is missing in the task list for some reason. Please restart Kibana to automatically reschedule this task.'
);
return;
}

try {
Expand All @@ -151,10 +164,11 @@ export class SessionManagementService {
state: {},
});
} catch (err) {
this.logger.error(`Failed to register session index cleanup task: ${err.message}`);
this.logger.error(`Failed to schedule session index cleanup task: ${err.message}`);
throw err;
}

this.isCleanupTaskScheduled = true;
this.logger.debug('Successfully scheduled session index cleanup task.');
}
}