Plugin Framework #1435

shemnon · 2019-05-10T21:32:00Z

PR description

Provide a mechanism for Pantheon to integrate plugin code at runtime.

Fixed Issue(s)

…ramework

…amework

remove debug logging

mbaxter · 2019-05-16T14:36:23Z

...ptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/PantheonNode.java

+            homeDirectory.resolve("plugins").toFile().mkdirs();
+            copyResource(
+                pluginName + ".jar", homeDirectory.resolve("plugins/" + pluginName + ".jar"));
+            PantheonNode.this.plugins.add(pluginName);


nice trick - haven't seen this syntax before 😄

It's essential for inner classes where names are shadowed.

mbaxter · 2019-05-16T14:40:03Z

...ptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/PantheonNode.java

@@ -390,7 +403,7 @@ public Address getAddress() {
    return Util.publicKeyToAddress(keyPair.getPublicKey());
  }

-  Path homeDirectory() {
+  public Path homeDirectory() {


(nit) getHomeDirectory ?

+1 for consistency of getters.

Fixed at the interface, along with other inconsistent names.

mbaxter · 2019-05-16T14:42:59Z

...ptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/PantheonNode.java

+  }
+
+  @Override
+  public void setPlugins(final List<String> plugins) {


this looks like dead code

When we are acceptance testing specific plugins it will be needed, so I'll bring it back then.

mbaxter · 2019-05-16T15:13:48Z

plugins/src/main/java/tech/pegasys/pantheon/plugins/internal/PantheonPluginContextImpl.java

+
+      for (final PantheonPlugin plugin : serviceLoader) {
+        try {
+          plugin.register(this);


Is it a little dangerous to give plugins access to the object that can start / stop / register plugins? Why not just have a separate object that holds your services?

This is written with Java modules in mind. The class they would need to cast it to (PantheonPluginContextImpl) will not be visible in the modules and the JVM will enforce that barrier at compile time and runtime. The interface only exposes a PantheonContext to them.

If we used a proxy class they will still be able to (pre Java modules) use reflection to navigate to the containing context and get access.

Plugins right now are not meant to run byzantine plugins.

If I am missing something ping me on slack.

mbaxter · 2019-05-16T15:16:23Z

plugins/src/main/java/tech/pegasys/pantheon/plugins/PantheonContext.java

+
+public interface PantheonContext {
+
+  <T> Optional<T> getService(Class<T> serviceType);


Curious - why go with this generic pattern rather than just explicitly exposing some services like registerCLIOptions, getPantheonEvents, etc? Is the idea that this makes it easier for us to remove services in the future?

Also wondering if we shouldn't just return the service class directly rather than wrapping in it in an Optional? And in the case where a plugin asks for a service that isn't registered, we could just throw an exception and disable that plugin. Seems safer to disable plugins that might be missing required services.

This is so we can expose more services later and not have to mutate the API. In a future revision the plugins themselves could expose services.

The Optional is there in case plugins want to do graceful degraded service. A plugin may want a service but not require it. For example a plugin that interacts with consensus engines would only expect one to be operating but would go down the list of possible engines that expose services to the plugin architecture.

plugins/src/main/java/tech/pegasys/pantheon/plugins/internal/PantheonPluginContextImpl.java

mbaxter · 2019-05-16T15:25:30Z

plugins/src/test/java/tech/pegasys/pantheon/plugins/TestPlugin.java

+  public void register(final PantheonContext context) {
+    this.context = context;
+
+    if (System.getProperty("testPlugin.failAtRegister") != null) {


Any plans to add tests that hit these failure cases or check that the registered CLI flags are used?

Yes, with this commit. Validating the flag is a bit tricky and needs to be done as part of the acceptance tests.

mbaxter · 2019-05-16T15:43:18Z

plugins/src/main/java/tech/pegasys/pantheon/plugins/services/PicoCLIOptions.java

+package tech.pegasys.pantheon.plugins.services;
+
+/** This service will be available during the registration callbacks. */
+public interface PicoCLIOptions {


Just checking my understanding here. Seems like the idea in tying this class to PicoCLI rather than making it something more generic like CLIOptions is that in the future we might use a different CLI system, that will be supported by a completely different service / api?

+1, strongly coupled to Pico cli library.

Yes, graceful migration.

AbdelStark

Few comments left.
General mechanism looks good to me.

AbdelStark · 2019-05-16T15:56:26Z

...ptance-tests/src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/PantheonNode.java

@@ -390,7 +403,7 @@ public Address getAddress() {
    return Util.publicKeyToAddress(keyPair.getPublicKey());
  }

-  Path homeDirectory() {
+  public Path homeDirectory() {


+1 for consistency of getters.

AbdelStark · 2019-05-16T15:57:47Z

...src/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ProcessPantheonNodeRunner.java

@@ -184,6 +184,13 @@ public void startNode(final PantheonNode node) {
            .directory(new File(System.getProperty("user.dir")).getParentFile())
            .redirectErrorStream(true)
            .redirectInput(Redirect.INHERIT);
+    if (!node.getPlugins().isEmpty()) {


I'm quite sceptical to put environment variables directly in the code.

It's a test harness and environmental variables is the documented way we add Java options (such as memory) to the invocation. This reflects real world usage.

AbdelStark · 2019-05-16T15:59:59Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/DefaultCommandValues.java

@@ -43,6 +43,7 @@
  long DEFAULT_MIN_REFRESH_DELAY = 1;
  String DOCKER_GENESIS_LOCATION = "/etc/pantheon/genesis.json";
  String DOCKER_DATADIR_LOCATION = "/var/lib/pantheon";
+  String DOCKER_PLUGINSDIR_LOCATION = "/etc/pantheon/plugins";


This will disappear with Kubernetes work as all others docker paths.

Until k8s lands we need it.

AbdelStark · 2019-05-16T16:01:46Z

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -1205,6 +1217,21 @@ private Path dataDir() {
    }
  }

+  private Path pluginsDir() {
+    if (isFullInstantiation()) {


Same as previous comment. App should not behave differently when run inside docker.

This is a concern outside the scope of this PR. I am being consistent with the current architecture.

AbdelStark · 2019-05-16T16:04:31Z

plugins/src/main/java/tech/pegasys/pantheon/plugins/internal/PantheonPluginContextImpl.java

+    state = Lifecycle.STARTING;
+    final Iterator<PantheonPlugin> pluginsIterator = plugins.iterator();
+
+    while (pluginsIterator.hasNext()) {


Do we really want to start plugins sequentially ?

The other option being parallel? I don't see parallel startup ending well.

AbdelStark · 2019-05-16T16:06:16Z

plugins/src/main/java/tech/pegasys/pantheon/plugins/services/PicoCLIOptions.java

+package tech.pegasys.pantheon.plugins.services;
+
+/** This service will be available during the registration callbacks. */
+public interface PicoCLIOptions {


+1, strongly coupled to Pico cli library.

…antheonPluginContextImpl.java Co-Authored-By: mbaxter <mbaxter@users.noreply.github.com>

unit tests style fixes

...ests/src/test/java/tech/pegasys/pantheon/tests/acceptance/plugins/PluginsAcceptanceTest.java

mbaxter · 2019-05-16T19:05:48Z

plugins/src/main/java/tech/pegasys/pantheon/plugins/services/PicoCLIOptions.java

+   * @param optionObject The instance of the object to be inspected. PicoCLI will reflect the fields
+   *     of this object to extract the CLI options.
+   */
+  void addPicoCLIOptions(String namespace, Object optionObject);


Suggested change

void addPicoCLIOptions(String namespace, Object optionObject);

void addCLIOptions(String namespace, Object optionObject);

(optional) We could still probably make the method name more generic

Because it is very tied to PicoCLI's style I feel we should communicate that wherever possible.

mbaxter · 2019-05-16T19:06:23Z

...ests/src/test/java/tech/pegasys/pantheon/tests/acceptance/plugins/PluginsAcceptanceTest.java

+public class PluginsAcceptanceTest extends AcceptanceTestBase {
+  private PantheonNode node;
+
+  // context: https://en.wikipedia.org/wiki/The_Magic_Words_are_Squeamish_Ossifrage


…ramework

…eptance/plugins/PluginsAcceptanceTest.java Co-Authored-By: mbaxter <mbaxter@users.noreply.github.com>

…ramework

shemnon added 14 commits May 10, 2019 15:30

rough implementation

49ebfc3

stop

0dead54

Merge branch 'master' into plugin_framework

fb486a2

Merge branch 'master' of github.com:PegaSysEng/pantheon into plugin_f…

4f0b4ef

…ramework

Merge branch 'master' of github.com:PegaSysEng/pantheon into plugin_f…

6dfb20a

…ramework

Demo plugin with some error handling

613705b

Merge remote-tracking branch 'origin/plugin_framework' into plugin_fr…

ce7928b

…amework

acceptance test for process execution only

042150d

spotless

3ae2fce

jenkins debug

b9b5e46

more jenkins debugging

3afabf6

a different jenkins approach, SourceSets

efeabf7

spotless

4640c8c

remove debug logging

Merge branch 'master' into plugin_framework

94f0ba8

shemnon marked this pull request as ready for review May 16, 2019 14:28

mbaxter reviewed May 16, 2019

View reviewed changes

AbdelStark reviewed May 16, 2019

View reviewed changes

shemnon and others added 2 commits May 16, 2019 11:08

Update plugins/src/main/java/tech/pegasys/pantheon/plugins/internal/P…

92b383b

…antheonPluginContextImpl.java Co-Authored-By: mbaxter <mbaxter@users.noreply.github.com>

CLI tests

581c842

unit tests style fixes

mbaxter approved these changes May 16, 2019

View reviewed changes

shemnon and others added 6 commits May 16, 2019 13:08

Merge branch 'master' of github.com:PegaSysEng/pantheon into plugin_f…

e5a4583

…ramework

update to properties cleanup

ebd4246

Update acceptance-tests/src/test/java/tech/pegasys/pantheon/tests/acc…

a79fb04

…eptance/plugins/PluginsAcceptanceTest.java Co-Authored-By: mbaxter <mbaxter@users.noreply.github.com>

Merge branch 'master' of github.com:PegaSysEng/pantheon into plugin_f…

9c96867

…ramework

Merge branch 'master' of github.com:PegaSysEng/pantheon into plugin_f…

2c81b2b

…ramework

spotless

b0d0774

shemnon merged commit 1037eb5 into PegaSysEng:master May 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin Framework #1435

Plugin Framework #1435

shemnon commented May 10, 2019

mbaxter May 16, 2019

shemnon May 16, 2019

mbaxter May 16, 2019

AbdelStark May 16, 2019

shemnon May 16, 2019

mbaxter May 16, 2019

shemnon May 16, 2019 •

edited

Loading

mbaxter May 16, 2019

shemnon May 16, 2019

mbaxter May 16, 2019

mbaxter May 16, 2019

shemnon May 16, 2019

mbaxter May 16, 2019

shemnon May 16, 2019

mbaxter May 16, 2019

AbdelStark May 16, 2019

shemnon May 16, 2019

AbdelStark left a comment

AbdelStark May 16, 2019

AbdelStark May 16, 2019

shemnon May 16, 2019

AbdelStark May 16, 2019

shemnon May 16, 2019

AbdelStark May 16, 2019

shemnon May 16, 2019

AbdelStark May 16, 2019

shemnon May 16, 2019

AbdelStark May 16, 2019

mbaxter May 16, 2019

shemnon May 16, 2019

mbaxter May 16, 2019


		public interface PantheonContext {

		<T> Optional<T> getService(Class<T> serviceType);

	void addPicoCLIOptions(String namespace, Object optionObject);
	void addCLIOptions(String namespace, Object optionObject);

Plugin Framework #1435

Plugin Framework #1435

Conversation

shemnon commented May 10, 2019

PR description

Fixed Issue(s)

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

shemnon May 16, 2019 • 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

AbdelStark 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

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

shemnon May 16, 2019 •

edited

Loading