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

[WIP] Store should not update template on every restart. #28611

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@
"ngreact": "0.5.1",
"no-ui-slider": "1.2.0",
"node-fetch": "1.3.2",
"object-hash": "^1.3.1",
"opn": "^5.4.0",
"oppsy": "^2.0.0",
"pegjs": "0.9.0",
Expand Down
23 changes: 23 additions & 0 deletions x-pack/plugins/task_manager/lib/template_properties.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* 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.
*/

export const templateProperties = {
type: { type: 'keyword' },
task: {
properties: {
taskType: { type: 'keyword' },
runAt: { type: 'date' },
interval: { type: 'text' },
attempts: { type: 'integer' },
status: { type: 'keyword' },
params: { type: 'text' },
state: { type: 'text' },
user: { type: 'keyword' },
scope: { type: 'keyword' },
timeout: { type: 'integer' },
},
},
};
3 changes: 2 additions & 1 deletion x-pack/plugins/task_manager/task_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export class TaskManager {
index: config.get('xpack.task_manager.index'),
maxAttempts: config.get('xpack.task_manager.max_attempts'),
supportedTypes: Object.keys(this.definitions),
logger,
});
const pool = new TaskPool({
logger,
Expand Down Expand Up @@ -94,7 +95,7 @@ export class TaskManager {
this.isInitialized = true;
})
.catch((err: Error) => {
logger.warning(err.message);
logger.warning('Poller: ' + err.message);

// rety again to initialize store and poller, using the timing of
// task_manager's configurable poll interval
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/task_manager/task_poller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ describe('TaskPoller', () => {
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
logger: mockLogger(),
});
});

Expand Down
18 changes: 15 additions & 3 deletions x-pack/plugins/task_manager/task_store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import _ from 'lodash';
import sinon from 'sinon';
import { TaskInstance, TaskStatus } from './task';
import { FetchOpts, TaskStore } from './task_store';
import { mockLogger } from './test_utils';

describe('TaskStore', () => {
describe('init', () => {
Expand All @@ -18,11 +19,16 @@ describe('TaskStore', () => {
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
logger: mockLogger(),
});

await store.init();

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

sinon.assert.calledWithMatch(callCluster, 'indices.getTemplate', {
name: 'tasky',
});

sinon.assert.calledWithMatch(callCluster, 'indices.putTemplate', {
body: {
Expand Down Expand Up @@ -50,12 +56,13 @@ describe('TaskStore', () => {
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['report', 'dernstraight', 'yawn'],
logger: mockLogger(),
});
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 Down Expand Up @@ -122,6 +129,7 @@ describe('TaskStore', () => {
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
logger: mockLogger(),
});

const result = await store.fetch(opts);
Expand Down Expand Up @@ -286,6 +294,7 @@ describe('TaskStore', () => {
supportedTypes: ['a', 'b', 'c'],
index: 'tasky',
maxAttempts: 2,
logger: mockLogger(),
...opts,
});

Expand All @@ -307,6 +316,7 @@ describe('TaskStore', () => {
supportedTypes: ['a', 'b', 'c'],
index: 'tasky',
maxAttempts: 2,
logger: mockLogger(),
});

const result = await store.fetchAvailableTasks();
Expand Down Expand Up @@ -447,6 +457,7 @@ describe('TaskStore', () => {
index: 'tasky',
maxAttempts: 2,
supportedTypes: ['a', 'b', 'c'],
logger: mockLogger(),
});

const result = await store.update(task);
Expand Down Expand Up @@ -491,6 +502,7 @@ describe('TaskStore', () => {
index: 'myindex',
maxAttempts: 2,
supportedTypes: ['a'],
logger: mockLogger(),
});
const result = await store.remove(id);

Expand Down
88 changes: 49 additions & 39 deletions x-pack/plugins/task_manager/task_store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
* This module contains helpers for managing the task manager storage layer.
*/

import hash from 'object-hash';
import { TaskManagerLogger } from './lib/logger';
import { templateProperties } from './lib/template_properties';
import { ConcreteTaskInstance, ElasticJs, TaskInstance, TaskStatus } from './task';

const DOC_TYPE = '_doc';
Expand All @@ -17,6 +20,7 @@ export interface StoreOpts {
index: string;
maxAttempts: number;
supportedTypes: string[];
logger: TaskManagerLogger;
}

export interface FetchOpts {
Expand Down Expand Up @@ -64,11 +68,15 @@ export interface RawTaskDoc {
* interface into the index.
*/
export class TaskStore {
get isInitialized() {
return this._isInitialized;
}
public readonly maxAttempts: number;
private callCluster: ElasticJs;
private index: string;
private supportedTypes: string[];
private _isInitialized = false; // tslint:disable-line:variable-name
private logger: TaskManagerLogger;

/**
* Constructs a new TaskStore.
Expand All @@ -83,6 +91,7 @@ export class TaskStore {
this.index = opts.index;
this.maxAttempts = opts.maxAttempts;
this.supportedTypes = opts.supportedTypes;
this.logger = opts.logger;

this.fetchAvailableTasks = this.fetchAvailableTasks.bind(this);
}
Expand All @@ -103,51 +112,52 @@ export class TaskStore {
throw new Error('TaskStore has already been initialized!');
}

const properties = {
type: { type: 'keyword' },
task: {
properties: {
taskType: { type: 'keyword' },
runAt: { type: 'date' },
interval: { type: 'text' },
attempts: { type: 'integer' },
status: { type: 'keyword' },
params: { type: 'text' },
state: { type: 'text' },
user: { type: 'keyword' },
scope: { type: 'keyword' },
},
},
};

let shouldUpdate = true;
let existingTemplate;
try {
const templateResult = await this.callCluster('indices.putTemplate', {
existingTemplate = await this.callCluster('indices.getTemplate', {
name: this.index,
body: {
index_patterns: [this.index],
mappings: {
_doc: {
dynamic: 'strict',
properties,
},
},
settings: {
number_of_shards: 1,
auto_expand_replicas: '0-1',
},
},
});
this._isInitialized = true;
return templateResult;
} catch (err) {
throw err;

if (existingTemplate) {
shouldUpdate =
hash(existingTemplate[this.index].mappings._doc.properties) !== hash(templateProperties);
Copy link
Member

Choose a reason for hiding this comment

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

This does prevent overwriting it upon every restart, but it also means that if an older version happens to be restarted, then it will trample a newer version's template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see this as an issue. Because index templates don't change existing indices. If you delete the index, it will then get created with the new mappings from the older templated version but before task manager would run and index a task it would update the template.

We could double check the mappings once per run before indexing or handle indexing error appropriately to determine the index was changed and doesn't match.

Copy link
Member

@tsullivan tsullivan Jan 14, 2019

Choose a reason for hiding this comment

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

I think @pickypg is right in that updates need to be controlled in that they must either install the template from scratch, upgrade it, or do nothing. If they can't upgrade it, they'll potentially break things in ways that will be hard to troubleshoot for the user: the outcome of the problem will be some kind of TypeError when an old instance of Kibana claims a new task and fields it depends on have moved.

Consider:

  1. Kibana 0.1.0 starts up, installs the template from scratch
  2. Kibana 1.0.0 starts up, overwrites the template
  3. Kibana 1.0.0 schedules a task
  4. Kibana 0.1.0 claims the task
  5. Boom.

Edit: to add more thoughts, the original PR we're working on handles this in two ways:

  1. Numeric versioning ensures old templates do not overwrite new templates, provides helpful log messaging to get users to upgrade
  2. API versioning ensures older instances are only able to claim tasks they understand

if (shouldUpdate) {
this.logger.info('Found different index template, it will be updated!');
}
}
} catch (e) {
if (e.message !== 'Not Found') {
this.logger.error(`Could not determine state of index template ${e.message}`);
}
}

return;
}
if (shouldUpdate) {
try {
const templateResult = await this.callCluster('indices.putTemplate', {
name: this.index,
body: {
index_patterns: [this.index],
mappings: {
_doc: {
dynamic: 'strict',
properties: templateProperties,
},
},
settings: {
number_of_shards: 1,
auto_expand_replicas: '0-1',
},
},
});
this._isInitialized = true;
return templateResult;
} catch (err) {
throw err;
}
}

get isInitialized() {
return this._isInitialized;
return existingTemplate;
}

/**
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -15633,6 +15633,11 @@ object-copy@^0.1.0:
define-property "^0.2.5"
kind-of "^3.0.3"

object-hash@^1.3.1:
version "1.3.1"
resolved "https://registry.yarnpkg.com/object-hash/-/object-hash-1.3.1.tgz#fde452098a951cb145f039bb7d455449ddc126df"
integrity sha512-OSuu/pU4ENM9kmREg0BdNrUDIl1heYa4mBZacJc+vVWz4GtAwu7jO8s4AIt2aGRUTqxykpWzI3Oqnsm13tTMDA==

object-inspect@~0.4.0:
version "0.4.0"
resolved "https://registry.yarnpkg.com/object-inspect/-/object-inspect-0.4.0.tgz#f5157c116c1455b243b06ee97703392c5ad89fec"
Expand Down