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

Revamp core environment class to support upcoming core<-->legacy bootstrap inversion. #21885

Merged
merged 7 commits into from
Aug 23, 2018

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Aug 10, 2018

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 azasypkin added blocked Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v6.5.0 labels Aug 10, 2018
packageInfo: PackageInfo;
mode: EnvironmentMode;
[key: string]: any;
configs: string[];
Copy link
Member Author

Choose a reason for hiding this comment

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

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

const isKibanaDistributable = pkg.build && pkg.build.distributable === true;
this.packageInfo = Object.freeze({
branch: pkg.branch,
buildNum: isKibanaDistributable ? pkg.build.number : Number.MAX_SAFE_INTEGER,
Copy link
Member Author

Choose a reason for hiding this comment

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

note: taken from

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

// instance with connection options so that we can properly bridge core and
// the "legacy" Kibana internally.
this.env.legacy.emit('connection', {
options: serverOptions,
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

import { BehaviorSubject, k$, map } from '../../lib/kbn_observable';
import { Env } from '../config';
import { Root } from '../root';
import { BasePathProxyRoot } from '../root/base_path_proxy_root';

function initEnvironment(rawKbnServer: any) {
function initEnvironment(rawKbnServer: any, isDevClusterMaster = false) {
Copy link
Member Author

@azasypkin azasypkin Aug 10, 2018

Choose a reason for hiding this comment

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

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)

/**
* List of the server events to be forwarded to the legacy platform.
*/
const ServerEventsToForward = ['listening', 'error', 'clientError', 'connection'];
const ServerEventsToForward = [
Copy link
Member Author

Choose a reason for hiding this comment

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

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

// and server options that were used to create that server so that we can properly
// bridge with the "legacy" Kibana. If server isn't run (e.g. if process is managed
// by ClusterManager or optimizer) then this event will never fire.
this.env.legacy.once('connection', (connectionInfo: ConnectionInfo) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@azasypkin 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 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 azasypkin removed the blocked label Aug 20, 2018
* @internal
*/
constructor(readonly homeDir: string, options: EnvOptions) {
this.configDir = resolve(this.homeDir, 'config');
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

💚 Build Succeeded

@azasypkin azasypkin requested a review from spalger August 20, 2018 14:39
@azasypkin
Copy link
Member Author

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
Copy link
Contributor

spalger commented Aug 20, 2018

👍 taking a look now

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

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

@@ -18,8 +18,13 @@
*/

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@azasypkin azasypkin Aug 21, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

works for me!

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

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@azasypkin azasypkin Aug 22, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

prod: !this.cliArgs.dev,
});

const isKibanaDistributable = pkg.build && pkg.build.distributable === true;
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need env.isDistributable at some point, but I guess we can expose it once we need 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.

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


private getDefaultConfigFile() {
return resolve(this.configDir, 'kibana.yml');
this.legacy = new EventEmitter();
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

isDevClusterMaster: true,
});

expect(Object.entries(defaultEnv)).toMatchSnapshot('env properties');
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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


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

Choose a reason for hiding this comment

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

Did you mean to remove this assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, good catch!

@@ -50,12 +63,27 @@ export class LegacyPlatformProxifier extends EventEmitter {
return [
eventName,
(...args: any[]) => {
// Since it's just a proxy and the underlying server is managed by the core
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

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 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?

Copy link
Contributor

@spalger spalger Aug 22, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@azasypkin
Copy link
Member Author

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
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin
Copy link
Member Author

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

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@azasypkin
Copy link
Member Author

6.x/6.5: 35c081c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants