Closed mshustov closed 3 years ago
Pinging @elastic/kibana-platform (Team:Platform)
Pinging @elastic/kibana-operations (Team:Operations)
I took an initial look at the exact dependencies coming from DevCliMode
to core
and how we could address this cyclic dependency issue.
As explained in the issue's description, in dev mode, we are booting a pseudo Kibana server with most features disabled just to be able to boot the legacy service which will instantiate the cliDevMode
with a subset of core configuration and services. CliDevMode
will then kickstart the optimizer, the basePath server and the 'real' Kibana server (in another process).
This is just bad design, driven by the fact that CliDevMode
is not able to manually read the configuration and have to rely on core to do it. That adds a lot of if !isDevCliParent
checks in core code, and it just hard to maintain. In addition, this makes the clustering mode core even complicated (https://github.com/elastic/kibana/pull/94057), and is going to be a blocker for Bazel because of the cyclic dependency.
What would need to be done instead is to remove the core
->devCliMode
dependency. The dev mode should not need an instance of a Kibana server to kickstart itself.
The basic idea is that, when DEV_MODE_SUPPORTED
is true, the serve
script (src/cli/serve/serve.js
) would bootstrap DevCliMode
directly (via its own bootstrap script or similar), instead of starting core.
Now, this is easier said than done.
basePathProxy
Currently the basePath proxy server is created from core, and passed down to the CliDevMode
instance. Note that BasePathProxyServer
is exclusively used in development, and should be moved out of core to land in the src/dev/cli_dev_mode
module (or at least in its own module under src/dev
).
Now if we take a look at the dependencies from core this class is currently using:
There are three things here:
dev
confighttp
configLogger
Solving the logger dependency should be quite easy. It doesn't make any sense to have the dev proxy server use the Kibana instance's logging configuration anyway, so we should adapt the proxy server to use the console logger implementation used by other parts of the cli dev mode when we'll extract it out of core:
As an improvement, we could also have the cli logger implements the Logger
interface, now that this is exposed from the @kbn/logging
package. Not sure this is really necessary for a logger only meant to be used in development and to only output to the console though.
Regarding the dependencies to core's configuration schemas, we'll see that in the next section
CliDevMode
instanceThe three parameters are:
cliArgs
from core's Env
These cliArgs
are currently passed directly from the serve
script to core's bootstrap
, so having the serve
script pass them down to the yet-to-be CliDevMode
bootstrap script directly should not be a problem.
The legacy configuration is only used to access two properties: plugins.path
and plugins.scanDirs
The properties are, in new platform terms, respectively PluginsConfig.additionalPluginPaths
and PluginsConfig.pluginSearchPaths
. so the real dependency is against core's plugins
configuration.
See previous section, currently passed from core
to cliDevMode
, will be instantiated directly by CliDevMode
once we extract it.
So, from last section, we can see that the only thing currently really blocking us from just extracting the instantiation of DevCliMode
from core's legacy service to the serve.js
script is the required access to core's configuration.
We will need to find a way to have CliDevMode
be able to read and parse the configuration itself, instead of relying on core to do it and then pass the parsed configuration as an argument.
It would allow us to break the cyclic dependency by cutting the Core
-> CliDevMode
dep.
Before
After
Now, how do we do that?
We recently extracted most of core's config service code into @kbn/config
(#76874). However, only the service's interface and implementation got extracted. Core's config schemas, deprecations and associated types/classes are still in core.
At the time, we chose not to move core's config schemas to @kbn/config
because the associated types are way less isolated than the config service implementation. Moving the schemas and the associated types and classes (e.g the HttpConfig
class) represents some significant refactoring, and would also split core services code between the core module and some Kibana packages, which is not really desirable and would ideally be avoided. Note that another issue with extracting more core classes from the core
module is that api-extractor
does not generate documentation for external types, meaning that we will be loosing generated documentation for all public types we move to the package (may be a deprecated problem though, now that we're planning on @kbn/docs-utils
as a replacement)
I think I only see two options here:
We could move that to @kbn/config
or create a new @kbn/core-config-schemas
package. Not sure which one is preferable. I did not yet look at all we would need to move if we go that way, but I'm expecting it to be quite significant. That way, DevCliMode
could register the necessary schemas and then load and access the configuration without having to rely on core.
DevCliMode
import the schemas and types from coreWith the new dependency graph, preserving the CliDevMode
-> Core
dependency is not an issue, as we removed the inverse dependency and as the cli
module would depends on core anyway. So we could just have the src/dev/cli_dev_mode
import the schemas and types from core without moving them to a package.
This option probably represents less work, but seems to be a worse design. Note that, with Bazel, we will no longer be able to use deep imports from dependencies (not 100% sure, can someone confirm that), so we will have to publicly expose some of core's config schema and internal types from the server entrypoint.
With our ideal end of 7.13
target to address this cyclic dependency, I'm mixed between both options to be honest. 1.
seems like the proper design, but is a significant amount of work for a refactoring only required for dev/infra purposes. 2.
is probably faster to implement, but feels more like a workaround or a temporary solution than a definitive implementation.
@rest-ohwait @mshustov @joshdover wdyt?
I think the option we choose likely depends on the long-term plans of what we're going to do with the BasePathProxy. Personally, I'd like to get away from using this JS proxy and would really prefer we use something realistic to what our users are using (like nginx or haproxy). In either of those cases, I think it's like we use Bazel to start and wire up that frontend with the Kibana backend in development.
Given that, is it necessary that we configure these components exactly the same way? Could we get away with not having to pass the HttpConfig into CliDevMode?
We may also be able to remove the dependency on the plugin path configs. Currently, these are only needed for two things 1) knowing with directories to watch for restarting the server; and 2) running the optimizer. I think 1) could be removed by only supporting this feature for src
, x-pack
, examples
and plugins
. I think 2) will be removed once the web pack builds are built by Bazel.
I ask because I think (?) option (1) has little benefit other than this specific case and I'd really like to avoid doing that high-effort, low-value work if possible.
Could we get away with not having to pass the HttpConfig into CliDevMode?
I doubt so. This is the config with the most internal logic, e.g for ssl
and the BasePathProxyServer
is relying on that
The basic idea is that, when
DEV_MODE_SUPPORTED
is true, theserve
script (src/cli/serve/serve.js
) would bootstrapDevCliMode
directly (via its own bootstrap script or similar), instead of starting core.
@pgayvallet I think I would prefer that instead of calling src/cli/serve
in order to start the dev CLI we would instead call the dev CLI directly from the script, and it would in turn start the src/cli
in the child process as the mechanism for starting src/core
this way we don't need to have any dev-specific logic in the production CLI and can just pass through all flags from the dev CLI to src/cli/serve
. Doing this will also allow us to log deprecation messages and things in the child process rather than the parent process addressing...
- Finish core's config extraction, and move the schemas and associated types to a package ...
- Have the
DevCliMode
import the schemas and types from core
@pgayvallet I'd prefer a third option where the dev CLI parses the config file and CLI args independently, only extracting the little information that it needs, and then relies on the child Kibana process to handle full config validation, deprecation warnings, etc. In the parent process we should need a very small subset of the standard config and it seems fairly trivial to maintain an isolated implementation of the config parsing logic that can be moved to a package with the rest of the cli_dev_mode
code.
I'd like to get away from using this JS proxy and would really prefer we use something realistic to what our users are using (like nginx or haproxy).
@joshdover I understand the idea here, but the BasePathProxy has taken on additional roles since it was created and named and is now responsible for ensuring that browser refreshes get the latest assets/don't fail because of server restarts by integrating with the child processes and pausing requests when the optimizer is running or the server is restarting. Removing this functionality by switching to a more production-like proxy feels like it will only hurt dev experience and I don't think that is a good idea.
@spalger
I think I would prefer that instead of calling src/cli/serve in order to start the dev CLI we would instead call the dev CLI directly from the script
By script
, you mean src/cli/dev.js
, right?
That would require some refactoring in src/cli/serve/serve.js
to extract applyConfigOverrides
, as I think we also need the dev cli config parsing to be aware of overrides. But I agree that it seems cleaner, as we would be able to get rid of DEV_MODE_SUPPORTED
in src/cli/serve/serve.js
, which will have the exact same behavior in dev and production mode.
I'd prefer a third option where the dev CLI parses the config file and CLI args independently, only extracting the little information that it needs
I'd really like that too tbh, and it's almost possible as the config service itself is in a package. The main thing kinda blocking that option is the fact that the basePath proxy relies on the HttpConfig
class, as the class is directly used to invoke getServerOptions
and getListenerOptions
. The hardest part is the ssl config logic, contained into SslConfig
, that is used in getServerOptions
:
Now, I'm not sure if we really have / want to support SSL for the basePath server.
As we're gonna need to extract getServerOptions
and getListenerOptions
to a package anyway (everything that is currently in src/core/server/http/http_tools.ts
AFAIK), we could also extract the ssl parsing logic into that package and import/use it from both SslConfig
and the new devMode 'config parser'
it seems fairly trivial to maintain an isolated implementation of the config parsing logic
Not that trivial if we want to preserve the SSL capabilities of the dev mode.
@spalger I opened a PR (https://github.com/elastic/kibana/pull/95145), starting by extracting the http stuff that now needs to be shared between core and cli_dev_mode.
I'm now looking at how to bootstrap cli_dev_mode
directly from src/cli/dev.js
. I was going to extract applyConfigOverrides
from src/cli/serve/serve.js
, but it will not be sufficient. ATM all the options, including the dev only options, are registered on the serve
command.
Also, the construction of what we call CliArgs
is also done in the serve
command and depends on the registered options (and of course, convert/rename them). If we want to isolate the 2 scripts, we may need to duplicate or extract quite a lot of things (like 95% of the content of src/cli/serve/serve.js
: applyConfigOverrides
and most of the option registration and conversion to cliArgs
)
I think uncoupling the two is going to require some work. I'm not even sure what the correct approach would be. Should I create a new command for dev mode and use it as default when DEV_MODE_SUPPORTED
instead of serve
here?
Or do you have another idea in mind?
During a sync discussion with @spalger yesterday, we decided to keep the src/cli/serve
as the single entry point for both dev and production mode for now.
CLI relies on core types & utils, core uses
cli
in runtime. discussion https://github.com/elastic/kibana/issues/46773#issuecomment-679215603 related work: https://github.com/elastic/kibana/issues/76003The circular dependency exists due to
CliDevMode
references from the Kibana platform'slegacy
service.CliDevMode
is used for (from https://github.com/elastic/kibana/pull/78710):@kbn/optimizer
in the same process, which launches webpack workers in child processesThat's no Kibana platform responsibility. Due to that, the platform code contains a lot of code checking whether it's run in dev cluster mode
isDevClusterMaster
to disable some functionality: SO migration, plugin discovery, plugin initialization, etc. (see https://github.com/elastic/kibana/issues/79037). Which makes the platform code error-prone and hard to maintain.https://github.com/elastic/kibana/blob/4584a8b570402aa07832cf3e5b520e5d2cfa7166/src/core/server/legacy/legacy_service.ts#L183-L189
https://github.com/elastic/kibana/blob/4584a8b570402aa07832cf3e5b520e5d2cfa7166/src/core/server/legacy/cli_dev_mode.js#L9