feat: metric plugin system based on hashicorp go-plugin #2514

zachaller · 2023-01-11T15:34:05Z

This adds support for a plugin system for metric providers using go-pluing from hashicorp.

github-actions · 2023-01-11T15:46:20Z

Go Published Test Results

1 850 tests 1 850 ✔️ 2m 31s ⏱️
  112 suites       0 💤
      1 files       0 ❌

Results for commit 2cb9a35.

♻️ This comment has been updated with latest results.

codecov · 2023-01-11T16:06:17Z

Codecov Report

Base: 81.66% // Head: 81.59% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (2cb9a35) compared to base (b60f69d).
Patch coverage: 77.39% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2514      +/-   ##
==========================================
- Coverage   81.66%   81.59%   -0.07%     
==========================================
  Files         126      130       +4     
  Lines       19147    19469     +322     
==========================================
+ Hits        15636    15886     +250     
- Misses       2717     2766      +49     
- Partials      794      817      +23

Impacted Files	Coverage Δ
analysis/analysis.go	`85.53% <0.00%> (ø)`
analysis/controller.go	`61.53% <ø> (ø)`
utils/analysis/factory.go	`92.41% <0.00%> (-1.79%)`	⬇️
utils/defaults/defaults.go	`89.10% <ø> (ø)`
controller/controller.go	`89.53% <33.33%> (-1.89%)`	⬇️
utils/plugin/plugin.go	`53.84% <53.84%> (ø)`
utils/plugin/downloader.go	`59.54% <59.54%> (ø)`
...ricproviders/plugin/rpc/rpc_test_implementation.go	`91.42% <91.42%> (ø)`
metricproviders/plugin/rpc/rpc.go	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

github-actions · 2023-01-11T16:25:02Z

E2E Tests Published Test Results

    2 files     2 suites 1h 40m 23s ⏱️
  95 tests   88 ✔️ 3 💤 4 ❌
194 runs 184 ✔️ 6 💤 4 ❌

For more details on these failures, see this check.

Results for commit 2cb9a35.

♻️ This comment has been updated with latest results.

zachaller · 2023-01-13T20:31:39Z

After talking with @jessesuen who brought up an idea to look into wasm for plugins. The findings of that research turned out to be that today wasm plugins are currently not an option for Rollouts they have some shortcomings such as no golang runtime currently support network requests or really any io syscalls. There are other maturity issues as well, so the plan is to continue with go-plugins and revisit wasm if/when it makes sense.

skandragon · 2023-01-24T20:32:41Z

Does this only support a single plugin at a time? It also requires admin-side config, where jobs did not, so perhaps that is also an issue.

Or do I misunderstand that only one plugin can be provided?

zachaller · 2023-01-25T00:22:47Z

Yes this is an admin level config due to the fact in order to set it up you need to add a flag to the controller with a path to the binary executable plugin. This limits it to one plugin per controller aka you have some custom metrics system you company uses but it is not supported by rollouts this gives you the option to add that. You could create a plugin though that supports multiple systems etc.

zachaller · 2023-01-25T00:44:43Z

I did think about possibly doing a multiple plugin system that would allow end users to install other plugins per analysis templates but then we need to figure out how to package those up and install them as well etc. On the flip side I could also see this plugin system having someone else just create say a multi end-user plugin system that deals with packaging downloading plugins etc etc if people think that is valuable but would be curious about use cases either way.

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

…plugin-system Signed-off-by: zachaller <zachaller@users.noreply.github.com>

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

cmd/sample-metrics-plugin/internal/plugin/plugin.go

metricproviders/plugin/plugin.go

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

cmd/rollouts-controller/main.go

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz

Please check my comments.
Thank you.

metricproviders/plugin/rpc/rpc.go

test/cmd/sample-metrics-plugin/internal/plugin/plugin.go

utils/plugin/plugin.go

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

metricproviders/plugin/client/client.go

metricproviders/plugin/plugin.go

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz

Looking good overall. I have added 2 more comments.

leoluz · 2023-02-07T20:16:29Z

utils/plugin/plugin_downloader.go

+			return fmt.Errorf("failed to parse plugin location: %w", err)
+		}
+
+		finalFileLocation := filepath.Join(absoluteFilepath, plugin.Name)


how are we going to handle 2 plugins with the same name? wouldn't it be better to have some sort of namespace for plugins as well?

correct this does not support two plugins with the same name I am not sure of the value 100% with that and it is somewhat controlled via the plugin themself because it is the registration method as well, aka plugins need to know where to look for their config at. I think we could provide a best practice as a plugin author to namespace them. Do you have any thoughts on what would make sense for namespace?

Talking with leo we will probably try to enforce a strict file naming convention based on git repo to allow multiple plugins.

leoluz · 2023-02-07T20:24:29Z

utils/plugin/plugin_test.go

+	"k8s.io/client-go/kubernetes/fake"
+)
+
+func TestGetPluginLocation(t *testing.T) {


We use mockery to auto generate mocks from interfaces in Argo CD. This is something that you can have taken advantage as well to implement more test cases. Not a blocker, mainly a suggestion to improve the coverage.

utils/plugin/plugin_downloader.go

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz · 2023-02-07T20:41:42Z

utils/plugin/plugin_downloader_test.go

+	assert.Error(t, err)
+}
+
+func TestInitPlugin(t *testing.T) {


Small suggestion but personally I prefer BDD style test for readability. Example

Will move to this on the traffic router plugin branch

…plugin-system

leoluz

LGTM

sonarcloud · 2023-02-07T20:59:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
1.8% Duplication

zachaller force-pushed the feat-plugin-system branch from 7c887c3 to 2b1e125 Compare January 16, 2023 16:52

zachaller requested a review from leoluz January 18, 2023 15:52

zachaller force-pushed the feat-plugin-system branch from bd72d42 to a1ca692 Compare January 18, 2023 15:54

zachaller marked this pull request as ready for review January 21, 2023 01:05

zachaller marked this pull request as draft January 27, 2023 22:41

clean up commits

14fc188

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

zachaller force-pushed the feat-plugin-system branch from 2667fee to 14fc188 Compare January 29, 2023 14:55

Merge branch 'master' of github.com:argoproj/argo-rollouts into feat-…

fa20177

…plugin-system Signed-off-by: zachaller <zachaller@users.noreply.github.com>

zachaller marked this pull request as ready for review January 30, 2023 06:54

zachaller added 2 commits January 30, 2023 10:02

add test for error

acc21f1

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

remove test changes

6ceb973

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz reviewed Jan 30, 2023

View reviewed changes

cmd/sample-metrics-plugin/internal/plugin/plugin.go Outdated Show resolved Hide resolved

leoluz reviewed Jan 30, 2023

View reviewed changes

metricproviders/plugin/plugin.go Show resolved Hide resolved

leoluz reviewed Jan 30, 2023

View reviewed changes

metricproviders/plugin/plugin.go Outdated Show resolved Hide resolved

leoluz reviewed Jan 30, 2023

View reviewed changes

metricproviders/plugin/plugin.go Outdated Show resolved Hide resolved

zachaller added 2 commits January 30, 2023 14:26

small refactor

4a66c5b

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

refactor

00edfbb

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz reviewed Jan 30, 2023

View reviewed changes

cmd/rollouts-controller/main.go Outdated Show resolved Hide resolved

refactor

42a5b03

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz requested changes Jan 30, 2023

View reviewed changes

zachaller added 3 commits January 30, 2023 15:28

change type

9b7ca41

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

allow multiple plugins

14907af

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

allow multiple plugins

58b30b1

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

create /tmp dir for plugins

520b68d

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz requested changes Feb 2, 2023

View reviewed changes

zachaller added 18 commits February 2, 2023 11:20

review changes

be708e1

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

depend on forwarded functions when we can

924451c

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

refactor plugin download

49eab75

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

make config thread safe

be31179

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

update comments

ec93e92

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

lint

623ef07

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

download plugins on non leading controllers as well

cb321d8

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

fix tests

6e59272

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

lint

fc5957f

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

cleanup tests

fce8c6c

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

add new test

5d62e95

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

lint

825bfb2

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

fix test

b9400db

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

fix test

1cff9a2

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

more tests

ccf4aba

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

more tests

d02efd3

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

add godoc

4ecca70

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

use distroless for plugins chmod cp did not work

b4d5993

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz requested changes Feb 7, 2023

View reviewed changes

leoluz reviewed Feb 7, 2023

View reviewed changes

utils/plugin/plugin_downloader.go Outdated Show resolved Hide resolved

rename file

3f94bcc

Signed-off-by: zachaller <zachaller@users.noreply.github.com>

leoluz reviewed Feb 7, 2023

View reviewed changes

Merge branch 'master' of github.com:argoproj/argo-rollouts into feat-…

2cb9a35

…plugin-system

leoluz approved these changes Feb 7, 2023

View reviewed changes

zachaller merged commit 5c9bc03 into argoproj:master Feb 7, 2023

zachaller mentioned this pull request Feb 7, 2023

feat: add support for traffic router plugins #2573

Merged

harikrongali mentioned this pull request Feb 8, 2023

feat(analysis): add Apache SkyWalking as metrics provider #2491

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: metric plugin system based on hashicorp go-plugin #2514

feat: metric plugin system based on hashicorp go-plugin #2514

zachaller commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

codecov bot commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

zachaller commented Jan 13, 2023

skandragon commented Jan 24, 2023

zachaller commented Jan 25, 2023 •

edited

Loading

zachaller commented Jan 25, 2023

leoluz left a comment

leoluz left a comment

leoluz Feb 7, 2023

zachaller Feb 7, 2023

zachaller Feb 7, 2023

leoluz Feb 7, 2023

leoluz Feb 7, 2023

zachaller Feb 7, 2023

leoluz left a comment

sonarcloud bot commented Feb 7, 2023

feat: metric plugin system based on hashicorp go-plugin #2514

feat: metric plugin system based on hashicorp go-plugin #2514

Conversation

zachaller commented Jan 11, 2023 • edited Loading

github-actions bot commented Jan 11, 2023 • edited Loading

Go Published Test Results

codecov bot commented Jan 11, 2023 • edited Loading

Codecov Report

github-actions bot commented Jan 11, 2023 • edited Loading

E2E Tests Published Test Results

zachaller commented Jan 13, 2023

skandragon commented Jan 24, 2023

zachaller commented Jan 25, 2023 • edited Loading

zachaller commented Jan 25, 2023

leoluz left a comment

Choose a reason for hiding this comment

leoluz left a comment

Choose a reason for hiding this comment

leoluz Feb 7, 2023

Choose a reason for hiding this comment

zachaller Feb 7, 2023

Choose a reason for hiding this comment

zachaller Feb 7, 2023

Choose a reason for hiding this comment

leoluz Feb 7, 2023

Choose a reason for hiding this comment

leoluz Feb 7, 2023

Choose a reason for hiding this comment

zachaller Feb 7, 2023

Choose a reason for hiding this comment

leoluz left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 7, 2023

zachaller commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

codecov bot commented Jan 11, 2023 •

edited

Loading

github-actions bot commented Jan 11, 2023 •

edited

Loading

zachaller commented Jan 25, 2023 •

edited

Loading