Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loading of external shared-object plugins #2364

Closed
wants to merge 3 commits into from
Closed

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented Feb 4, 2017

this is for supporting external plugins.

external plugins will depend on a few telegraf interface types, as well
as a common telegraf registry.

because the Go 'plugin' package asserts that commit hashes of shared
dependencies must match, we are breaking these shared dependencies into
a separate repo that should very rarely change, which will allow
cross-version plugins to work.

see #1717

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@sparrc sparrc force-pushed the telegraf-registry branch 2 times, most recently from 6b3d602 to 72e3735 Compare February 4, 2017 17:29
@phemmer
Copy link
Contributor

phemmer commented Feb 4, 2017

How about instead versioning the "registry"?
Meaning instead of github.com/influxdata/telegraf-registry, do github.com/influxdata/telegraf-registry/v1.0 (or use something like gopkg.in). And then on any changes to the registry we copy it (if not using gopkg.in), and bump the version number. We would make sure telegraf is still compatible with all of them, and plugins compiled with a specific one would still load.

Also, they don't need to be in a separate repo. The registry can still be kept within telegraf, as long as it's in a different package (directory)

I can put together a demo showing what I mean if it helps.

@sparrc
Copy link
Contributor Author

sparrc commented Feb 4, 2017

versioning it is probably a good idea.

Also, they don't need to be in a separate repo. The registry can still be kept within telegraf, as long as it's in a different package (directory)

I tried this but I got errors stating plugin.Open: plugin was built with a different version of package ...

@phemmer
Copy link
Contributor

phemmer commented Feb 4, 2017

Put together a quick demo, mostly just for fun, but it shows what I mean, and doesn't have any issues with keeping the common package in the main repo:

https://github.com/phemmer/plugtest <-- The main application
https://github.com/phemmer/plugfoo <-- A plugin
Both of these repos have 2 tags, v1 and v2. The v1 app is compatible with the v1 plugin. The v2 app is compatible with both the v1 and v2 plugin.

# go get -d github.com/phemmer/plugtest

# cd $GOPATH/src/github.com/phemmer/plugtest

# git checkout v1
HEAD is now at 7baed59... v1

# go build

# mkdir plugins

# go get -d github.com/phemmer/plugfoo

# (cd $GOPATH/src/github.com/phemmer/plugfoo && git checkout v1 )
HEAD is now at 3db7ebe... v1

# go build -buildmode=plugin -o plugins/plugfoo.so github.com/phemmer/plugfoo

# ./plugtest
Loading API version 1 plugins
Initializing plugfoo. Built with API version 1
GetString: argData string

####
#### Now lets upgrade plugtest to v2, which adds GetInt() to the API
####

# git checkout v2
Previous HEAD position was 7baed59... v1
HEAD is now at 50ae366... v2

# go build

# ./plugtest
Loading API version 2 plugins
Loading API version 1 plugins
Initializing plugfoo. Built with API version 1
GetString: argData string

####
#### Now lets upgrade plugfoo to api v2, so it uses GetInt()
####

# (cd $GOPATH/src/github.com/phemmer/plugfoo && git checkout v2 )
Previous HEAD position was 3db7ebe... v1
HEAD is now at c18e7bd... v2

# go build -buildmode=plugin -o plugins/plugfoo.so github.com/phemmer/plugfoo

# ./plugtest 
Loading API version 2 plugins
Initializing plugfoo. Built with API version 2
GetString: argData string
GetInt: 123

@sparrc
Copy link
Contributor Author

sparrc commented Feb 4, 2017

I was previously under the impression that just a change in the commit hash was enough to do it. But you're correct, I've just tested it myself and realized that it was because I was modifying the inputs package with a testing function.

I'll delete the telegraf-registry repo and keep it all within telegraf. Probably will keep the "registry" portion of the code compartmentalized so that we can do the semantic versioning as you've suggested.

@sparrc sparrc force-pushed the telegraf-registry branch 2 times, most recently from 52a54c5 to 5467e69 Compare February 6, 2017 11:15
this is for supporting external plugins.

external plugins will depend on a few telegraf interface types, as well
as a common telegraf registry.

this will allow external and internal plugins to both share this package
and make it easier to vendor/version the whole thing semantically, which
will make it easier to keep plugins supported across build and telegraf
versions.

see #1717
@sparrc sparrc changed the title Use external telegraf-registry repo loading of external shared-object plugins Feb 6, 2017
@sparrc
Copy link
Contributor Author

sparrc commented Feb 7, 2017

closing this in favor of #2373

in that PR we rely on the plugin developer defining a Plugin symbol that we can lookup, rather than the plugin getting added to the registry in an init() function.

@sparrc sparrc closed this Feb 7, 2017
@danielnelson danielnelson deleted the telegraf-registry branch July 30, 2019 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants