[Enhancement] Pass localNode info to all plugins on node start (opens…

…earch-project#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
imRishN · Jun 27, 2023 · 279f60f · 279f60f
1 parent a19eee5
commit 279f60f
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -43,6 +43,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Change http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
 - Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283))
 - Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
+- Pass localNode info to all plugins on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919))
 
 ### Deprecated
 

diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java
@@ -1356,7 +1356,7 @@ public void onTimeout(TimeValue timeout) {
 
         logger.info("started");
 
-        pluginsService.filterPlugins(ClusterPlugin.class).forEach(ClusterPlugin::onNodeStarted);
+        pluginsService.filterPlugins(ClusterPlugin.class).forEach(plugin -> plugin.onNodeStarted(clusterService.localNode()));
 
         return this;
     }

diff --git a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java
@@ -32,6 +32,7 @@
 
 package org.opensearch.plugins;
 
+import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.routing.allocation.ExistingShardsAllocator;
 import org.opensearch.cluster.routing.allocation.allocator.ShardsAllocator;
 import org.opensearch.cluster.routing.allocation.decider.AllocationDecider;
@@ -86,7 +87,19 @@ default Map<String, ExistingShardsAllocator> getExistingShardsAllocators() {
 
     /**
      * Called when the node is started
+     *
+     * DEPRECATED: Use {@link #onNodeStarted(DiscoveryNode)} for newer implementations.
      */
+    @Deprecated
     default void onNodeStarted() {}
 
+    /**
+     * Called when node is started. DiscoveryNode argument is passed to allow referring localNode value inside plugin
+     *
+     * @param localNode local Node info
+     */
+    default void onNodeStarted(DiscoveryNode localNode) {
+        onNodeStarted();
+    }
+
 }
diff --git a/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java
@@ -0,0 +1,78 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.plugins;
+
+import org.opensearch.cluster.node.DiscoveryNode;
+import org.opensearch.test.OpenSearchSingleNodeTestCase;
+
+import java.util.Collection;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+public class ClusterPluginTests extends OpenSearchSingleNodeTestCase {
+
+    @Override
+    protected Collection<Class<? extends Plugin>> getPlugins() {
+        return pluginList(DummyClusterPlugin.class, DummyClusterPlugin2.class);
+    }
+
+    public void testOnNodeStarted_shouldContainLocalNodeInfo() {
+
+        DiscoveryNode localNode = DummyClusterPlugin.getLocalNode();
+
+        assertTrue(localNode != null);
+        // TODO Figure out if there is a way to check ephemeralId
+        assertTrue(localNode.getId().equals(node().getNodeEnvironment().nodeId()));
+    }
+
+    public void testOnNodeStarted_shouldCallDeprecatedMethod() {
+        DummyClusterPlugin2 dummyClusterPlugin2 = mock(DummyClusterPlugin2.class);
+        dummyClusterPlugin2.onNodeStarted();
+        verify(dummyClusterPlugin2, times(1)).onNodeStarted();
+
+        DiscoveryNode localNode = DummyClusterPlugin2.getLocalNode();
+        assertTrue(localNode != null);
+    }
+
+}
+
+final class DummyClusterPlugin extends Plugin implements ClusterPlugin {
+
+    private static volatile DiscoveryNode localNode;
+
+    public DummyClusterPlugin() {}
+
+    @Override
+    public void onNodeStarted(DiscoveryNode localNode) {
+        DummyClusterPlugin.localNode = localNode;
+    }
+
+    public static DiscoveryNode getLocalNode() {
+        return localNode;
+    }
+}
+
+class DummyClusterPlugin2 extends Plugin implements ClusterPlugin {
+
+    private static volatile DiscoveryNode localNode;
+
+    public DummyClusterPlugin2() {}
+
+    @Override
+    public void onNodeStarted() {
+        localNode = mock(DiscoveryNode.class);
+    }
+
+    public static DiscoveryNode getLocalNode() {
+        return localNode;
+    }
+
+}