Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
fdcd142
get template version
tsullivan Jan 10, 2019
0a8a195
ensure putTemplate only creates/updates the template
tsullivan Jan 10, 2019
f01f89f
fix tests
tsullivan Jan 10, 2019
e55750a
new test for throwing error re old template
tsullivan Jan 10, 2019
640fa48
note comment
tsullivan Jan 10, 2019
5e18822
clean up test
tsullivan Jan 10, 2019
3d39746
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 11, 2019
1196d88
test clarification
tsullivan Jan 11, 2019
a0845b1
store kibana metadata in scheduled task doc
tsullivan Jan 11, 2019
d7b1891
map `dynamic: false`
tsullivan Jan 11, 2019
1ca259b
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 14, 2019
1ccafe5
logging
tsullivan Jan 14, 2019
4f2b61a
add kibana uuid
tsullivan Jan 14, 2019
a367be4
fix tests
tsullivan Jan 14, 2019
eda8508
last todo
tsullivan Jan 14, 2019
d4bccd6
fetching available task uses apiVersion in the query
tsullivan Jan 14, 2019
8e76fbd
scheduledAt
tsullivan Jan 14, 2019
d2d8ccb
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 15, 2019
bd7f717
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 16, 2019
1a9cc96
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 17, 2019
5eb15d7
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 28, 2019
d0eb5c2
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 29, 2019
5745cc7
ts fix
tsullivan Jan 29, 2019
730fe9e
ts fix
tsullivan Jan 30, 2019
537e7ca
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 30, 2019
578bc29
Merge branch 'master' into taskmanager/template-safety
tsullivan Jan 30, 2019
b659944
fix snapshot
tsullivan Jan 30, 2019
dcd90d8
await to fail test if snapshot does not match
tsullivan Jan 30, 2019
0e5d17c
Merge branch 'master' into taskmanager/template-safety
tsullivan Feb 5, 2019
3e4a3fe
Merge branch 'master' into taskmanager/template-safety
tsullivan Feb 5, 2019
36dd212
fix bad merge
tsullivan Feb 5, 2019
c4d1611
remove _.get
tsullivan Feb 5, 2019
5efaa93
fix typeError happening in tests
tsullivan Feb 5, 2019
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
11 changes: 11 additions & 0 deletions x-pack/plugins/task_manager/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import xPackage from '../../package.json';
import { getTemplateVersion } from './lib/get_template_version';

export const TASK_MANAGER_API_VERSION = 1;
export const TASK_MANAGER_TEMPLATE_VERSION = getTemplateVersion(xPackage.version);
43 changes: 43 additions & 0 deletions x-pack/plugins/task_manager/lib/get_template_version.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { getTemplateVersion } from './get_template_version';

describe('getTemplateVersion', () => {
it('converts a release build version string into an integer', () => {
const versionStr1 = '7.1.2';
expect(getTemplateVersion(versionStr1)).toBe(7010299);

const versionStr2 = '10.1.0';
expect(getTemplateVersion(versionStr2)).toBe(10010099);
});

it('converts a alpha build version string into an integer', () => {
const versionStr1 = '7.0.0-alpha1';
expect(getTemplateVersion(versionStr1)).toBe(7000001);

const versionStr2 = '7.0.0-alpha3';
expect(getTemplateVersion(versionStr2)).toBe(7000003);
});

it('converts a beta build version string into an integer', () => {
const versionStr1 = '7.0.0-beta4';
expect(getTemplateVersion(versionStr1)).toBe(7000029);

const versionStr5 = '7.0.0-beta8';
expect(getTemplateVersion(versionStr5)).toBe(7000033);
});

it('converts a snapshot build version string into an integer', () => {
expect(getTemplateVersion('8.0.0-alpha1')).toBe(8000001);
expect(getTemplateVersion('8.0.0-alpha1-snapshot')).toBe(8000001);
});

it('not intended to handle any version parts with 3-digits: it will create malformed integer values', () => {
expect(getTemplateVersion('60.61.1') === getTemplateVersion('6.6.101')).toBe(true); // both produce 60610199
expect(getTemplateVersion('1.32.0') < getTemplateVersion('1.3.223423')).toBe(true);
});
});
56 changes: 56 additions & 0 deletions x-pack/plugins/task_manager/lib/get_template_version.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { padLeft } from 'lodash';

/*
* The logic for ID is: XXYYZZAA, where XX is major version, YY is minor
* version, ZZ is revision, and AA is alpha/beta/rc indicator.
*
* AA values below 25 are for alpha builder (since 5.0), and above 25 and below
* 50 are beta builds, and below 99 are RC builds, with 99 indicating a release
* the (internal) format of the id is there so we can easily do after/before
* checks on the id
*
* Note: the conversion method is carried over from Elasticsearch:
* https://github.com/elastic/elasticsearch/blob/de962b2/server/src/main/java/org/elasticsearch/Version.java
*/
export function getTemplateVersion(versionStr: string): number {
// break up the string parts
Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this, and it doesn't seem to handle some edge cases.

The following expression evaluates to true, but should be false (e.g. 1.32 is greater than 1.3).

getTemplateVersion('1.32.0') < getTemplateVersion('1.3.223423')

Is there a reason we aren't using the semver library to handle semvers? It's what we're using in migrations, for example.

Copy link
Member

Choose a reason for hiding this comment

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

He matched the version behavior that Elasticsearch uses, which simply doesn't allow for 3-digit, or greater, minor and patch releases. If we get into a situation where we have Kibana 6.2.100, then I think we have other issues.

We could add a test that pulls in the packaged version and asserts that it isn't >= 99 so that we can be aware that the next minor / patch release will have that issue though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love using semver here, but the value for the version field needs to be an integer per the ES API. I took a stab at looking for whether using semver would help this code more-or-less as it is. It can be used to replace versionStr.split but I don't see much other benefit.

But as this function needs to produce an integer, I think it still has the same problem with 3-digit version parts that you bring up

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 thread is relevant to a call we had with @njd5475 and @chrisdavies and others

  1. What actually "upgrades" existing documents in the task manager index?
    Since that isn't a concern to worry about for this version, I haven't been addressing that very much. I think this PR will create a logical space in the code where upgrade logic will go in the future.
  2. Use semver? Or integer?
    In this PR, I'm setting up a way to define what version of Kibana a document will be compatible with, and doing that with the kibana.apiVersion field. That lets me create a filter in the search query to ES. However, if the documents are going to be compatible with the Kibana Migrations service, their versioning will be done with semver, and the version should not necessarily update with each release. Also, if the documents were stored using the Saved Objects client, this would happen automatically.

If I understand things correctly, Kibana Migrations and Saved Objects client mean we don't need to worry about BWC in our own code? That would be great! Although, for now there seem to be no features that use those capabilities other than core Kibana for the .kibana index. Given that, I think the more hands-on versioning that I'm doing here is OK for now (Task Manager has no consumers yet, and is a beta service). In the future, maintainers can go over TaskStore and fashion it to use the Saved Objects client.

Does this sound ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @cwurm, was on the call and had some thoughts about Saved Object client as well

Copy link
Contributor

Choose a reason for hiding this comment

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

Not me :)

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry - I meant @weltenwort

const splitted = versionStr.split('.');
const minorStr = splitted[2] || '';

// pad each part with leading 0 to make 2 characters
const padded = splitted.map((v: string) => {
const vMatches = v.match(/\d+/);
if (vMatches) {
return padLeft(vMatches[0], 2, '0');
}
return '00';
});
const [majorV, minorV, patchV] = padded;

// append the alpha/beta/rc indicator
let buildV;
if (minorStr.match('alpha')) {
const matches = minorStr.match(/alpha(?<alpha>\d+)/);
if (matches != null && matches.groups != null) {
const alphaVerInt = parseInt(matches.groups.alpha, 10); // alpha build indicator
buildV = padLeft(`${alphaVerInt}`, 2, '0');
}
} else if (minorStr.match('beta')) {
const matches = minorStr.match(/beta(?<beta>\d+)/);
if (matches != null && matches.groups != null) {
const alphaVerInt = parseInt(matches.groups.beta, 10) + 25; // beta build indicator
buildV = padLeft(`${alphaVerInt}`, 2, '0');
}
} else {
buildV = '99'; // release build indicator
}

const joinedParts = [majorV, minorV, patchV, buildV].join('');
return parseInt(joinedParts, 10);
}
12 changes: 12 additions & 0 deletions x-pack/plugins/task_manager/task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,12 @@ export interface TaskInstance {
*/
taskType: string;

/**
* The date and time that this task was originally scheduled. This is used
* for convenience to task run functions, and for troubleshooting.
*/
scheduledAt?: Date;

/**
* The date and time that this task is scheduled to be run. It is not
* guaranteed to run at this time, but it is guaranteed not to run earlier
Expand Down Expand Up @@ -210,6 +216,12 @@ export interface ConcreteTaskInstance extends TaskInstance {
*/
version: number;

/**
* The date and time that this task was originally scheduled. This is used
* for convenience to task run functions, and for troubleshooting.
*/
scheduledAt: Date;

/**
* The number of unsuccessful attempts since the last successful run. This
* will be zeroed out after a successful run.
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/task_manager/task_manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ describe('TaskManager', () => {
});

function testOpts() {
const callCluster = sinon.stub();
callCluster.withArgs('indices.getTemplate').returns(Promise.resolve({ tasky: {} }));

const $test = {
events: {} as any,
afterPluginsInit: _.noop,
Expand All @@ -100,7 +103,7 @@ describe('TaskManager', () => {
plugins: {
elasticsearch: {
getCluster() {
return { callWithInternalUser: _.noop };
return { callWithInternalUser: callCluster };
},
status: {
on(eventName: string, callback: () => any) {
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/task_manager/task_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,15 @@ export class TaskManager {

const logger = new TaskManagerLogger((...args: any[]) => server.log(...args));

/* Kibana UUID needs to be pulled live (not cached), as it takes a long time
* to initialize, and can change after startup */
const store = new TaskStore({
callCluster: server.plugins.elasticsearch.getCluster('admin').callWithInternalUser,
index: config.get('xpack.task_manager.index'),
maxAttempts: config.get('xpack.task_manager.max_attempts'),
supportedTypes: Object.keys(this.definitions),
logger,
getKibanaUuid: () => config.get('server.uuid'),
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like we are using the kibana uuid. Why exactly would we want to add it if we are not using it?

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 is for troubleshooting. With any Kibana instance in the cluster able to claim a task, it'll be great being able to track problematic instances doing stuff to the tasks. We're doing this in the Reporting index as well, because we saw benefit in being able to track it

});
const pool = new TaskPool({
logger,
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/task_manager/task_poller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,13 @@ let store: TaskStore;

describe('TaskPoller', () => {
beforeEach(() => {
const callCluster = sinon.spy();
const callCluster = sinon.stub();
callCluster.withArgs('indices.getTemplate').returns(Promise.resolve({ tasky: {} }));
const getKibanaUuid = sinon.stub().returns('kibana-123-uuid-test');
store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
Expand Down
70 changes: 64 additions & 6 deletions x-pack/plugins/task_manager/task_store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,33 @@

import _ from 'lodash';
import sinon from 'sinon';
import {
TASK_MANAGER_API_VERSION as API_VERSION,
TASK_MANAGER_TEMPLATE_VERSION as TEMPLATE_VERSION,
} from './constants';
import { TaskInstance, TaskStatus } from './task';
import { FetchOpts, TaskStore } from './task_store';
import { mockLogger } from './test_utils';

const getKibanaUuid = sinon.stub().returns('kibana-uuid-123-test');

describe('TaskStore', () => {
describe('init', () => {
test('creates the task manager index', async () => {
const callCluster = sinon.spy();
const callCluster = sinon.stub();
callCluster.withArgs('indices.getTemplate').returns(Promise.resolve({ tasky: {} }));
const store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
});

await store.init();

sinon.assert.calledOnce(callCluster);
sinon.assert.calledTwice(callCluster); // store.init calls twice: once to check for existing template, once to put the template (if needed)

sinon.assert.calledWithMatch(callCluster, 'indices.putTemplate', {
body: {
Expand All @@ -35,27 +45,64 @@ describe('TaskStore', () => {
name: 'tasky',
});
});

test('logs a warning if newer index template exists', async () => {
const callCluster = sinon.stub();
callCluster
.withArgs('indices.getTemplate')
.returns(Promise.resolve({ tasky: { version: Infinity } }));

const logger = {
info: sinon.spy(),
debug: sinon.spy(),
warning: sinon.spy(),
error: sinon.spy(),
};

const store = new TaskStore({
callCluster,
getKibanaUuid,
logger,
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
});

await store.init();
const loggingCall = logger.warning.getCall(0);
expect(loggingCall.args[0]).toBe(
`This Kibana instance defines an older template version (${TEMPLATE_VERSION}) than is currently in Elasticsearch (Infinity). ` +
`Because of the potential for non-backwards compatible changes, this Kibana instance will only be able to claim scheduled tasks with ` +
`"kibana.apiVersion" <= ${API_VERSION} in the task metadata.`
);
expect(logger.warning.calledOnce).toBe(true);
});
});

describe('schedule', () => {
async function testSchedule(task: TaskInstance) {
const callCluster = sinon.spy(() =>
const callCluster = sinon.stub();
callCluster.withArgs('index').returns(
Promise.resolve({
_id: 'testid',
_version: 3344,
})
);
callCluster.withArgs('indices.getTemplate').returns(Promise.resolve({ tasky: {} }));
const store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['report', 'dernstraight', 'yawn'],
});
await store.init();
const result = await store.schedule(task);

sinon.assert.calledTwice(callCluster);
sinon.assert.calledThrice(callCluster);

return { result, callCluster, arg: callCluster.args[1][1] };
return { result, callCluster, arg: callCluster.args[2][1] };
}

test('serializes the params and state', async () => {
Expand All @@ -80,7 +127,7 @@ describe('TaskStore', () => {
});
});

test('retiurns a concrete task instance', async () => {
test('returns a concrete task instance', async () => {
const task = {
params: { hello: 'world' },
state: { foo: 'bar' },
Expand Down Expand Up @@ -119,6 +166,8 @@ describe('TaskStore', () => {
const callCluster = sinon.spy(async () => ({ hits: { hits } }));
const store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
Expand Down Expand Up @@ -283,6 +332,7 @@ describe('TaskStore', () => {
const callCluster = sinon.spy(async () => ({ hits: { hits } }));
const store = new TaskStore({
callCluster,
logger: mockLogger(),
supportedTypes: ['a', 'b', 'c'],
index: 'tasky',
maxAttempts: 2,
Expand All @@ -304,6 +354,8 @@ describe('TaskStore', () => {
const callCluster = sinon.spy(async () => ({ hits: { hits: [] } }));
const store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
supportedTypes: ['a', 'b', 'c'],
index: 'tasky',
maxAttempts: 2,
Expand Down Expand Up @@ -339,6 +391,7 @@ describe('TaskStore', () => {
{ terms: { 'task.taskType': ['foo', 'bar'] } },
{ range: { 'task.attempts': { lte: maxAttempts } } },
{ range: { 'task.runAt': { lte: 'now' } } },
{ range: { 'kibana.apiVersion': { lte: 1 } } },
],
},
},
Expand Down Expand Up @@ -432,6 +485,7 @@ describe('TaskStore', () => {
const runAt = new Date();
const task = {
runAt,
scheduledAt: runAt,
id: 'task:324242',
params: { hello: 'world' },
state: { foo: 'bar' },
Expand All @@ -444,6 +498,8 @@ describe('TaskStore', () => {
const callCluster = sinon.spy(async () => ({ _version: task.version + 1 }));
const store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
Expand Down Expand Up @@ -488,6 +544,8 @@ describe('TaskStore', () => {
);
const store = new TaskStore({
callCluster,
getKibanaUuid,
logger: mockLogger(),
index: 'myindex',
maxAttempts: 2,
supportedTypes: ['a'],
Expand Down
Loading