Move KibanaMigrator into Server SavedObjectsService #43433

rudolf · 2019-08-16T10:42:09Z

~~Note to reviewers: The code isn't 100% there yet and still has large blocks of commented out code, but I'd like some feedback on the approach before tidying up.~~

Edit: updated after conversations

Summary

This PR moves the KibanaMigrator out of the legacy KbnServer savedObjectsMixin into a new Core SavedObjectsService. The SavedObjecsService creates a KibanaMigrator and injects this migrator into the LegacyService/KbnServer.

Background

This unblocks the larger effort of exposing the Saved Objects client on the server-side which has the following dependency SavedObjectsClient -> SavedObjectsRepository -> KibanaMigrator.

Before this PR, KbnServer would load and initialize plugins before saved object migrations were completed. Migrations would run in the background while all Saved Object operations are buffered until the migrations are complete. This has sometimes led to subtle bugs where Elasticsearch write operations were performed against the .kibana index which would effectively break kibana or lead to data-loss. (Although plugins run in parallel with migrations, we would only listen to HTTP requests once migrations are complete.)

In contrast, the SavedObjectsService is started before the LegacyService (which initializes legacy plugins) and Http service and the start phase will block until all migrations have completed. This means no plugins can run and no HTTP requests will be received until migrations are completed.

Handling ES connectivity errors

Until we have a solution for #43456 the SavedObjectsService will retry any connectivity errors to elasticsearch indefinitely. Although we no longer rely on this logic being embedded in the Elasticsearch plugin's healthcheck, the behaviour stays the same:

When kibana is started without any Elasticsearch connection:

Kibana starts the "notReadyServer", but the HTTP server doesn't yet serve any routes.

When Elasticsearch becomes unavailable after Kibana had successfully started

We display the "Cannot connect to the Elasticsearch cluster. See the Kibana logs for details and try reloading the page." page on the Kibana UI.

`migrations.skip` configuration option

To perform migrations, we need an active connection to Elasticsearch. However, for building (--optimize and testing (grunt run:testBrowser) we run the Kibana server without an Elasticsearch cluster but still depend on the HTTP server to start up. This would be done by disabling the legacy Elasticsearch plugin which in turn would disable migrations so that startup isn't blocked on migrations.

Since in the NP we can no longer rely on disabling the Elasticsearch plugin, I introduced the migrations.skip configuration option.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

joshdover · 2019-08-22T20:50:21Z

The behavior here sounds good, though I haven't tested it myself yet.

My main questions are related to the --skip-migrations flag. When possible, I think we should try to reduce the number of configs & cli flags to reduce the combinational complexity of configurations. I'm curious if there's a single semantic that conveys the different cases that need to use this flag and other existing options.

Do you know why grunt run:testBrowser needs the HTTP server? Which endpoints does it use or is it just serving static files?

rudolf · 2019-08-22T20:54:33Z

It's just used as a file server as far as I can tell. I find our build system a rather scary black box, so I don't know how much work this would be, but conceptually it feels like we shouldn't need to boot all of Kibana to optimize our frontend bundles or server unit testing bundles, so perhaps there's a way to untangle these from KbnServer.

walterra

ML changes LGTM

rudolf · 2019-08-23T14:13:45Z

We need to support two behaviours:

Run Optimizer and exit, ran as part of the build process. This is already supported with the --optimize flag. Since the optimizer also optimizes plugin bundles it requires scanning for plugins.
Compile tests bundle and serve tests bundle to browser. The test bundle is created in the tests plugin https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/tests_bundle/index.js#L102 and then we use '--optimize.bundleFilter=tests' so that this is the only bundle being served https://github.com/elastic/kibana/blob/master/tasks/config/run.js#L61

Instead of introducing a new --skip-migrations flag which describes what happens, but not why, we could create a --serve-tests-bundle (or something similar) flag which better expresses the intention that we're starting up a reduced functionality Kibana for the single purpose of building and serving test files. I'm not sure how much of a difference this would make, but we could later theoretically use this flag to disable more parts of Core to speed up this testing server.

The other option would be to decouple this from Core/KbnServer completely and create a lightweight file server that uses the PluginService in a standalone way to collect pluginSpecs and compile the test bundles.

@elastic/kibana-operations Do you have any feedback or suggestions?

joshdover · 2019-08-23T19:48:55Z

Is this ready for review? I noticed some WIP code in the service class: https://github.com/elastic/kibana/pull/43433/files#diff-e77c8c0ea344aeee10b656f96c2a3f4aR123

elasticmachine · 2019-09-27T20:07:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-27T20:34:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mshustov · 2019-09-30T16:01:46Z

src/core/server/config/config_service.mock.ts

+const createConfigServiceMock = ({
+  atPath = {},
+  getConfig$ = {},
+}: { atPath?: unknown; getConfig$?: Record<string, any> } = {}) => {


with this approach atPath loses type safety. const configService = configServiceMock.create({ atPath: 1 });

elasticmachine · 2019-09-30T21:16:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

gmmorris

Unless I'm mistaken the only changes you need reviewed by Stack Services are this single line change, right?

If so, LGTM :)
It seems to make sense from the context.

* Rename SavedObjectsService -> SavedObjectsLegacyService * Expose legacy pluginSpecs from Core LegacyService * Expose legacy uiExports from Core LegacyService * Move kibana config to NP * Expose pluginExtendedConfig from LegacyService * Make KibanaMigrator NP compatible * KibanaMigrator -> NP SavedObjectsService * SavedObjectsService never stop retrying ES connection error * Move waiting for migrations to complete till after legacy service start * Fix ESArchiver's KibanaMigrator * Fix reload logging config tests * Run migrations on savedobjects start * Fix env tests * Fix and make legacy tests more robust/isolated * Cleanup code * Fix invalid config test * Fix SavedObject Migrations logging test * SavedObjectsService tests * Lifecycle logging and improve getting kibanaConfig instance * Fix awaitMigration bug and test * Fix typing error * Review comments * Remove unecessary KibanaConfig class * Move legacy plugin config extension, specs, uiExports entirely into Core uiExports, specs, disabledSpecs, config now get injected into KbnServer * Fix config deprecation test * Use existing logger mock * Create SavedObjectsConfig for migration config * Define KibanaMigratorContract type * KibanaMigratorContract -> IKibanaMigrator + docs improvements * Fix esArchiver's KibanaMigrator * Fix plugin generator integration test * ConfigServiceContract -> IConfigService * Address review comments * Review nits * Document migrations.skip config * Review comments continued... * awaitMigrations -> runMigrations * Type improvements

elasticmachine · 2019-11-21T13:02:10Z

Pinging @elastic/kibana-platform (Team:Platform)

Thanks to #43433 being merged we no longer need to wait for the migrations to run as they are guaranteed to have run by the time plugin init has completed. This is just a cleanup making it easier to move towards the migration to the Kibana Platform.

Thanks to elastic#43433 being merged we no longer need to wait for the migrations to run as they are guaranteed to have run by the time plugin init has completed. This is just a cleanup making it easier to move towards the migration to the Kibana Platform.

rudolf force-pushed the server-saved-objects-service branch 2 times, most recently from cd9ea4d to f7cea53 Compare August 20, 2019 14:04

rudolf added 12 commits August 22, 2019 14:09

Rename SavedObjectsService -> SavedObjectsLegacyService

4b85ee9

Expose legacy pluginSpecs from Core LegacyService

6263726

Expose legacy uiExports from Core LegacyService

13301e2

Move kibana config to NP

19c9806

Expose pluginExtendedConfig from LegacyService

728e97d

Make KibanaMigrator NP compatible

b36daf3

KibanaMigrator -> NP SavedObjectsService

4458c26

SavedObjectsService never stop retrying ES connection error

6e1f026

Move waiting for migrations to complete till after legacy service start

86b6c62

Fix ESArchiver's KibanaMigrator

b2cfb14

Fix reload logging config tests

6fea102

Run migrations on savedobjects start

ea6486a

rudolf force-pushed the server-saved-objects-service branch from b70eff5 to ea6486a Compare August 22, 2019 12:10

rudolf marked this pull request as ready for review August 22, 2019 13:30

rudolf requested review from a team as code owners August 22, 2019 13:30

rudolf added the release_note:skip Skip the PR/issue when compiling release notes label Aug 22, 2019

rudolf changed the title ~~Server saved objects service~~ Move KibanaMigrator into Server SavedObjectsService Aug 22, 2019

Fix env tests

4b2e03e

Fix and make legacy tests more robust/isolated

4c6f865

walterra approved these changes Aug 23, 2019

View reviewed changes

rudolf added 3 commits September 27, 2019 19:01

Document migrations.skip config

a214e44

Review comments continued...

38adb1a

awaitMigrations -> runMigrations

86c9eef

rudolf requested a review from a team September 27, 2019 19:25

mshustov reviewed Sep 30, 2019

View reviewed changes

mshustov approved these changes Sep 30, 2019

View reviewed changes

mikecote requested a review from gmmorris September 30, 2019 17:18

rudolf added 2 commits September 30, 2019 21:11

Merge branch 'master' into server-saved-objects-service

6e39936

Type improvements

6980d6a

epixa mentioned this pull request Sep 30, 2019

Migrate saved object service #33587

Closed

20 tasks

joshdover mentioned this pull request Sep 30, 2019

Move KibanaConfig to Core #46280

Closed

7 tasks

gmmorris approved these changes Oct 1, 2019

View reviewed changes

rudolf merged commit 85c8232 into elastic:master Oct 1, 2019

rudolf deleted the server-saved-objects-service branch October 1, 2019 07:11

rudolf mentioned this pull request Oct 1, 2019

[7.x] Move KibanaMigrator into Server SavedObjectsService (#43433) #47011

Merged

mshustov mentioned this pull request Nov 15, 2019

Invalid version check doesn't work for Legacy plugins #50785

Closed

rudolf mentioned this pull request Nov 21, 2019

Neglecting to set elasticsearch.username and elasticsearch.password in kibana.yml causes kibana to throw fatal error #51242

Closed

epixa added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Nov 21, 2019

gmmorris mentioned this pull request Nov 22, 2019

[Task Manager] removes await migrations as it is no longer needed #51422

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move KibanaMigrator into Server SavedObjectsService #43433

Move KibanaMigrator into Server SavedObjectsService #43433

rudolf commented Aug 16, 2019 •

edited

Loading

joshdover commented Aug 22, 2019

rudolf commented Aug 22, 2019

walterra left a comment

rudolf commented Aug 23, 2019

joshdover commented Aug 23, 2019

elasticmachine commented Sep 27, 2019

elasticmachine commented Sep 27, 2019

mshustov Sep 30, 2019

elasticmachine commented Sep 30, 2019

gmmorris left a comment

elasticmachine commented Nov 21, 2019

Move KibanaMigrator into Server SavedObjectsService #43433

Move KibanaMigrator into Server SavedObjectsService #43433

Conversation

rudolf commented Aug 16, 2019 • edited Loading

Summary

Background

Handling ES connectivity errors

migrations.skip configuration option

Checklist

For maintainers

joshdover commented Aug 22, 2019

rudolf commented Aug 22, 2019

walterra left a comment

Choose a reason for hiding this comment

rudolf commented Aug 23, 2019

joshdover commented Aug 23, 2019

elasticmachine commented Sep 27, 2019

💔 Build Failed

elasticmachine commented Sep 27, 2019

💔 Build Failed

mshustov Sep 30, 2019

Choose a reason for hiding this comment

elasticmachine commented Sep 30, 2019

💚 Build Succeeded

gmmorris left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 21, 2019

rudolf commented Aug 16, 2019 •

edited

Loading

`migrations.skip` configuration option