[savedObjects] Use index template #14271

spalger · 2017-10-03T18:03:18Z

Remove Kibana index creation from the health check
Only patchKibanaIndex() when it exists
Convert the SavedObjectsClient's onBeforeWrite() handler so that every write PUTs the index template to elasticsearch
Respond with a special 503 when SavedObjectsClient#create fails because of action.auto_create_index: false
Render informational error when action.auto_create_index: false prevents Kibana from being able to saved objects

The elasticsearch plugin currently checks for the Kibana index on each iteration of the healthCheck, and creates it if it does not exist. This removes that step from the healthCheck and instead, before each savedObject is written to elasticsearch, ensures that Elasticsearch has the necessary index template should the write result in index creation. The healthCheck still has the `patchKibanaIndex()` logic, which checks the type in the Kibana index and adds any missing types. This step now does nothing when the Kibana index does not exist, and does what it has always done when it does.

tylersmalley · 2017-10-31T14:52:25Z

Looks like the tests are failing for no such index.

…ana-index-template

(cherry picked from commit b1ef897dafeb6d43fe279776e44a9d793a389dc3)

azasypkin · 2017-11-03T14:14:33Z

src/core_plugins/elasticsearch/lib/health_check.js

 import kibanaVersion from './kibana_version';
 import { ensureEsVersion } from './ensure_es_version';
 import { ensureNotTribe } from './ensure_not_tribe';
 import { patchKibanaIndex } from './patch_kibana_index';
+import { format } from 'util';


optional nit: I know it was here before (hence up to you), but it seems the only place it's used in is:

plugin.status.red(format('Unable to connect to Elasticsearch at %s.', url));

that is basically just

plugin.status.red(`Unable to connect to Elasticsearch at ${url}`);

azasypkin · 2017-11-03T14:18:45Z

src/core_plugins/elasticsearch/lib/__tests__/health_check.js

          sinon.assert.notCalled(plugin.status.red);
          sinon.assert.calledOnce(plugin.status.green);

-          expect(plugin.status.green.args[0][0]).to.be('Kibana index ready');
+          expect(plugin.status.green.args[0][0]).to.be('Ready');


nit: calledWith here and in the test below? :)

azasypkin · 2017-11-03T14:22:20Z

src/core_plugins/elasticsearch/lib/create_kibana_index.js

@@ -1,25 +0,0 @@
-export default function (server) {


question: I couldn't find any comments about this "feature", but just in general why did we need this previously and why we don't need this anymore? Just for my understanding.

What I call the "saved objects index" used to be the "Kibana index", which needed to be created before the first write or elasticsearch would automatically create the index with automatic (and likely incorrect) mappings.

This part of the health checker was responsible for doing that, and we would run it on an interval, but this new strategy teaches elasticsearch how to automatically create the index correctly instead.

Got it, thanks

azasypkin · 2017-11-03T14:32:46Z

src/core_plugins/elasticsearch/lib/patch_kibana_index.js

@@ -25,6 +25,12 @@ export async function patchKibanaIndex(options) {

  const rootEsType = getRootType(kibanaIndexMappingsDsl);
  const currentMappingsDsl = await getCurrentMappings(callCluster, indexName, rootEsType);
+
+  // patchKibanaIndex() should do nothing if there are no current mappings


nit: would you mind adding a simple unit test to make sure we handle 404 properly and don't fail unexpectedly?

azasypkin · 2017-11-03T14:48:29Z

src/server/saved_objects/client/lib/errors.js

@@ -82,6 +82,19 @@ export function isEsUnavailableError(error) {
 }


+// 503 - Unable to automatically create index because of action.auto_create_index setting
+const CODE_ES_AUTO_CREATE_INDEX_ERROR = 'SavedObjectsClient/autoCreateIndex';
+export function createEsAutoCreateIndexError() {


nit: it seems every error here has a corresponding unit test suit, so it would be great if you can add one for this new error type as well.

azasypkin · 2017-11-03T14:54:18Z

jenkins, test this

azasypkin · 2017-11-03T15:14:27Z

src/server/saved_objects/saved_objects_mixin.js

@@ -30,30 +30,34 @@ export function savedObjectsMixin(kbnServer, server) {
    const adminCluster = server.plugins.elasticsearch.getCluster('admin');

    try {
-      await adminCluster.callWithInternalUser('cluster.health', {
-        timeout: `${server.config().get('savedObjects.indexCheckTimeout')}ms`,


nit: Seems like indexCheckTimeout is not used anymore, if so could you please remove it completely? I see it only in the config schema (it looks like it's the only property savedObjects config node) and in a single test.

azasypkin · 2017-11-03T15:29:02Z

src/server/saved_objects/saved_objects_mixin.js

+        },
+        err: {
+          message: error.message,
+          stack: error.stack,


nit: let's be consistent on the trailing commas within this file :)

So ready for #14631

azasypkin · 2017-11-03T16:08:18Z

src/server/saved_objects/saved_objects_mixin.js

-        index: server.config().get('kibana.index'),
-        waitForStatus: 'yellow',
+      const index = server.config().get('kibana.index');
+      await adminCluster.callWithInternalUser('indices.putTemplate', {


question: it must be a dumb question, but why do we want to put this template on every write? As far as I understand server.getKibanaIndexMappingsDsl() is supposed to return the same thing most of the time (always?) or am I missing something?

Not a dumb question. My intention was to ensure that Elasticsearch has the index template before every write so that even if a user deletes the index and index template in elasticsearch, writes will still work (unless the index template is deleted in the tiny amount of time between putting the index template and indexing the document).

Thanks for the explanation! Then optional nit: maybe you can add this comment to the code or even move this piece of code to a dedicated function like ensureKibanaIndexExists or something like that to make intention clearer (your comment above still seems to be useful to have there as well)?

azasypkin · 2017-11-03T16:55:34Z

src/ui/public/error_auto_create_index/error_auto_create_index.less

@@ -0,0 +1,3 @@
+.error-multi-allow-explicit-index {


nit: did you mean error-auto-create-index or something similar?

azasypkin · 2017-11-03T17:00:03Z

src/ui/public/saved_objects/saved_objects_client_provider.js

+
+  return new SavedObjectsClient({
+    $http,
+    basePath: chrome.getBasePath(),


nit: you can probably omit basePath property since it's the same as default one.

azasypkin · 2017-11-03T17:56:42Z

test/api_integration/apis/saved_objects/create.js

+
+            // loose ISO8601 UTC time with milliseconds validation
+            expect(resp.body).to.have.property('updated_at').match(/^[\d-]{10}T[\d:\.]{12}Z$/);
+
            expect(resp.body).to.eql({


optional nit: not really better, but just another way of using eql with object that has properties that aren't know upfront:

expect(resp.body).to.eql({ ...resp.body, type: 'visualization', version: 1, attributes: { title: 'My favorite vis' } });

yeah, I really wanted to list all expected properties

tylersmalley · 2017-11-08T00:41:22Z

Jenkins, test it

Failure:

18:10:04    └- ✖ fail: "console app "before all" hook"
18:10:04    │        [POST http://localhost:9515/session/29b51f08a0352993ba8f3f1f172fc37f/window/current/size / {"width":1200,"height":800}] disconnected: unable to connect to renderer
18:10:04    │         (Session info: chrome=62.0.3202.75)
18:10:04    │         (Driver info: chromedriver=2.32.498513

…ana-index-template

tylersmalley · 2017-11-09T00:42:07Z

Overall, looks good - just a couple things I came across:

In x-pack, when ES is unable I get 401 unauthorized

curl -X POST -H "Content-Type: application/json" -H "kbn-xsrf: true" -d '{ "attributes": { "title": "logstash-*" } }' http://elastic:changeme@localhost:5601/api/saved_objects/index-pattern
{"statusCode":401,"error":"Unauthorized”}

This differs from {"statusCode":503,"error":"Service Unavailable","message":"Service Unavailable”} in OS Kibana

Have we measured what effect putting the index template before each write has on object imports? Could it be debounced?

When starting Kibana without an index:

server    log   [00:32:54.002] [warning][kibana-monitoring][monitoring-ui] [index_not_found_exception] no such index, with { resource.type="index_or_alias" & resource.id=".kibana" & index_uuid="_na_" & index=".kibana" } :: {"path":"/.kibana/_search","query":{"filter_path":"aggregations.types.buckets"},"body":"{\"size\":0,\"query\":{\"bool\":{\"should\":[{\"term\":{\"type\":{\"value\":\"graph-workspace\"}}},{\"term\":{\"type\":{\"value\":\"timelion-sheet\"}}}]}},\"aggs\":{\"types\":{\"terms\":{\"field\":\"type\",\"size\":2}}}}","statusCode":404,"response":"{\"error\":{\"root_cause\":[{\"type\":\"index_not_found_exception\",\"reason\":\"no such index\",\"resource.type\":\"index_or_alias\",\"resource.id\":\".kibana\",\"index_uuid\":\"_na_\",\"index\":\".kibana\"}],\"type\":\"index_not_found_exception\",\"reason\":\"no such index\",\"resource.type\":\"index_or_alias\",\"resource.id\":\".kibana\",\"index_uuid\":\"_na_\",\"index\":\".kibana\"},\"status\":404}"}
    at respond (/Users/tyler/elastic/kibana/node_modules/elasticsearch/src/lib/transport.js:295:15)
    at checkRespForFailure (/Users/tyler/elastic/kibana/node_modules/elasticsearch/src/lib/transport.js:254:7)
    at HttpConnector.<anonymous> (/Users/tyler/elastic/kibana/node_modules/elasticsearch/src/lib/connectors/http.js:159:7)
    at IncomingMessage.bound (/Users/tyler/elastic/kibana/node_modules/elasticsearch/node_modules/lodash/dist/lodash.js:729:21)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickCallback (internal/process/next_tick.js:104:9)

tylersmalley

Couple questions/concerns with x-pack.

tylersmalley · 2017-11-09T15:38:03Z

I keep coming back to questioning if setting the index template before every write is too heavy, or necessary. What does it prevent against?

If they delete the Kibana index, then the index template set (on startup?) would define the mapping.
If they swap out the Elasticsearch instance behind it, then they will get dynamic mappings but they are also missing their data. Can we identify when this happens?

spalger · 2017-11-09T15:43:47Z

@tylersmalley I don’t think you should worry about it, the most expensive part of that request is probably serializing the JSON body. Any sort of conditional “do I need to put the index template?” logic would probably just muddy up the super simple implementation. That said, I’ll test the impact on object imports and see if it’s worth fixing

…ana-index-template

spalger · 2017-11-09T17:07:34Z

In x-pack, when ES is unable I get 401 unauthorized

curl -X POST -H "Content-Type: application/json" -H "kbn-xsrf: true" -d '{ "attributes": { "title": "logstash-*" } }' http://elastic:changeme@localhost:5601/api/saved_objects/index-pattern
{"statusCode":401,"error":"Unauthorized”}

This is the same behavior as master, although I think we should probably ensure it is a 500 instead.

spalger · 2017-11-09T17:56:56Z

Ran a test import with 110 saved objects and observed the difference between master/this branch. First thing I noticed is that writes are already "slow" because we wait for index refresh.

In master it took 20.31 seconds and in this branch it to 20.11 seconds, the variance is really just random based on the time it takes to refresh the es index.

tylersmalley · 2017-11-14T21:33:04Z

src/ui/ui_settings/ui_settings_service.js

@@ -99,6 +99,7 @@ export class UiSettingsService {
    try {
      await this._savedObjectsClient.update(this._type, this._id, changes);
    } catch (error) {
+      debugger;


Did you mean to commit this?

…ana-index-template

jbudz

LGTM

I tested swapping out the kibana.index, and using aliases along with a bunch of general crud operations. I think the template will give us a softer mapping failure in cases when we previously would try to patch, so 👍 to that.

I'm iffy with pushing the template on so many requests but I'm rationalizing it's probably cheaper than the polling health check over time.

* [es][savedObjects/index] put template on each savedObject write The elasticsearch plugin currently checks for the Kibana index on each iteration of the healthCheck, and creates it if it does not exist. This removes that step from the healthCheck and instead, before each savedObject is written to elasticsearch, ensures that Elasticsearch has the necessary index template should the write result in index creation. The healthCheck still has the `patchKibanaIndex()` logic, which checks the type in the Kibana index and adds any missing types. This step now does nothing when the Kibana index does not exist, and does what it has always done when it does. * [ftr] remove unused kibanaIndex service (cherry picked from commit b1ef897dafeb6d43fe279776e44a9d793a389dc3) * [savedObjects/integration] create now creates kibana index * [es/healthCheck] remove use of format() * [es/healthCheck/tests] use sinon assertions * [es/patchKibanaIndex] test for kibana index missing behavior * [savedObjects/errors] add tests for EsAutoCreateIndexError * [savedObjects/config] deprecate and remove savedObjects.indexCheckTimeout config * use dangling commas consistently * [ui/error_auto_create_index] fix class names * [ui/savedObjectsClient] no need to specify basePath * [eslint] fix linting issue

rhoboat · 2018-01-30T21:46:03Z

What about the idea of adding an empty ingest pipeline to .kibana index, to make it future-proof? This could be really cool. We can always help people upgrade their old indices if we had this.

spalger added blocked v6.1.0 v7.0.0 WIP Work in progress Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Oct 3, 2017

spalger changed the title ~~[savedObjects] Use template for savedObjects index~~ [savedObjects] Use index template Oct 3, 2017

spalger force-pushed the implement/kibana-index-template branch from 531dec1 to 10e920a Compare October 30, 2017 20:14

spalger requested review from tylersmalley and azasypkin October 30, 2017 20:19

spalger added review and removed blocked WIP Work in progress labels Oct 30, 2017

spalger added 2 commits October 31, 2017 13:14

Merge branch 'master' of github.com:elastic/kibana into implement/kib…

8720ebd

…ana-index-template

[ftr] remove unused kibanaIndex service

825f2e9

(cherry picked from commit b1ef897dafeb6d43fe279776e44a9d793a389dc3)

spalger force-pushed the implement/kibana-index-template branch from b1ef897 to 825f2e9 Compare October 31, 2017 20:15

[savedObjects/integration] create now creates kibana index

7441994

azasypkin reviewed Nov 3, 2017

View reviewed changes

spalger added 2 commits November 7, 2017 10:57

[ui/error_auto_create_index] fix class names

3dd3a82

[ui/savedObjectsClient] no need to specify basePath

1450117

Merge branch 'master' of github.com:elastic/kibana into implement/kib…

90b27ed

…ana-index-template

tylersmalley suggested changes Nov 9, 2017

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into implement/kib…

63e1724

…ana-index-template

tylersmalley reviewed Nov 14, 2017

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into implement/kib…

e593bd9

…ana-index-template

spalger force-pushed the implement/kibana-index-template branch from c1c6942 to e593bd9 Compare November 14, 2017 23:02

spalger added 2 commits November 15, 2017 00:35

Merge branch 'master' of github.com:elastic/kibana into implement/kib…

23127eb

…ana-index-template

[eslint] fix linting issue

e9d9883

elastic deleted a comment from kimjoar Nov 21, 2017

Merge branch 'master' of github.com:elastic/kibana into implement/kib…

91eb9c3

…ana-index-template

jbudz approved these changes Nov 21, 2017

View reviewed changes

spalger merged commit 90e2aa0 into elastic:master Nov 22, 2017

spalger mentioned this pull request Nov 22, 2017

[6.x] [savedObjects] Use index template (#14271) #15103

Merged

tylersmalley mentioned this pull request Dec 4, 2017

Specify order for index template #15404

Closed

spalger deleted the implement/kibana-index-template branch February 9, 2018 22:44

elasticmachine mentioned this pull request Apr 25, 2018

[Security] API access with ES gone returns 401 #18323

Closed

cjcenizal mentioned this pull request Jul 8, 2018

Reactify route-based errors #20543

Closed

rudolf mentioned this pull request Apr 17, 2020

Remove auto_create_index / SavedObjectsClient/autoCreateIndex error and error view #63894

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[savedObjects] Use index template #14271

[savedObjects] Use index template #14271

spalger commented Oct 3, 2017 •

edited

Loading

tylersmalley commented Oct 31, 2017

azasypkin Nov 3, 2017

azasypkin Nov 3, 2017

azasypkin Nov 3, 2017 •

edited

Loading

spalger Nov 7, 2017

azasypkin Nov 14, 2017

azasypkin Nov 3, 2017

spalger Nov 7, 2017

azasypkin Nov 3, 2017

spalger Nov 7, 2017

azasypkin commented Nov 3, 2017

azasypkin Nov 3, 2017

spalger Nov 7, 2017

azasypkin Nov 3, 2017

spalger Nov 7, 2017 •

edited

Loading

azasypkin Nov 3, 2017

spalger Nov 7, 2017

azasypkin Nov 15, 2017

azasypkin Nov 3, 2017

azasypkin Nov 3, 2017

azasypkin Nov 3, 2017

spalger Nov 7, 2017

tylersmalley commented Nov 8, 2017

tylersmalley commented Nov 9, 2017

tylersmalley left a comment

tylersmalley commented Nov 9, 2017

spalger commented Nov 9, 2017 •

edited

Loading

spalger commented Nov 9, 2017

spalger commented Nov 9, 2017

tylersmalley Nov 14, 2017

spalger Nov 14, 2017

jbudz left a comment

rhoboat commented Jan 30, 2018

[savedObjects] Use index template #14271

[savedObjects] Use index template #14271

Conversation

spalger commented Oct 3, 2017 • edited Loading

tylersmalley commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Nov 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylersmalley commented Nov 8, 2017

tylersmalley commented Nov 9, 2017

tylersmalley left a comment

Choose a reason for hiding this comment

tylersmalley commented Nov 9, 2017

spalger commented Nov 9, 2017 • edited Loading

spalger commented Nov 9, 2017

spalger commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbudz left a comment

Choose a reason for hiding this comment

rhoboat commented Jan 30, 2018

spalger commented Oct 3, 2017 •

edited

Loading

azasypkin Nov 3, 2017 •

edited

Loading

spalger Nov 7, 2017 •

edited

Loading

spalger commented Nov 9, 2017 •

edited

Loading