From 14211b47f2f0d5501ea645c715e5d857f15ffa01 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Thu, 2 Nov 2017 16:36:09 +0000 Subject: [PATCH] Use the correct Environment object in NativeControllerHolder (elastic/x-pack-elasticsearch#2847) We should not be constructing a temporary Environment object in production code. This currently isn't causing any problems, but it might in the future if elastic/elasticsearch#27144 or something similar is ever merged. Instead the master Environment of the node should always be used. Original commit: elastic/x-pack-elasticsearch@6276a54a45ada88ee6e5e53edc4f90dd97fd5376 --- .../xpack/ml/MachineLearning.java | 4 ++-- .../xpack/ml/MachineLearningFeatureSet.java | 10 ++++---- .../xpack/ml/MlLifeCycleService.java | 14 ++++++----- .../job/process/NativeControllerHolder.java | 11 ++++----- .../ml/MachineLearningFeatureSetTests.java | 23 +++++++++++++------ 5 files changed, 37 insertions(+), 25 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java index 156e82eba32..b83821313ed 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearning.java @@ -313,7 +313,7 @@ public class MachineLearning implements ActionPlugin { NormalizerProcessFactory normalizerProcessFactory; if (AUTODETECT_PROCESS.get(settings) && MachineLearningFeatureSet.isRunningOnMlPlatform(true)) { try { - NativeController nativeController = NativeControllerHolder.getNativeController(settings); + NativeController nativeController = NativeControllerHolder.getNativeController(env); if (nativeController == null) { // This will only only happen when path.home is not set, which is disallowed in production throw new ElasticsearchException("Failed to create native process controller for Machine Learning"); @@ -341,7 +341,7 @@ public class MachineLearning implements ActionPlugin { DatafeedJobBuilder datafeedJobBuilder = new DatafeedJobBuilder(internalClient, jobProvider, auditor, System::currentTimeMillis); DatafeedManager datafeedManager = new DatafeedManager(threadPool, internalClient, clusterService, datafeedJobBuilder, System::currentTimeMillis, auditor, persistentTasksService); - MlLifeCycleService mlLifeCycleService = new MlLifeCycleService(settings, clusterService, datafeedManager, autodetectProcessManager); + MlLifeCycleService mlLifeCycleService = new MlLifeCycleService(env, clusterService, datafeedManager, autodetectProcessManager); InvalidLicenseEnforcer invalidLicenseEnforcer = new InvalidLicenseEnforcer(settings, licenseState, threadPool, datafeedManager, autodetectProcessManager); PersistentTasksExecutorRegistry persistentTasksExecutorRegistry = new PersistentTasksExecutorRegistry(Settings.EMPTY, Arrays.asList( diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java index 9d95623d447..b5379951704 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSet.java @@ -20,6 +20,7 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.env.Environment; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.plugins.Platforms; import org.elasticsearch.xpack.XPackFeatureSet; @@ -64,9 +65,9 @@ public class MachineLearningFeatureSet implements XPackFeatureSet { private final Map nativeCodeInfo; @Inject - public MachineLearningFeatureSet(Settings settings, ClusterService clusterService, Client client, + public MachineLearningFeatureSet(Environment environment, ClusterService clusterService, Client client, @Nullable XPackLicenseState licenseState) { - this.enabled = XPackSettings.MACHINE_LEARNING_ENABLED.get(settings); + this.enabled = XPackSettings.MACHINE_LEARNING_ENABLED.get(environment.settings()); this.clusterService = Objects.requireNonNull(clusterService); this.client = Objects.requireNonNull(client); this.licenseState = licenseState; @@ -74,10 +75,11 @@ public class MachineLearningFeatureSet implements XPackFeatureSet { // Don't try to get the native code version if ML is disabled - it causes too much controversy // if ML has been disabled because of some OS incompatibility. Also don't try to get the native // code version in the transport or tribe client - the controller process won't be running. - if (enabled && XPackPlugin.transportClientMode(settings) == false && XPackPlugin.isTribeClientNode(settings) == false) { + if (enabled && XPackPlugin.transportClientMode(environment.settings()) == false + && XPackPlugin.isTribeClientNode(environment.settings()) == false) { try { if (isRunningOnMlPlatform(true)) { - NativeController nativeController = NativeControllerHolder.getNativeController(settings); + NativeController nativeController = NativeControllerHolder.getNativeController(environment); if (nativeController != null) { nativeCodeInfo = nativeController.getNativeCodeInfo(); } diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/MlLifeCycleService.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/MlLifeCycleService.java index 04a2a75f1cc..b0a0eebc49d 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/MlLifeCycleService.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/MlLifeCycleService.java @@ -8,7 +8,7 @@ package org.elasticsearch.xpack.ml; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.component.LifecycleListener; -import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.env.Environment; import org.elasticsearch.xpack.ml.datafeed.DatafeedManager; import org.elasticsearch.xpack.ml.job.process.NativeController; import org.elasticsearch.xpack.ml.job.process.NativeControllerHolder; @@ -18,16 +18,18 @@ import java.io.IOException; public class MlLifeCycleService extends AbstractComponent { + private final Environment environment; private final DatafeedManager datafeedManager; private final AutodetectProcessManager autodetectProcessManager; - public MlLifeCycleService(Settings settings, ClusterService clusterService) { - this(settings, clusterService, null, null); + public MlLifeCycleService(Environment environment, ClusterService clusterService) { + this(environment, clusterService, null, null); } - public MlLifeCycleService(Settings settings, ClusterService clusterService, DatafeedManager datafeedManager, + public MlLifeCycleService(Environment environment, ClusterService clusterService, DatafeedManager datafeedManager, AutodetectProcessManager autodetectProcessManager) { - super(settings); + super(environment.settings()); + this.environment = environment; this.datafeedManager = datafeedManager; this.autodetectProcessManager = autodetectProcessManager; clusterService.addLifecycleListener(new LifecycleListener() { @@ -47,7 +49,7 @@ public class MlLifeCycleService extends AbstractComponent { if (datafeedManager != null) { datafeedManager.isolateAllDatafeedsOnThisNode(); } - NativeController nativeController = NativeControllerHolder.getNativeController(settings); + NativeController nativeController = NativeControllerHolder.getNativeController(environment); if (nativeController != null) { // This kills autodetect processes WITHOUT closing the jobs, so they get reallocated. if (autodetectProcessManager != null) { diff --git a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/process/NativeControllerHolder.java b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/process/NativeControllerHolder.java index f7de3a9aebe..a17018d06a5 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/ml/job/process/NativeControllerHolder.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/ml/job/process/NativeControllerHolder.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.ml.job.process; -import org.elasticsearch.common.settings.Settings; import org.elasticsearch.env.Environment; import org.elasticsearch.xpack.ml.MachineLearning; import org.elasticsearch.xpack.ml.utils.NamedPipeHelper; @@ -29,16 +28,16 @@ public class NativeControllerHolder { * * The NativeController is created lazily to allow time for the C++ process to be started before connection is attempted. * - * null is returned to tests that haven't bothered to set up path.home and all runs where xpack.ml.autodetect_process=false. + * null is returned to tests where xpack.ml.autodetect_process=false. * * Calls may throw an exception if initial connection to the C++ process fails. */ - public static NativeController getNativeController(Settings settings) throws IOException { + public static NativeController getNativeController(Environment environment) throws IOException { - if (Environment.PATH_HOME_SETTING.exists(settings) && MachineLearning.AUTODETECT_PROCESS.get(settings)) { + if (MachineLearning.AUTODETECT_PROCESS.get(environment.settings())) { synchronized (lock) { if (nativeController == null) { - nativeController = new NativeController(new Environment(settings), new NamedPipeHelper()); + nativeController = new NativeController(environment, new NamedPipeHelper()); nativeController.tailLogsInThread(); } } @@ -51,7 +50,7 @@ public class NativeControllerHolder { * Get a reference to the singleton native process controller. * * Assumes that if it is possible for a native controller to exist that it will already have been created. - * Designed for use by objects that don't have access to settings but know a native controller must exist + * Designed for use by objects that don't have access to the environment but know a native controller must exist * for the object calling this method to exist. */ public static NativeController getNativeController() { diff --git a/plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSetTests.java b/plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSetTests.java index 3bcbf993c4e..1328e49e92e 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSetTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/ml/MachineLearningFeatureSetTests.java @@ -17,6 +17,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.env.Environment; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.XPackFeatureSet; @@ -53,12 +54,17 @@ import static org.mockito.Mockito.when; public class MachineLearningFeatureSetTests extends ESTestCase { + private Settings commonSettings; private ClusterService clusterService; private Client client; private XPackLicenseState licenseState; @Before public void init() throws Exception { + commonSettings = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath()) + .put(MachineLearning.AUTODETECT_PROCESS.getKey(), false) + .build(); clusterService = mock(ClusterService.class); client = mock(Client.class); licenseState = mock(XPackLicenseState.class); @@ -82,7 +88,8 @@ public class MachineLearningFeatureSetTests extends ESTestCase { } public void testAvailable() throws Exception { - MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(Settings.EMPTY, clusterService, client, licenseState); + MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(new Environment(commonSettings), clusterService, client, + licenseState); boolean available = randomBoolean(); when(licenseState.isMachineLearningAllowed()).thenReturn(available); assertThat(featureSet.available(), is(available)); @@ -100,13 +107,14 @@ public class MachineLearningFeatureSetTests extends ESTestCase { public void testEnabled() throws Exception { boolean useDefault = randomBoolean(); boolean enabled = true; - Settings.Builder settings = Settings.builder(); + Settings.Builder settings = Settings.builder().put(commonSettings); if (useDefault == false) { enabled = randomBoolean(); settings.put("xpack.ml.enabled", enabled); } boolean expected = enabled || useDefault; - MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(settings.build(), clusterService, client, licenseState); + MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(new Environment(settings.build()), clusterService, client, + licenseState); assertThat(featureSet.enabled(), is(expected)); PlainActionFuture future = new PlainActionFuture<>(); featureSet.usage(future); @@ -121,7 +129,7 @@ public class MachineLearningFeatureSetTests extends ESTestCase { public void testUsage() throws Exception { when(licenseState.isMachineLearningAllowed()).thenReturn(true); - Settings.Builder settings = Settings.builder(); + Settings.Builder settings = Settings.builder().put(commonSettings); settings.put("xpack.ml.enabled", true); Job opened1 = buildJob("opened1", Arrays.asList(buildMinDetector("foo"))); @@ -139,7 +147,8 @@ public class MachineLearningFeatureSetTests extends ESTestCase { buildDatafeedStats(DatafeedState.STOPPED) )); - MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(settings.build(), clusterService, client, licenseState); + MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(new Environment(settings.build()), clusterService, client, + licenseState); PlainActionFuture future = new PlainActionFuture<>(); featureSet.usage(future); XPackFeatureSet.Usage mlUsage = future.get(); @@ -201,11 +210,11 @@ public class MachineLearningFeatureSetTests extends ESTestCase { public void testUsageGivenMlMetadataNotInstalled() throws Exception { when(licenseState.isMachineLearningAllowed()).thenReturn(true); - Settings.Builder settings = Settings.builder(); + Settings.Builder settings = Settings.builder().put(commonSettings); settings.put("xpack.ml.enabled", true); when(clusterService.state()).thenReturn(ClusterState.EMPTY_STATE); - MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(settings.build(), + MachineLearningFeatureSet featureSet = new MachineLearningFeatureSet(new Environment(settings.build()), clusterService, client, licenseState); PlainActionFuture future = new PlainActionFuture<>(); featureSet.usage(future);