Stateful plugin loading #4806

PlayerWithoutName · 2018-03-10T20:23:20Z

Depends on #4506

The plugins are currently loaded during Executor creation, which takes some extensibility from them. This PR makes them 'stateful', which will allow for more flexibility.

magik6k

Some nitpicks, overall LGTM.

This could be used to solve the config problem from #4506 (comment) in a quite non-hacky way.

I wonder what impact this will have on the command execution perf as afaik we don't load plugins when executing commands on the daemon. On the other hand we might end up doing that anyways if command-plugins ever become a thing.

magik6k · 2018-03-10T20:52:09Z

cmd/ipfs/main.go

@@ -112,6 +112,31 @@ func mainRet() int {
 		}
 		log.Debugf("config path is %s", repoPath)

+		pluginpath := filepath.Join(repoPath, "plugins")


could wrap this whole code block into a separate function like loadPlugins

magik6k · 2018-03-10T20:53:38Z

cmd/ipfs/main.go

+			}
+
+			err = plugins.Initialize()
+			if err != nil {


When a function returns only a single error you can collapse it into if err := plugins.Initialize(); err != nil {

magik6k · 2018-03-10T21:00:10Z

core/builder.go

@@ -15,6 +15,7 @@ import (
 	dag "github.com/ipfs/go-ipfs/merkledag"
 	resolver "github.com/ipfs/go-ipfs/path/resolver"
 	pin "github.com/ipfs/go-ipfs/pin"
+	"github.com/ipfs/go-ipfs/plugin/loader"


Missed prefix

Kubuxu · 2018-03-10T22:34:14Z

The ConstructNode isn't the right place to pass this information. ConstructNode will not be called when running ipfs daemon so the Plugins can't be passed to the core node itself or used by anything really.

The problem we are having here is: want want to use plugins pre and post commands lib execution. Best course of action I see here is adding Plugins variable to oldcmds.Context.

This way the plugins can be loaded before command execution and then used latter with created core.Node.

@Stebalien what do you think?

whyrusleeping · 2018-03-14T23:19:38Z

cc @frrist

Kubuxu · 2018-03-14T23:47:22Z

It might help with Tracing API as you will be able to access config, just at latter stage after initial Run to the plugin. We could also make Init run before node and Run after the node is created but you won't be able to pass core.Node instance due to circular dependency.

frrist · 2018-03-15T19:51:50Z

The main reason for needing access to the nodes config was to determine the peerId of the node running the plugin, this was how we were going to name the tracer.
Currently this is being solved via an env variable since as @magik6k pointed out, the peerId may not be available if ipfs init hasn't been ran. If this allows us to pass the nodes config to a plugin after the node has been initialized that would be great!

PlayerWithoutName · 2018-03-16T07:12:51Z

Yeah, i think after this, you could create like ConfigConsumer interface, implement it on the tracing plugin, then create a method that provides the config for all the plugins that implement that interface and you should be good.

Kubuxu · 2018-03-20T12:21:37Z

@PlayerWithoutName could you rebase your PR. To solve conflicts with other PR (#4506 ) please base it on feat/opentrace branch.

magik6k · 2018-03-30T17:33:21Z

commands/request.go

@@ -18,6 +19,7 @@ type Context struct {
 	Online     bool
 	ConfigRoot string
 	ReqLog     *ReqLog
+	Plugins    *loader.PluginLoader


I'd separate this with a newline from the group above.

magik6k · 2018-03-30T17:39:34Z

plugin/loader/loader.go

-	}
+// Initialize all the loaded plugins
+func (loader *PluginLoader) Initialize() error {
+	return initialize(loader.plugins)


Why not just put these funcs on the struct directly?

Kubuxu

Apart from 2 nitpicks that @magik6k pointed out SGWM.

Kubuxu · 2018-04-15T03:41:53Z

plugin/loader/initializer.go

-func run(plugins []plugin.Plugin) error {
-	for _, pl := range plugins {
+//Run the plugins
+func (loader *PluginLoader) Run() error {


It is good practice to keep the implementation of given structure in one package.
In this case, it means keeping those functions in one file with the definition of the struct.

Kubuxu · 2018-04-17T19:30:52Z

@Stebalien want to review this?

Stebalien

LGTM. I do worry about the perf implications of loading plugins on the client but:

Most plugins should do most of their work on-demand anyways.
Ideally, we'd want to run IPLD plugins on the client.

whyrusleeping · 2018-04-20T14:48:41Z

@Kubuxu this one needs a rebase. Then i'll merge it in

PlayerWithoutName · 2018-04-21T14:15:21Z

Rebased

whyrusleeping · 2018-04-22T00:47:24Z

cmd/ipfs/main.go

@@ -47,6 +47,44 @@ const (
 	heapProfile        = "ipfs.memprof"
 )

+type cmdInvocation struct {


Looks like some code that got deleted in another PR is getting reintroduced here. cc @magik6k

Fixed, it was legacy code accidentally put up with second commit.

Stebalien · 2018-12-18T03:25:57Z

Rebased... will merge if I can get the tests to pass.

License: MIT Signed-off-by: Kacper Łukawski <kacluk98@gmail.com>

PlayerWithoutName requested a review from Kubuxu as a code owner March 10, 2018 20:23

PlayerWithoutName force-pushed the feat/plugins/loader branch 4 times, most recently from 9985522 to 0349fb8 Compare March 10, 2018 20:40

magik6k reviewed Mar 10, 2018

View reviewed changes

Kubuxu added the need/author-input Needs input from the original author label Mar 12, 2018

PlayerWithoutName force-pushed the feat/plugins/loader branch 2 times, most recently from e51dee1 to fbbf41c Compare March 12, 2018 19:56

magik6k removed the need/author-input Needs input from the original author label Mar 12, 2018

PlayerWithoutName force-pushed the feat/plugins/loader branch from fbbf41c to 6c3879b Compare March 12, 2018 20:24

Kubuxu requested a review from Stebalien March 13, 2018 19:05

Kubuxu mentioned this pull request Mar 20, 2018

Add Opentracing plugin support #4506

Merged

Kubuxu added the status/in-progress In progress label Mar 20, 2018

Kubuxu assigned Kubuxu and unassigned Kubuxu Mar 20, 2018

Kubuxu added need/author-input Needs input from the original author and removed status/in-progress In progress labels Mar 23, 2018

PlayerWithoutName force-pushed the feat/plugins/loader branch 3 times, most recently from 0f40a97 to aea981a Compare March 25, 2018 21:16

magik6k removed the need/author-input Needs input from the original author label Mar 25, 2018

magik6k mentioned this pull request Mar 30, 2018

plugin: support system-wide plugins #4244

Open

magik6k reviewed Mar 30, 2018

View reviewed changes

Kubuxu approved these changes Apr 7, 2018

View reviewed changes

PlayerWithoutName force-pushed the feat/plugins/loader branch 2 times, most recently from 2bb74de to 8b45855 Compare April 14, 2018 21:36

magik6k approved these changes Apr 14, 2018

View reviewed changes

Kubuxu requested changes Apr 15, 2018

View reviewed changes

PlayerWithoutName force-pushed the feat/plugins/loader branch from 8b45855 to 424ccf7 Compare April 15, 2018 04:51

Kubuxu approved these changes Apr 17, 2018

View reviewed changes

Kubuxu added the RFM label Apr 17, 2018

Stebalien approved these changes Apr 19, 2018

View reviewed changes

PlayerWithoutName force-pushed the feat/plugins/loader branch from 424ccf7 to abdfeca Compare April 21, 2018 02:18

whyrusleeping reviewed Apr 22, 2018

View reviewed changes

PlayerWithoutName force-pushed the feat/plugins/loader branch from abdfeca to 1b7aaf8 Compare April 22, 2018 01:19

Stebalien added needs rebase and removed RFM labels Jul 20, 2018

Stebalien force-pushed the feat/plugins/loader branch from 1b7aaf8 to 5bb935f Compare December 18, 2018 03:17

ghost assigned Stebalien Dec 18, 2018

ghost added the status/in-progress In progress label Dec 18, 2018

Stebalien removed the needs rebase label Dec 18, 2018

Stebalien force-pushed the feat/plugins/loader branch from 5bb935f to 802521b Compare December 18, 2018 03:25

plugin: wire in plugin loader for node creation

a1e654c

License: MIT Signed-off-by: Kacper Łukawski <kacluk98@gmail.com>

Stebalien force-pushed the feat/plugins/loader branch from 802521b to a1e654c Compare December 18, 2018 03:41

Stebalien merged commit 5866e6a into ipfs:master Dec 18, 2018

ghost removed the status/in-progress In progress label Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stateful plugin loading #4806

Stateful plugin loading #4806

Stateful plugin loading #4806

Stateful plugin loading #4806

Conversation

Depends on #4506

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