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

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

Closed
wants to merge 1 commit into from

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Jun 18, 2018

This PR serves as a sandbox for all the work that is being done for #19324, once it's ready some tasks may end up in separate PRs.


Tasks:

  • Integrate CLI skeleton code from new-platform branch into master
  • Add support for reading multiple config files and merging them into single config that can be processed by both core and "legacy" Kibana
  • Add support for CLI arguments that "legacy" CLI supports (via commander), but core CLI (via yargs) is still missing (e.g. oss, repl etc.)
  • Migrate from kbnServer.newPlatform object to kbnServer.core duplex broadcast channel (via EventEmitter) to make communication between core/legacy_compat and "legacy" KIbana easier (experimental, will need feedback)
  • Introduce LegacyService that will manage kbnServer life-cycle
  • The core should be able to merge CLI arguments with config values like we do in master
  • The core should be able to merge keystore content with config values like we do in master
  • Integrate core CLI with ClusterManager and get rid of hacks we had to introduce in BasePathProxy previously to overcome circular dependency between BasePathProxy and ClusterManager
  • Add support for optional CLI functionality (e.g. CAN_CLUSTER, XPACK_OPTIONAL etc.)
  • The core should respect server.autoListen = false used by the optimizer worker
  • Unify "normal" and "with base path proxy" bootstrap paths
  • Integrate core CLI with repl
  • The core should correctly process SIGINT/SIGTERM/SIGHUP and propagate that to the "legacy" Kibana when needed (e.g. applyLoggingConfiguration on SIGHUP)
  • Make core CLI (via yargs) --help output look similar as much as possible to what "legacy" CLI produces (via commander)
  • Move "legacy" Kibana CLI functionality we'll still rely on to src/legacy/cli (e.g. ClusterManager and repl)
  • Cover new functionality with Jest unit/integration tests and fix broken existing tests if any

Known issues or changes in behaviour:

  • When Kibana is run for the first time in non-dev mode, in console/terminal we'll see URL that should be used to open Kibana almost immediately (logged by the core HTTP Server), but it won't accessible until optimization phase in the "legacy" Kibana is completed. Previously we displayed that message only when Kibana is entirely ready (in kbnServer.listen). I'm not sure whether it's something we should really care about, but it still may confuse some people or maybe there is some automation around that "Kibana is ready" string too?

    When Kibana is accessed during optimization phase in "legacy" world user would get Unable to connect since Kibana isn't listening on the port yet, in "new platform" world we already have listener before optimization phase is started so we respond with 503 and Retry-After header (and Kibana server is not ready yet body) until Kibana is fully initialized. If Kibana is accessed through base path proxy then the proxy will "pause" request until Kibana is ready, and 503 will be seen only if user tries to bypass base path proxy (going directly to :5603).

  • Yargs is responsible for help/usage rendering now, so it not only displays argument's type and "mandatoriness", but default value as well. It's good in general, but looks a bit ugly for long default values (like paths to configs). Currently I rely on yargs default value functionality for config/plugins paths and --help output may look a bit broken, I can get rid of it if you feel it looks really bad.

  • I removed support for dedicated help command in favor of generic --help argument (./bin/kibana help serve vs ./bin/kibana serve --help), that's supported out of the box and feels much more natural to me, but would like to hear what you think is better.

  • cli_keystore and cli_plugin still rely on src/legacy/cli/command.js, I didn't touch them so we'll have inconsistent CLI behavior for main and "secondary" CLI's for now (I'd probably convert them to main CLI sub-commands e.g. ./bin/kibana keystore create).

  • If CLI fails I display Specify --help for available options message right after error message instead of full --help content like is done in "legacy" Kibana. Don't really know why, probably I just fed up with it, seen it so many times while working on this PR :) Tell me if you prefer to have it, and I'll return it back.

  • The "legacy" serve command rejects unknown single hyphen options as extra options that will be merged with the main config, I didn't include this piece of code in CLI and deferred that to config validation that will fail with unused config values anyway. Tell me if you prefer the previous behavior.


Blocked by:

  • Introduce schema.any (PR, merged)
  • Make core logging independent from the legacy Kibana (PR, merged)
  • Merge MutableLoggerFactory and LoggingService (PR, in review)
  • Revamp core environment class to support upcoming core<-->legacy bootstrap inversion (PR, in review)
  • Make core responsible for reading and merging of config files. Simplify legacy config adapter. (PR, ready for review)
  • Implement LegacyService. Use core to start legacy Kibana. (PR, ready for review)

@azasypkin azasypkin added WIP Work in progress Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Jun 18, 2018
@azasypkin azasypkin self-assigned this Jun 29, 2018
@azasypkin azasypkin changed the base branch from kbn-core to master July 11, 2018 09:14
@azasypkin azasypkin force-pushed the kbn-core-cli branch 3 times, most recently from 10df503 to f5b304e Compare July 24, 2018 14:41
@azasypkin azasypkin force-pushed the kbn-core-cli branch 2 times, most recently from e3f5cb8 to 9821d83 Compare July 30, 2018 15:49
@azasypkin azasypkin changed the title [WIP] Use new platform CLI as the main entry point [skip ci][WIP] Use new platform CLI as the main entry point Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@elastic elastic deleted a comment from elasticmachine Jul 31, 2018
@azasypkin
Copy link
Member Author

Hey @elastic/kibana-platform,

Would you mind giving this PR a spin? There is no need to do a code review (unless you really want to give preliminary feedback!) as I'll split it into more reviewable chunks/PRs later on.

Also there are no recommendations on how to test this one since I'm really interested in the way you use CLI on day to day basis so that we can notice issues I've missed.

Please, see Known issues or changes in behaviour and let me know what you think should be handled or improved on the very first iteration. There are likely some other subtle differences I haven't mentioned, so feel free to comment about any suspicious CLI behavior.

Thanks!

@azasypkin
Copy link
Member Author

Okay, here's quick update on the issue above:

  • 500 issue was happening since core was forwarding request to the "legacy" Kibana/Hapi that was triggering route handlers and request lifecycle hooks that were registered before optimization is started and Elasticsearch plugin is initialized ( TypeError: Cannot read property 'getCluster' of undefined for server.plugins.elasticsearch.getCluster('admin')). This could happen when you tried to access Kibana in non-dev mode or bypass base path proxy in dev mode (using :5603 directly) during optimization phase. In dev mode base path proxy would have properly "paused" all requests.

  • With "legacy" Kibana you'd just get Unable to connect since "legacy" Hapi isn't listening on port yet.

  • In the core we can either "pause" requests until Kibana is ready similar to what base path proxy does or just reject requests that would be similar to "Unable to connect" we have in master. I don't really want to add "pausing" logic overhead in request proxiyng code as it's called for every request, so I decided to just respond with 503 and Retry-After header (and Kibana server is not ready yet body - may not be the best wording though) until Kibana is fully initialized because it feels like Service Unavailable is what really happening here.

I've updated first point in Known issues or changes in behaviour with these details as well.

Does that make sense to you @spalger @epixa?

@epixa
Copy link
Contributor

epixa commented Aug 4, 2018

I'd probably convert them to main CLI sub-commands e.g. ./bin/kibana keystore create

The CLI commands are consistent across the stack, so we don't want to change them unless there's a good reason and we coordinated that with the ingest and elasticsearch teams.

@epixa
Copy link
Contributor

epixa commented Aug 4, 2018

I do think it's important that we don't log the host and port until all endpoints are actually loaded up. It's nice that the base path proxy handles it for you, but I'm far more interested in the behaviors when the basepath proxy is not present than when it is. I just tested on a snapshot build of this branch and it took multiple seconds between when we logged the statement and when Kibana would stop spitting out errors.

This doesn't appear to have anything to do with optimization because it's happening on a fresh install from a snapshot, so the optimizer is not firing.

Perhaps we could just pass a function with the binding logic into legacy server that it can invoke when it's all loaded up?

@azasypkin
Copy link
Member Author

azasypkin commented Aug 5, 2018

The CLI commands are consistent across the stack, so we don't want to change them unless there's a good reason and we coordinated that with the ingest and elasticsearch teams.

I got you, but it seems it doesn't stop us from using ./bin/kibana keystore and ./bin/kibana plugin for ./bin/kibana-keystore and ./bin/kibana-plugin under the hood, right? Anyway, it's out of scope for this task, but I'll keep that in mind.

I do think it's important that we don't log the host and port until all endpoints are actually loaded up. It's nice that the base path proxy handles it for you, but I'm far more interested in the behaviors when the basepath proxy is not present than when it is. I just tested on a snapshot build of this branch and it took multiple seconds between when we logged the statement and when Kibana would stop spitting out errors.
This doesn't appear to have anything to do with optimization because it's happening on a fresh install from a snapshot, so the optimizer is not firing.

Yeah, optimizer is just the slowest thing, but we also have a bunch of other initialization logic in the legacy world.

Perhaps we could just pass a function with the binding logic into legacy server that it can invoke when it's all loaded up?

We don't need to pass anything since core now manages kbn server and core/.../legacy_service can just log this message whenever kbn server is ready (additionally I'll downgrade messages emitted by core/.../http_service to debug so that it doesn't distract users, but will still allow us to get more insight into core life cycle during debugging).

@epixa
Copy link
Contributor

epixa commented Aug 5, 2018

I got you, but it seems it doesn't stop us from using ./bin/kibana keystore and ./bin/kibana plugin for ./bin/kibana-keystore and ./bin/kibana-plugin under the hood, right? Anyway, it's out of scope for this task, but I'll keep that in mind.

I'm not really sure what you mean by this, so let's just sync up whenever we do tackle that.

We don't need to pass anything since core now manages kbn server and core/.../legacy_service can just log this message whenever kbn server is ready (additionally I'll downgrade messages emitted by core/.../http_service to debug so that it doesn't distract users, but will still allow us to get more insight into core life cycle during debugging).

Awesome!

@epixa
Copy link
Contributor

epixa commented Aug 5, 2018

I tested running Kibana with a snapshot build as well as installing a plugin with a snapshot build, and other than the feedback I had about the timing of the hostname/port log, it seemed good.

I'm sure I'll have more concrete feedback on a PR by PR basis.

@elasticmachine

This comment has been minimized.

@azasypkin azasypkin changed the title Use new platform CLI as the main entry point [skip ci] Use new platform CLI as the main entry point Aug 14, 2018
@azasypkin azasypkin force-pushed the kbn-core-cli branch 2 times, most recently from c28ac07 to bccd53f Compare August 28, 2018 12:56
@azasypkin azasypkin removed the blocked label Sep 6, 2018
@azasypkin
Copy link
Member Author

As it turned out we can make core responsible for the bootstrapping of Kibana even without new platform CLI (see #22190), so I'm closing this one for now.

/cc @elastic/kibana-operations, just in case you may find something useful in this PR at some point since, if I understand correctly, CLI is on your plate.

@azasypkin azasypkin closed this Sep 7, 2018
@azasypkin azasypkin deleted the kbn-core-cli branch September 7, 2018 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants