[ML] NP: migrate server #58680

alvarezmelissa87 · 2020-02-26T23:29:53Z

Summary

Migrate legacy server to NP.

Create server plugin/index for ml in x-pack/plugins
Move all legacy/server files to plugins/ml
Update all shared type/constants/etc imports in NP server to import from legacy/ (once client is fully moved over those import paths can be updated)

NOTES

x-pack/legacy/plugins/ml/public/application/license/check_license.tsx getFeatures has been hardcoded to return full permissions for now. This will be handled in a follow-up.

In the NP plugin license check, isAvailable is set to isEnabled for now as the NP license check returns false for isAvailable with a basic license - ML should be available on basic with very reduced functionality. This will also be sorted in the follow up.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-02-26T23:29:56Z

Pinging @elastic/ml-ui (:ml)

jgowdyelastic · 2020-02-27T08:29:51Z

x-pack/plugins/ml/server/routes/system.ts

-        return response.customError(wrapError(error));
+          const { getPrivileges } = privilegesProvider(
+            context.ml!.mlClient.callAsCurrentUser,
+            cachedLicenseCheckResult,


I think it's ok to call getLicenseCheckResults again in here rather than caching the license object locally.
getLicenseCheckResults just returns the already created license results object and has no overhead.
job_service.ts is doing this without a cache also in this file on line 80.

Updated in 609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T08:45:37Z

x-pack/plugins/ml/server/routes/job_validation.ts

@@ -25,7 +25,9 @@ type CalculateModelMemoryLimitPayload = TypeOf<typeof modelMemoryLimitSchema>;
 /**
 * Routes for job validation
 */
-export function jobValidationRoutes({ config, xpackMainPlugin, router }: RouteInitialization) {
+export function jobValidationRoutes({ version, getLicenseCheckResults, router }: any) {
+  // const { isSecurityDisabled } = getLicenseCheckResults();


commented code can be removed

609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T08:46:06Z

x-pack/plugins/ml/server/routes/system.ts

-}: RouteInitialization) {
+export function systemRoutes({ getLicenseCheckResults, router, spacesPlugin, cloud }: any) {
+  let cachedLicenseCheckResult: LicenseCheckResult;
+  // const { isSecurityDisabled } = getLicenseCheckResults();


commented code can be removed

609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T08:49:30Z

x-pack/plugins/ml/server/plugin.ts

+    let sampleLinksInitialized = false;
+
+    plugins.features.registerFeature({
+      id: 'ml',


all 'ml' strings in this file could be PLUGIN_ID from types.ts

Good call - added in 609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T09:02:50Z

x-pack/plugins/ml/server/types.ts

+import { LicensingPluginSetup } from '../../licensing/server';
+import { SpacesPluginSetup } from '../../spaces/server';
+
+export const PLUGIN_ID = 'ml';


it'd be good to move this to common as we often need this string client side. it could go in app.ts.
(also API_BASE_PATH in there can go as it's a leftover from transforms being in ML)

Updated in 609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T09:28:42Z

x-pack/plugins/ml/server/plugin.ts

+      };
+    });
+
+    const routeInit = {


this should have a type so we can use it in each route.

Added in 609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T09:35:06Z

x-pack/plugins/ml/server/plugin.ts

+    };
+
+    annotationRoutes({
+      ...routeInit,


i wonder if it's worth merging these into a single object?
An alternative approach could be to pass the routeInit as a single first argument to each route, so they are all the same, and for the routes that require additional information, pass that as a second argument.
The routes themselves could then manage the type of that second "additional dependancies" parameter.
Something like:

systemRoutes(routeInit, { spacesPlugin: plugins.spaces, cloud: plugins.cloud, });

Good idea - used the second idea of having an 'additional dependencies' second parameter where applicable. 609be5c4c218d1feed4b249158c438ce7c30f99d

peteharverson · 2020-02-27T10:13:46Z

x-pack/plugins/ml/server/plugin.ts

+        if (isEnabled === true && plugins.home) {
+          if (
+            this.licenseCheckResults.type &&
+            ['platinum', 'trial'].includes(this.licenseCheckResults.type)


This should include enterprise too, in line with the list of VALID_FULL_LICENSE_MODES in server/lib/check_license/check_license.ts. We should probably define that list of full license modes in e.g. common/constants/license.ts.

Updated and moved to constants/license.ts in 609be5c4c218d1feed4b249158c438ce7c30f99d

peteharverson · 2020-02-27T10:14:32Z

x-pack/plugins/ml/server/lib/check_license/check_license.ts

@@ -49,10 +49,11 @@ export function checkLicense(xpackLicenseInfo: XPackInfo): Response {

  const VALID_FULL_LICENSE_MODES = ['platinum', 'enterprise', 'trial'];


See other comment regarding moving this into a common constants file. Could be done in the follow-up to fix the licensing checks.

609be5c4c218d1feed4b249158c438ce7c30f99d

peteharverson · 2020-02-27T10:15:12Z

x-pack/plugins/ml/server/routes/anomaly_detectors.ts

-import { licensePreRoutingFactory } from '../new_platform/licence_check_pre_routing_factory';
-import { RouteInitialization } from '../new_platform/plugin';
+import { licensePreRoutingFactory } from './license_check_pre_routing_factory';
+// import { RouteInitialization } from '../new_platform/plugin';


Are all these commented out lines for the RouteInitialization needed in all the routes files?

RouteInitialization could be reused for my requested change.
#58680 (comment)

Done in 609be5c4c218d1feed4b249158c438ce7c30f99d

peteharverson · 2020-02-27T10:58:06Z

Gave this a good test, and all works well, apart from the ML item with the Jobs List doesn't appear in the Management app. Is this related to the licensing work that needs to be done?

jgowdyelastic · 2020-02-27T11:13:41Z

x-pack/legacy/plugins/ml/index.ts

-      };
-
-      plugin(initializerContext).setup(core, plugins);
+    async init(server: Server) {


managementSections: ['plugins/ml/application/management'], on line 24 isn't registering our management section anymore.
Is there a NP equivalent?
(github wouldn't let me add this to line 24)

There is, but I think it should work. Maybe this this an issue with the fact that there is now both a x-pack/legacy/plugins/ml and x-pack/plugins/ml folders? But the NP ml plugin has no public part, so I'm not sure to understand. @elastic/kibana-app-arch would you mind to take a look?

@jgowdyelastic - looks like the issue is that in applicatoins/management the code to register the app in management is wrapped in an if block that relies on legacy xpackInfo.

The solution is to use the frontend licensing service - npSetup.plugins.licensing where npSetup comes from import { npSetup } from 'ui/new_platform'

Then we can can pull the relevant license info from

licensing.license$.subscribe(async license => { if (license.type && VALID_FULL_LICENSE_MODES.includes(license.type)) { ... <register management app>

Probably this same approached can be used in the work you'll be doing for check_license.tsx @jgowdyelastic 🤔
cc @peteharverson for context

jgowdyelastic · 2020-02-27T11:24:28Z

x-pack/plugins/ml/server/types.ts

+  features: FeaturesPluginSetup;
+  home: HomeServerPluginSetup;
+  licensing: LicensingPluginSetup;
+  security: any;


this should be SecurityPluginSetup

609be5c4c218d1feed4b249158c438ce7c30f99d

jgowdyelastic · 2020-02-27T13:26:10Z

x-pack/plugins/ml/server/types.ts

+}
+
+export interface PluginsSetup {
+  cloud: any;


this should be CloudSetup

609be5c4c218d1feed4b249158c438ce7c30f99d

joshdover · 2020-02-27T17:13:53Z

x-pack/legacy/plugins/ml/kibana.json

@@ -3,6 +3,7 @@
  "version": "0.0.1",
  "kibanaVersion": "kibana",
  "configPath": ["ml"],
+  "requiredPlugins": ["licensing"],
  "server": true,
  "ui": true
 }


This file doesn't do anything in legacy. I would remove it completely.

Removed in 609be5c4c218d1feed4b249158c438ce7c30f99d

alvarezmelissa87 · 2020-02-27T17:29:52Z

Registering the management section can be addressed in a follow-up PR as we have to do follow up work for licensing issues as well.

This has been updated for all comments and is ready for final look cc @jgowdyelastic, @peteharverson, @joshdover

jgowdyelastic

Tested and LGTM!
This PR is a cut (over) above the rest!
It certainly makes the cut (over)!
Sorry, yeah I know, I could cut (over) it out.

peteharverson

Latest edits LGTM!

alvarezmelissa87 · 2020-02-27T21:31:07Z

retest

joshdover

No major issues spotted 👍

joshdover · 2020-02-27T21:44:39Z

x-pack/plugins/ml/server/plugin.ts

+      makeMlUsageCollector(plugins.usageCollection, core.savedObjects);
+    });
+
+    plugins.licensing.license$.subscribe(async license => {


Not a huge concern, but you should unsubscribe from this observable in the stop phase.

Oh good catch - added to checklist 👍

joshdover · 2020-02-27T21:45:23Z

x-pack/plugins/ml/server/plugin.ts

+        // This `isAvailable` check for the ml plugin returns false for a basic license
+        // ML should be available on basic with reduced functionality (only file data visualizer)
+        // TODO: This will need to be updated in the second step of this cutover to NP.
+        isAvailable: isEnabled,


How did this work before / what is the approach to solving this in the follow up?

kibanamachine · 2020-02-28T02:10:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3a872d5

History

💔 Build #29580 failed 53c2b6f1e55ed64d330e6e8640def48ab82829f8
💔 Build #29559 failed 25603177277414674f498f72d64b75f5b26b012f
💔 Build #29474 failed 609be5c4c218d1feed4b249158c438ce7c30f99d
💔 Build #29352 failed e5db02df04b0bf7388fa4d91399109670f63dab5
💔 Build #29324 failed af0b7edbd184afd07966c62d7c1d746503b4e68d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* remove obsolete legacy server deps * licensePreRoutingFactory uses licensing plugin rather than legacy xpack * move schemas to dir in routes * use NP license check method for license check * store license data in plugin for passing to check * create server plugin files in NP plugin dir * remove dependency on legacy xpack plugin * add sample data links first step * move all server dirs from legacy to np dir * fix requiredPlugin spaces name and update import routes * delete unnecessary files and add sample data links * update license and privilege check tests * add routeInit types

alvarezmelissa87 added :ml release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 26, 2020

alvarezmelissa87 requested a review from a team as a code owner February 26, 2020 23:29

alvarezmelissa87 self-assigned this Feb 26, 2020

alvarezmelissa87 mentioned this pull request Feb 26, 2020

[ML] NP Migration: remove use of legacy xpack plugin from route license precheck #58425

Closed

1 task

alvarezmelissa87 requested review from joshdover, peteharverson, jgowdyelastic, darnautov and walterra February 27, 2020 03:35

jgowdyelastic reviewed Feb 27, 2020

View reviewed changes

peteharverson reviewed Feb 27, 2020

View reviewed changes

jgowdyelastic reviewed Feb 27, 2020

View reviewed changes

joshdover reviewed Feb 27, 2020

View reviewed changes

jgowdyelastic approved these changes Feb 27, 2020

View reviewed changes

peteharverson approved these changes Feb 27, 2020

View reviewed changes

alvarezmelissa87 force-pushed the ml-np-server-cutover-step-one branch from 609be5c to 2560317 Compare February 27, 2020 20:55

peteharverson mentioned this pull request Feb 27, 2020

[ML] New platform migration #49743

Closed

78 tasks

joshdover approved these changes Feb 27, 2020

View reviewed changes

alvarezmelissa87 force-pushed the ml-np-server-cutover-step-one branch from 2560317 to 53c2b6f Compare February 27, 2020 22:28

alvarezmelissa87 added 13 commits February 27, 2020 19:21

remove obsolete legacy server deps

88859cc

licensePreRoutingFactory uses licensing plugin rather than legacy xpack

34554b4

move schemas to dir in routes

42cba74

use NP license check method for license check

eb4fe67

store license data in plugin for passing to check

d9f0039

create server plugin files in NP plugin dir

e76a6c3

remove dependency on legacy xpack plugin

e20f85f

add sample data links first step

d6bc9a0

move all server dirs from legacy to np dir

4ada0ae

fix requiredPlugin spaces name and update import routes

db9c10b

delete unnecessary files and add sample data links

04452ee

update license and privilege check tests

e54623e

add routeInit types

3a872d5

alvarezmelissa87 force-pushed the ml-np-server-cutover-step-one branch from 53c2b6f to 3a872d5 Compare February 28, 2020 00:21

alvarezmelissa87 merged commit 8c3d71b into elastic:master Feb 28, 2020

alvarezmelissa87 mentioned this pull request Feb 28, 2020

[7.x] [ML] NP: migrate server (#58680) #58809

Merged

alvarezmelissa87 deleted the ml-np-server-cutover-step-one branch February 28, 2020 04:05

This was referenced Mar 2, 2020

[ML] Fixing licensing after server NP cutover #58853

Merged

[ML] Fixing licensing after server NP cutover #59275

Merged

		@@ -49,10 +49,11 @@ export function checkLicense(xpackLicenseInfo: XPackInfo): Response {

		const VALID_FULL_LICENSE_MODES = ['platinum', 'enterprise', 'trial'];

[ML] NP: migrate server #58680

[ML] NP: migrate server #58680

Conversation

alvarezmelissa87 commented Feb 26, 2020

Summary

NOTES

Checklist

elasticmachine commented Feb 26, 2020

jgowdyelastic Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgowdyelastic Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarezmelissa87 Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Feb 27, 2020

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Feb 27, 2020

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 28, 2020

💚 Build Succeeded

History

jgowdyelastic Feb 27, 2020 •

edited

Loading

jgowdyelastic Feb 27, 2020 •

edited

Loading

alvarezmelissa87 Feb 27, 2020 •

edited

Loading