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

[dev/cli] detect worker type using env, not cluster module #83977

Merged
merged 8 commits into from
Nov 25, 2020
3 changes: 1 addition & 2 deletions packages/kbn-config/src/__mocks__/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export function getEnvOptions(options: DeepPartial<EnvOptions> = {}): EnvOptions
runExamples: false,
...(options.cliArgs || {}),
},
isDevClusterMaster:
options.isDevClusterMaster !== undefined ? options.isDevClusterMaster : false,
isDevCliParent: options.isDevCliParent !== undefined ? options.isDevCliParent : false,
};
}
12 changes: 6 additions & 6 deletions packages/kbn-config/src/__snapshots__/env.test.ts.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/kbn-config/src/env.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ test('correctly creates default environment in dev mode.', () => {
REPO_ROOT,
getEnvOptions({
configs: ['/test/cwd/config/kibana.yml'],
isDevClusterMaster: true,
isDevCliParent: true,
})
);

Expand Down
6 changes: 3 additions & 3 deletions packages/kbn-config/src/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { PackageInfo, EnvironmentMode } from './types';
export interface EnvOptions {
configs: string[];
cliArgs: CliArgs;
isDevClusterMaster: boolean;
isDevCliParent: boolean;
}

/** @internal */
Expand Down Expand Up @@ -104,7 +104,7 @@ export class Env {
* Indicates that this Kibana instance is run as development Node Cluster master.
spalger marked this conversation as resolved.
Show resolved Hide resolved
* @internal
*/
public readonly isDevClusterMaster: boolean;
public readonly isDevCliParent: boolean;

/**
* @internal
Expand All @@ -122,7 +122,7 @@ export class Env {

this.cliArgs = Object.freeze(options.cliArgs);
this.configs = Object.freeze(options.configs);
this.isDevClusterMaster = options.isDevClusterMaster;
this.isDevCliParent = options.isDevCliParent;

const isDevMode = this.cliArgs.dev || this.cliArgs.envName === 'development';
this.mode = Object.freeze<EnvironmentMode>({
Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-legacy-logging/src/log_format_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const type = _.memoize((t: string) => {
return color(t)(_.pad(t, 7).slice(0, 7));
});

const workerType = process.env.kbnWorkerType ? `${type(process.env.kbnWorkerType)} ` : '';
const prefix = process.env.isDevCliChild ? `${type('server')} ` : '';

export class KbnLoggerStringFormat extends BaseLogFormat {
format(data: Record<string, any>) {
Expand All @@ -71,6 +71,6 @@ export class KbnLoggerStringFormat extends BaseLogFormat {
return s + `[${color(t)(t)}]`;
}, '');

return `${workerType}${type(data.type)} [${time}] ${tags} ${msg}`;
return `${prefix}${type(data.type)} [${time}] ${tags} ${msg}`;
}
}
7 changes: 0 additions & 7 deletions packages/kbn-legacy-logging/src/rotate/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/

import { isMaster, isWorker } from 'cluster';
import { Server } from '@hapi/hapi';
import { LogRotator } from './log_rotator';
import { LegacyLoggingConfig } from '../schema';
Expand All @@ -30,12 +29,6 @@ export async function setupLoggingRotate(server: Server, config: LegacyLoggingCo
return;
}

// We just want to start the logging rotate service once
// and we choose to use the master (prod) or the worker server (dev)
if (!isMaster && isWorker && process.env.kbnWorkerType !== 'server') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In master all workers are of type server, so this condition will always evaluate to false

return;
}

// We don't want to run logging rotate server if
// we are not logging to a file
if (config.dest === 'stdout') {
Expand Down
13 changes: 2 additions & 11 deletions packages/kbn-legacy-logging/src/rotate/log_rotator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/

import * as chokidar from 'chokidar';
import { isMaster } from 'cluster';
import fs from 'fs';
import { Server } from '@hapi/hapi';
import { throttle } from 'lodash';
Expand Down Expand Up @@ -351,22 +350,14 @@ export class LogRotator {
}

_sendReloadLogConfigSignal() {
if (isMaster) {
(process as NodeJS.EventEmitter).emit('SIGHUP');
if (!process.env.isDevCliChild || !process.send) {
process.emit('SIGHUP', 'SIGHUP');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are there two arguments to this function? It appears the second argument is supposed to be a socket or server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what @types/node calls for, signal listeners get the signal as their first argument and the emit type reflects that.

return;
}

// Send a special message to the cluster manager
// so it can forward it correctly
// It will only run when we are under cluster mode (not under a production environment)
if (!process.send) {
this.log(
['error', 'logging:rotate'],
'For some unknown reason process.send is not defined, the rotation was not successful'
);
return;
}

process.send(['RELOAD_LOGGING_CONFIG_FROM_SERVER_WORKER']);
}
}
4 changes: 0 additions & 4 deletions src/apm.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ let apmConfig;
const isKibanaDistributable = Boolean(build && build.distributable === true);

module.exports = function (serviceName = name) {
if (process.env.kbnWorkerType === 'optmzr') {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to start APM for both parent and child CLI process? Do they behave differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if we want to run APM in both processes, but they do behave differently. The parent process is basically just loading enough of Kibana to understand the config files and proxy requests to the child process which runs the full server and is restarted by the parent process when a server file is changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the way things are setup it looks like APM is intentionally setup in the parent process, and it uses a different service name to identify itself as the parent.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the info, yeah makes sense. I am fine with keeping it as it is for now.

return;
}

apmConfig = loadConfiguration(process.argv, ROOT_DIR, isKibanaDistributable);
const conf = apmConfig.getConfig(serviceName);
const apm = require('elastic-apm-node');
Expand Down
2 changes: 0 additions & 2 deletions src/cli/cluster/cluster_manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ import { BasePathProxyServer } from '../../core/server/http';
import { Log } from './log';
import { Worker } from './worker';

process.env.kbnWorkerType = 'managr';

export type SomeCliArgs = Pick<
CliArgs,
| 'quiet'
Expand Down
4 changes: 1 addition & 3 deletions src/cli/cluster/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ export class Worker extends EventEmitter {
private readonly clusterBinder: BinderFor;
private readonly processBinder: BinderFor;

private type: string;
private title: string;
private log: any;
private forkBinder: BinderFor | null = null;
Expand All @@ -76,7 +75,6 @@ export class Worker extends EventEmitter {
super();

this.log = opts.log;
this.type = opts.type;
this.title = opts.title || opts.type;
this.watch = opts.watch !== false;
this.startCount = 0;
Expand All @@ -88,7 +86,7 @@ export class Worker extends EventEmitter {

this.env = {
NODE_OPTIONS: process.env.NODE_OPTIONS || '',
kbnWorkerType: this.type,
isDevCliChild: 'true',
kbnWorkerArgv: JSON.stringify([...(opts.baseArgv || baseArgv), ...(opts.argv || [])]),
ELASTIC_APM_SERVICE_NAME: opts.apmServiceName || '',
};
Expand Down
6 changes: 3 additions & 3 deletions src/cli/serve/serve.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function canRequire(path) {
}

const CLUSTER_MANAGER_PATH = resolve(__dirname, '../cluster/cluster_manager');
const CAN_CLUSTER = canRequire(CLUSTER_MANAGER_PATH);
const DEV_MODE_SUPPORTED = canRequire(CLUSTER_MANAGER_PATH);

const REPL_PATH = resolve(__dirname, '../repl');
const CAN_REPL = canRequire(REPL_PATH);
Expand Down Expand Up @@ -189,7 +189,7 @@ export default function (program) {
);
}

if (CAN_CLUSTER) {
if (DEV_MODE_SUPPORTED) {
command
.option('--dev', 'Run the server with development mode defaults')
.option('--ssl', 'Run the dev server using HTTPS')
Expand Down Expand Up @@ -240,7 +240,7 @@ export default function (program) {
dist: !!opts.dist,
},
features: {
isClusterModeSupported: CAN_CLUSTER,
isCliDevModeSupported: DEV_MODE_SUPPORTED,
isReplModeSupported: CAN_REPL,
},
applyConfigOverrides: (rawConfig) => applyConfigOverrides(rawConfig, opts, unknownOptions),
Expand Down
11 changes: 5 additions & 6 deletions src/core/server/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,15 @@
*/

import chalk from 'chalk';
import { isMaster } from 'cluster';
import { CliArgs, Env, RawConfigService } from './config';
import { Root } from './root';
import { CriticalError } from './errors';

interface KibanaFeatures {
// Indicates whether we can run Kibana in a so called cluster mode in which
// Kibana is run as a "worker" process together with optimizer "worker" process
// that are orchestrated by the "master" process (dev mode only feature).
isClusterModeSupported: boolean;
// Indicates whether we can run Kibana in dev mode in which Kibana is run as
// a child process together with optimizer "worker" processes that are
// orchestrated by a parent process (dev mode only feature).
isCliDevModeSupported: boolean;

// Indicates whether we can run Kibana in REPL mode (dev mode only feature).
isReplModeSupported: boolean;
Expand Down Expand Up @@ -71,7 +70,7 @@ export async function bootstrap({
const env = Env.createDefault(REPO_ROOT, {
configs,
cliArgs,
isDevClusterMaster: isMaster && cliArgs.dev && features.isClusterModeSupported,
isDevCliParent: cliArgs.dev && features.isCliDevModeSupported && !process.env.isDevCliChild,
});

const rawConfigService = new RawConfigService(env.configs, applyConfigOverrides);
Expand Down
2 changes: 1 addition & 1 deletion src/core/server/http/http_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ test('does not start http server if process is dev cluster master', async () =>
const service = new HttpService({
coreId,
configService,
env: Env.createDefault(REPO_ROOT, getEnvOptions({ isDevClusterMaster: true })),
env: Env.createDefault(REPO_ROOT, getEnvOptions({ isDevCliParent: true })),
logger,
});

Expand Down
2 changes: 1 addition & 1 deletion src/core/server/http/http_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export class HttpService
* @internal
* */
private shouldListen(config: HttpConfig) {
return !this.coreContext.env.isDevClusterMaster && config.autoListen;
return !this.coreContext.env.isDevCliParent && config.autoListen;
}

public async stop() {
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/legacy/legacy_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ describe('once LegacyService is set up in `devClusterMaster` mode', () => {
REPO_ROOT,
getEnvOptions({
cliArgs: { silent: true, basePath: false },
isDevClusterMaster: true,
isDevCliParent: true,
})
),
logger,
Expand Down Expand Up @@ -391,7 +391,7 @@ describe('once LegacyService is set up in `devClusterMaster` mode', () => {
REPO_ROOT,
getEnvOptions({
cliArgs: { quiet: true, basePath: true },
isDevClusterMaster: true,
isDevCliParent: true,
})
),
logger,
Expand Down
8 changes: 3 additions & 5 deletions src/core/server/legacy/legacy_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ export class LegacyService implements CoreService {
this.log.debug('starting legacy service');

// Receive initial config and create kbnServer/ClusterManager.
if (this.coreContext.env.isDevClusterMaster) {
if (this.coreContext.env.isDevCliParent) {
await this.createClusterManager(this.legacyRawConfig!);
} else {
this.kbnServer = await this.createKbnServer(
Expand Down Expand Up @@ -310,10 +310,8 @@ export class LegacyService implements CoreService {
logger: this.coreContext.logger,
});

// The kbnWorkerType check is necessary to prevent the repl
// from being started multiple times in different processes.
// We only want one REPL.
if (this.coreContext.env.cliArgs.repl && process.env.kbnWorkerType === 'server') {
// Prevent the repl from being started multiple times in different processes.
if (this.coreContext.env.cliArgs.repl && process.env.isDevCliChild) {
// eslint-disable-next-line @typescript-eslint/no-var-requires
require('./cli').startRepl(kbnServer);
}
Expand Down
8 changes: 4 additions & 4 deletions src/core/server/plugins/plugins_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const createPlugin = (
});
};

async function testSetup(options: { isDevClusterMaster?: boolean } = {}) {
async function testSetup(options: { isDevCliParent?: boolean } = {}) {
mockPackage.raw = {
branch: 'feature-v1',
version: 'v1',
Expand All @@ -116,7 +116,7 @@ async function testSetup(options: { isDevClusterMaster?: boolean } = {}) {
coreId = Symbol('core');
env = Env.createDefault(REPO_ROOT, {
...getEnvOptions(),
isDevClusterMaster: options.isDevClusterMaster ?? false,
isDevCliParent: options.isDevCliParent ?? false,
});

config$ = new BehaviorSubject<Record<string, any>>({ plugins: { initialize: true } });
Expand Down Expand Up @@ -638,10 +638,10 @@ describe('PluginsService', () => {
});
});

describe('PluginService when isDevClusterMaster is true', () => {
describe('PluginService when isDevCliParent is true', () => {
beforeEach(async () => {
await testSetup({
isDevClusterMaster: true,
isDevCliParent: true,
});
});

Expand Down
4 changes: 2 additions & 2 deletions src/core/server/plugins/plugins_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS

constructor(private readonly coreContext: CoreContext) {
this.log = coreContext.logger.get('plugins-service');
this.discoveryDisabled = coreContext.env.isDevClusterMaster;
this.discoveryDisabled = coreContext.env.isDevCliParent;
this.pluginsSystem = new PluginsSystem(coreContext);
this.configService = coreContext.configService;
this.config$ = coreContext.configService
Expand Down Expand Up @@ -133,7 +133,7 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
const config = await this.config$.pipe(first()).toPromise();

let contracts = new Map<PluginName, unknown>();
const initialize = config.initialize && !this.coreContext.env.isDevClusterMaster;
const initialize = config.initialize && !this.coreContext.env.isDevCliParent;
if (initialize) {
contracts = await this.pluginsSystem.setupPlugins(deps);
this.registerPluginStaticDirs(deps);
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,10 @@ test(`doesn't setup core services if legacy config validation fails`, async () =
expect(mockI18nService.setup).not.toHaveBeenCalled();
});

test(`doesn't validate config if env.isDevClusterMaster is true`, async () => {
test(`doesn't validate config if env.isDevCliParent is true`, async () => {
const devParentEnv = Env.createDefault(REPO_ROOT, {
...getEnvOptions(),
isDevClusterMaster: true,
isDevCliParent: true,
});

const server = new Server(rawConfigService, devParentEnv, logger);
Expand Down
Loading