Revamp core environment class to support upcoming core<-->legacy bootstrap inversion. by azasypkin · Pull Request #21885 · elastic/kibana

azasypkin · 2018-08-10T16:04:17Z

After #19994 env will store some additional data (e.g. cliArgs) that are required by the "legacy" Kibana, so in this PR I revamped env class to support that.

I'm also experimenting with the new way of passing data (env.legacy broadcast channel) from almost any place in the core to the legacy world: e.g. connection options from Root -> Server -> HttpModule -> HttpService -> HttpServer or anything else we may need in the future to glue core and the "legacy" Kibana.

I found env.legacy to be the easiest way to do that, not ideal though. So if you have any better ideas I'm all ears!

~~Blocked by #21879~~
Unblocks #19994

azasypkin · 2018-08-10T16:17:42Z

src/core/server/config/env.ts

note: Kibana can rely on multiple configs at the same time (content will be merged).

azasypkin · 2018-08-10T16:17:56Z

src/core/server/config/env.ts

note: taken from

kibana/src/server/config/config.js

Lines 110 to 111 in 304a9f6

buildNum: IS_KIBANA_DISTRIBUTABLE ? pkg.build.number : Number.MAX_SAFE_INTEGER,

buildSha: IS_KIBANA_DISTRIBUTABLE ? pkg.build.sha : 'XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX'

azasypkin · 2018-08-10T16:20:54Z

src/core/server/http/http_server.ts

note: we don't use these options in this PR yet, but we'll need them in the upcoming PRs (these are being passed to the legacy Hapi server so that we don't duplicate connection options like that https://github.com/elastic/kibana/blob/master/src/server/http/index.js#L33-L61 anymore).

azasypkin · 2018-08-10T16:25:02Z

src/core/server/legacy_compat/index.ts

note: in the upcoming PRs, it will be detected like that: https://github.com/elastic/kibana/pull/19994/files#diff-806634e0e6f42d8ea73cd3565c9089cbR34 (and this function will go away completely)

azasypkin · 2018-08-10T16:28:14Z

src/core/server/legacy_compat/legacy_platform_proxifier.ts

note: I added few more events (e.g. upgrade) to support socket.io functionality that Canvas uses.

azasypkin · 2018-08-10T16:28:49Z

src/core/server/legacy_compat/legacy_platform_proxifier.ts

note: this will be moved into legacy_service in the upcoming PRs.

…tstrap inversion.

azasypkin · 2018-08-20T12:45:48Z

src/core/server/config/env.ts

+   * @internal
+   */
+  constructor(readonly homeDir: string, options: EnvOptions) {
+    this.configDir = resolve(this.homeDir, 'config');


note: these corePluginsDir, logDir and staticFilesDir are properties that we can get rid of for now and add back when we really need them, I just preserved them since new-platform branch. Let me know if you want me to remove them.

elasticmachine · 2018-08-20T14:30:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-20T14:40:54Z

This one should be ready for review @spalger. It's getting closer to the point when we invert the bootstrap process and core will be started first hence you may notice some unused methods that will be needed soon, just didn't manage to make #19994 split cleaner .

spalger · 2018-08-20T20:44:22Z

👍 taking a look now

spalger

Couple questions, but I'm sure you have reasons for everything that's happening here that will make more sense once we get closer to merging #19994

spalger · 2018-08-20T20:45:33Z

src/core/server/config/__tests__/config_service.test.ts

 */

 /* tslint:disable max-classes-per-file */
+


While you're in here, would you mind moving these test files out of the __tests__ directory? These should be right next to the files they are testing right?

These should be right next to the files they are testing right?

Correct, __tests__ are still used everywhere in src/core/server as it was in new-platform branch. I was planning to get rid of all __tests__ from src/core/server in a dedicated PR, but can do it gradually in every PR that touches tests, like here. Will remove all __tests__ that I touch in this PR.

Oh, no, I realized it will be a rebase nightmare for me since I have 3 more PRs based on this one that change these tests :/ Would you mind I prepare a dedicated PR to get rid of all __tests__ in src/core/server right after #22190?

works for me!

spalger · 2018-08-20T20:48:30Z

src/core/server/config/env.ts

-    this.logDir = resolve(this.homeDir, 'log');
-    this.staticFilesDir = resolve(this.homeDir, 'ui');
-  }
+  public readonly packageInfo: Readonly<PackageInfo>;


Do you think maybe we want to use RecursiveReadOnly and something like https://github.com/elastic/kibana/blob/743edc6c0e696f861aab98fd3172619914459a70/src/core/public/injected_metadata/deep_freeze.ts?

Hmm, we have only one level of nesting here, but we can still use RecursiveReadOnly and deep_freeze. How do you see sharing this function between server and public?

It's been pretty common for plugins to use /public, /server, and /common directories for this type of thing... What do you think @epixa?

Okay, I don't want to block on this one for too long, so I moved deep_freeze into src/core/utils that we have already for other similar things and started to use it src/core/server/config/env. We can rename it to common or whatever once/if we want to in the future PRs without any problem.

Hah, sharing code between client and server turned out to be not that easy as I thought :) Ugh

We discussed that with @spalger on Slack and agreed that it'd make sense to work on a proper solution for the code that is shared between core server and core public in a separate PR. So I've reverted my latest commit.

spalger · 2018-08-20T20:50:08Z

src/core/server/config/env.ts

+      prod: !this.cliArgs.dev,
+    });
+
+    const isKibanaDistributable = pkg.build && pkg.build.distributable === true;


We'll need env.isDistributable at some point, but I guess we can expose it once we need it.

Yep, let's expose it when we need it.

spalger · 2018-08-20T20:51:08Z

src/core/server/config/env.ts


-  private getDefaultConfigFile() {
-    return resolve(this.configDir, 'kibana.yml');
+    this.legacy = new EventEmitter();


this will be moved into legacy_service in the upcoming PRs.

I'm pretty sure that means this legacy "chanel" is temporary, right? When we get rid of it will the LegacyService just be able to talk to the HttpService and get a promise/observable for the server connection rather than using an EventEmitter on the Env?

Yes, my plan is to get rid of it in #22190. I still don't like any approach to be honest since I don't want to pass internal server reference around and be a part of any public contract :/ But there is no other choice for now.

spalger · 2018-08-20T21:26:30Z

src/core/server/config/__tests__/env.test.ts

+    isDevClusterMaster: true,
+  });
+
+  expect(Object.entries(defaultEnv)).toMatchSnapshot('env properties');


Why can't this just check expect(defaultEnv).toMatchSnapshot()? Doesn't Object.entries() make this sensitive to property ordering? I certainly think it makes the snapshots harder to grok. If it has to do with hidden properties or something maybe expect({ ...defaultEnv }).toMatchSnapshot() would be a slightly better choice?

Why can't this just check expect(defaultEnv).toMatchSnapshot()?

Hmm, we can, I have no idea why I have these Object.entries 🙈

spalger · 2018-08-20T21:35:42Z

src/core/server/legacy_compat/__tests__/legacy_platform_proxifier.test.ts


    expect(listener).toHaveBeenCalledTimes(2);
    expect(listener).toHaveBeenCalledWith(1, 2, 3, 4);
-    expect(listener).toHaveBeenCalledWith(5, 6, 7, 8);


Did you mean to remove this assertion?

Hah, good catch!

spalger · 2018-08-20T21:39:38Z

src/core/server/legacy_compat/legacy_platform_proxifier.ts

        return [
          eventName,
          (...args: any[]) => {
+            // Since it's just a proxy and the underlying server is managed by the core


Hmm, what inspired this change? I feel like the default behavior of falling over when something isn't listening for the error event is generally desired, but this now listens for that event and swallows it if nothing else is listening for it...

I think I was hit by this during testing/development at some point, when core http server failed before legacy Kibana subscribed to the proxifier events. So the original error has been handled by Hapi 17 and core, but nothing was listening for the "forwarded" error event so we got unhandled exception.

Legacy Kibana only attaches error handler when Hapi starts to listen so it may take some time.

What do you think?

I think I need more time to think this through, and maybe we could chat about it, but I think if something fails before legacy Kibana server attaches its error handler we probably shouldn't be swallowing it and ignoring it.

Okay, I'll remove it then and we'll see how it goes.

Removed ✔️ We can get back to it if it becomes a problem.

elasticmachine · 2018-08-21T08:56:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-21T10:04:32Z

retest

elasticmachine · 2018-08-21T11:17:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-22T00:38:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-22T05:01:29Z

retest

elasticmachine · 2018-08-22T06:41:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-22T06:56:02Z

Have no idea why x-pack job is failing with 07:35:44 │ proc [ftr] ERROR Error: remote failed to start within 2 minutes - will try to figure that out

elasticmachine · 2018-08-22T09:12:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-22T11:00:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

… should be frozen.

elasticmachine · 2018-08-22T15:22:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ops that should be frozen.

elasticmachine · 2018-08-22T18:49:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-23T04:55:46Z

I think PR is ready for another pass @spalger, thanks!

spalger

LGTM

azasypkin · 2018-08-24T11:05:40Z

6.x/6.5: 35c081c

azasypkin added blocked Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// Feature:New Platform v6.5.0 labels Aug 10, 2018

azasypkin mentioned this pull request Aug 10, 2018

[skip ci] Use new platform CLI as the main entry point #19994

Closed

22 tasks

azasypkin commented Aug 10, 2018

View reviewed changes

azasypkin changed the title ~~[skip ci] Revamp core environement class to support upcoming core<-->legacy bootstrap inversion.~~ [skip ci] Revamp core environment class to support upcoming core<-->legacy bootstrap inversion. Aug 14, 2018

azasypkin mentioned this pull request Aug 14, 2018

Make core responsible for reading and merging of config files. Simplify legacy config adapter. #21956

Merged

azasypkin force-pushed the issue-xxx-core-env branch from e9921c5 to 5816f36 Compare August 17, 2018 14:41

Revamp core environement class to support upcoming core<-->legacy boo…

be9aa6c

…tstrap inversion.

azasypkin changed the title ~~[skip ci] Revamp core environment class to support upcoming core<-->legacy bootstrap inversion.~~ Revamp core environment class to support upcoming core<-->legacy bootstrap inversion. Aug 20, 2018

azasypkin force-pushed the issue-xxx-core-env branch from 5816f36 to be9aa6c Compare August 20, 2018 12:43

azasypkin removed the blocked label Aug 20, 2018

azasypkin commented Aug 20, 2018

View reviewed changes

azasypkin requested a review from spalger August 20, 2018 14:39

spalger reviewed Aug 20, 2018

View reviewed changes

azasypkin added 2 commits August 21, 2018 08:46

Do not use Object.entries in env tests.

67a1f43

Remove missing test assertion.

557b3ac

Merge branch 'master' into issue-xxx-core-env

6a2a072

Remove special handling of error even in legacy platform proxifier.

9699d03

Use utils/deep_freeze instead of Object.freeze for env props that…

32c73ce

… should be frozen.

Revert: Use utils/deep_freeze instead of Object.freeze for env pr…

f555fe2

…ops that should be frozen.

spalger approved these changes Aug 23, 2018

View reviewed changes

azasypkin merged commit 60ae2f5 into elastic:master Aug 23, 2018

azasypkin deleted the issue-xxx-core-env branch August 23, 2018 20:31

azasypkin mentioned this pull request Aug 23, 2018

[6.x] Revamp core environment class to support upcoming core<-->legacy bootstrap inversion. (#21885) #22333

Merged

azasypkin added the backport pending label Aug 23, 2018

azasypkin added backported and removed backport pending labels Aug 24, 2018

	buildNum: IS_KIBANA_DISTRIBUTABLE ? pkg.build.number : Number.MAX_SAFE_INTEGER,
	buildSha: IS_KIBANA_DISTRIBUTABLE ? pkg.build.sha : 'XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX'

Conversation

azasypkin commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 20, 2018

💚 Build Succeeded

Uh oh!

azasypkin commented Aug 20, 2018

Uh oh!

spalger commented Aug 20, 2018

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Aug 10, 2018 •

edited

Loading

azasypkin Aug 10, 2018 •

edited

Loading

azasypkin Aug 21, 2018 •

edited

Loading

azasypkin Aug 22, 2018 •

edited

Loading

spalger Aug 22, 2018 •

edited

Loading