Delta Diff MVP: Plugins manager #5153

Jonathan-Rosenberg · 2023-01-31T16:00:44Z

The plugin manager will be used to store go-plugin application clients and to load gRPC clients needed to communicate with gRPC server.
Each type of "plugin service" (like Diff Service for example) will initialize its own plugin manager with its plugin type.
This PR is the final version of #5024

Optional
This is a "go-plugin implementation" diagram to help understand this package:

Generated code: Code generated by running protoc
Implemented code: Code needed to be implemented by us.
go-plugin code: Some related go-plugin code

Closes #4965

arielshaqed

Thanks! Very nice & useful.

A bunch of minor-but-important things that I think we should improve.

pkg/plugins/manager.go

arielshaqed · 2023-02-02T10:32:05Z

pkg/plugins/manager.go

+	if m == nil || m.pluginApplicationClients == nil {
+		return ErrUninitializedManager
+	}


Do we ever need an uninitialized plugin manager? I'd just remove it if that's impossible. It's not like the caller can do anything on receipt of this error other than panic. And a nil pointer dereference here will be more informative than the error message for a programming error.

arielshaqed · 2023-02-02T11:04:17Z

pkg/plugins/manager.go

+}
+
+// RegisterPlugin is used to register a new plugin client with the corresponding plugin type.
+func (m *Manager[T]) RegisterPlugin(name string, id PluginIdentity, auth PluginHandshake, p plugin.Plugin) error {


Suggested change

func (m *Manager[T]) RegisterPlugin(name string, id PluginIdentity, auth PluginHandshake, p plugin.Plugin) error {

func (m *Manager[T]) RegisterPlugin(name string, id PluginIdentity, handshake PluginHandshake, p plugin.Plugin) error {

It's a handshake not an auth:

No secrets, can and should log it.

Comes from regular configuration (e.g. K8s configmap not secret).

it kinda is- the plugin client and server agree on a certain key and value that identify one another

arielshaqed · 2023-02-02T11:05:29Z

pkg/plugins/manager.go

+	Cmd *exec.Cmd
+}
+
+// PluginHandshake includes handshake properties for the plugin.


I wonder, isn't this tied 1:1 with PluginIdentity? I know we've been over this before, I don't see in the code why they need to be separate.

the previous argument was that a handshake is more of an auth entity rather than an identifier.

arielshaqed · 2023-02-02T11:10:55Z

pkg/plugins/manager.go

+}
+
+// LoadPluginClient loads a Client of type T.
+func (m *Manager[T]) LoadPluginClient(name string) (*T, error) {


Consider spec'cing the manager on *T when you create it, and returning T here. For instance, if T is an interface (it often will be!) then this is a pointer-to-interface, which is really weird. Even if the current implementation in the library returns a struct, there is no requirement for it to stay that way.

What do you mean by Consider **spec'cing** the manager on *T?

The type Manager[T] returns clients of type *T. This is technically correct if the client is a struct, and we instantiate it on the struct type:

type MyClient struct { /* stuff */ } // But really the client is *MyClient func a(m Manager[MyClient], name string) { var client *MyClient = m.LoadPluginClient(name) // client stuff

This works mostly because *MyClient satisfies the MyClient interface (the converse is not really true!).

BUT if T is an interface type, then Manager[T].LoadPluginClient returns a *T, which is a pretty useless type.

AFAIU the more Go-like interface would be to return a T from Manager[T].LoadPluginClient. In the *MyClient case above, you would need to use Manager[*MyClient] as the manager type. But that makes sense: the client type really is *MyClient there!

arielshaqed · 2023-02-02T11:12:41Z

pkg/plugins/manager.go

+	c, ok := m.pluginApplicationClients[name]
+	if !ok {
+		return nil, ErrPluginNameNotFound
+	}
+	grpcPluginClient, err := c.Client()
+	if err != nil {
+		return nil, err
+	}
+	rawGrpcClientStub, err := grpcPluginClient.Dispense(name) // Returns an implementation of the stub service.
+	if err != nil {
+		return nil, err
+	}


This code needs a mutex to be thread-safe.

True, it's implemented on the next PR 🤫

arielshaqed · 2023-02-02T11:17:02Z

pkg/plugins/manager_test.go

+	return cmd
+}
+
+// This is not a real test. This is the plugin server that will be initialized by the tests


Then it shouldn't be a test. Perhaps this needs to be TestMain?

It's triggered by the tests themselves, not initialized at the beginning of the tests

…` -> `uint` Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

talSofer

Thanks! added a few minor comments

talSofer · 2023-02-02T08:35:17Z

pkg/plugins/manager_test.go

+	m           *Manager[PingPongStub]
+	name        string
+	p           plugin.Plugin
+	err         error


Suggested change

err error

expectedErr error

pkg/plugins/manager.go

Co-authored-by: talSofer <tal.sofer@treeverse.io>

arielshaqed

The TestPluginServer hook is a really strange way to run the test plugin server. It should just be a separate program. Failing that, if building 2 binaries for a test is too hard because Go, it might execute as a TestMain and then do one of 2 things.

But I want to question even that. Why run the plugin server as a separate process? It's just a web server, can't we run it in a separate goroutine?

arielshaqed · 2023-02-05T09:24:45Z

pkg/plugins/manager.go

+}
+
+// LoadPluginClient loads a Client of type T.
+func (m *Manager[T]) LoadPluginClient(name string) (*T, error) {


The type Manager[T] returns clients of type *T. This is technically correct if the client is a struct, and we instantiate it on the struct type:

type MyClient struct { /* stuff */ } // But really the client is *MyClient func a(m Manager[MyClient], name string) { var client *MyClient = m.LoadPluginClient(name) // client stuff

This works mostly because *MyClient satisfies the MyClient interface (the converse is not really true!).

BUT if T is an interface type, then Manager[T].LoadPluginClient returns a *T, which is a pretty useless type.

AFAIU the more Go-like interface would be to return a T from Manager[T].LoadPluginClient. In the *MyClient case above, you would need to use Manager[*MyClient] as the manager type. But that makes sense: the client type really is *MyClient there!

arielshaqed · 2023-02-05T09:25:23Z

pkg/plugins/manager.go

+	if m == nil || m.pluginApplicationClients == nil {
+		return
+	}


Not sure when this is useful.

arielshaqed · 2023-02-05T09:27:01Z

pkg/plugins/manager_test.go

+// This is not a real test. This is the plugin server that will be triggered by the tests
+func TestPluginServer(*testing.T) {


If this is not a test, it should start with something other than Test. Also not sure why it takes *testing.T parameter.

It's not a test, yet the command that runs it is go test which accepts the -test.run flag that specifies the tests it should run. Unfortunately, golang don't allow tests to be named other than Test... and must accept *testing.T as an argument:

You write a test by creating a file with a name ending in _test.go that contains functions named TestXXX with signature func (t *testing.T).

I like the ping-pong protobuf and additional classes, since it explains very well how plugins and grcp work. However, I'm wondering about this methodology, because this test doesn't (and shouldn't) test the logic of the plugins. I would expect this test to mock both plugin.Plugin and plugin.Client, so it can become a strict unit test.

One problem is the plugin.Client is a struct and not an interface, but I suggest creating an interface on our side in order to allow mocking it as well:

type Client interface { Client() (ClientProtocol, error) }

arielshaqed · 2023-02-05T09:28:23Z

pkg/plugins/manager_test.go

+		return
+	}
+
+	defer os.Exit(0)


Not sure why a test helper should ever exit. This is even worse, it exits successfully when something strange happens. But if that happens, the test succeeds even though it should fail.

true this is useless

Jonathan-Rosenberg · 2023-02-05T10:18:52Z

@arielshaqed, the plugin manager is dependent on the go-plugin (Hashicorp's) package which receives an exec.Command as the plugin server's binary it should run. It also kills it when the plugin's job is over.
It means that a running go routine is not an option.
We can implement a separate process, but I think it's quite an overkill.

arielshaqed

Thanks for your patience explaining the go-plugin test horridness to me!

pkg/plugins/manager.go

arielshaqed · 2023-02-05T10:53:51Z

Thanks for your patience explaining the go-plugin test horridness to me!

While we're there, can you use one of the go-plugin test plugin plugins, maybe this one?

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

johnnyaug

Mainly cleanups and naming. Also, I really enjoyed reading the tests, they helped me understand the world of go-plugins. However, not sure the methodology is optimal for these specific tests.

johnnyaug · 2023-02-02T11:21:35Z

pkg/logging/logger.go

+	IsDebugging() bool
+	IsInfo() bool
+	IsError() bool
+	IsWarn() bool


What are these for?
Also - these are some very confusing name, each in its own way.

johnnyaug · 2023-02-02T11:23:48Z

pkg/plugins/Logger.go

@@ -0,0 +1,64 @@
+package plugins
+
+import (


filename should be in lowercase

johnnyaug · 2023-02-02T11:25:26Z

pkg/plugins/test.proto

+  Sound sound = 1;
+}
+
+service PingPong {


johnnyaug · 2023-02-02T11:33:02Z

pkg/plugins/test_utils.go

+	return nil
+}
+
+// GRPCClient will return the Delta diff GRPC custom client


I think the comment is outdated?

johnnyaug · 2023-02-02T11:33:21Z

pkg/plugins/test_utils.go

+}
+
+// GRPCClient will return the Delta diff GRPC custom client
+func (p GRPCPlugin) GRPCClient(ctx context.Context, broker *plugin.GRPCBroker, c *grpc.ClientConn) (interface{}, error) {


Suggested change

func (p GRPCPlugin) GRPCClient(ctx context.Context, broker *plugin.GRPCBroker, c *grpc.ClientConn) (interface{}, error) {

func (p GRPCPlugin) GRPCClient(_ context.Context, _ *plugin.GRPCBroker, c *grpc.ClientConn) (interface{}, error) {

Also in other places in the file

johnnyaug · 2023-02-05T13:22:47Z

pkg/plugins/manager_test.go

+// This is not a real test. This is the plugin server that will be triggered by the tests
+func TestPluginServer(*testing.T) {


I like the ping-pong protobuf and additional classes, since it explains very well how plugins and grcp work. However, I'm wondering about this methodology, because this test doesn't (and shouldn't) test the logic of the plugins. I would expect this test to mock both plugin.Plugin and plugin.Client, so it can become a strict unit test.

One problem is the plugin.Client is a struct and not an interface, but I suggest creating an interface on our side in order to allow mocking it as well:

type Client interface { Client() (ClientProtocol, error) }

johnnyaug · 2023-02-05T13:35:30Z

pkg/plugins/manager.go

+		MagicCookieKey:   auth.Key,
+		MagicCookieValue: auth.Value,
+	}
+	cmd := id.Cmd


It's confusing to me that an "id" contains a command.

johnnyaug · 2023-02-05T13:36:10Z

pkg/plugins/manager.go

+}
+
+// LoadPluginClient loads a Client of type T.
+func (m *Manager[T]) LoadPluginClient(name string) (T, error) {


Suggested change

func (m *Manager[T]) LoadPluginClient(name string) (T, error) {

func (m *Manager[T]) DispensePluginClient(name string) (T, error) {

I think is more accurate?

johnnyaug · 2023-02-05T13:37:38Z

pkg/plugins/manager.go

+//	grpcPluginClient, err := c.Client() // Returns a plugin.GRPCClient
+//	rawGrpcClientStub, err := grpcPluginClient.Dispense(name) // Calls grpcPluginClient's GRPCClient method and returns the gRPC stub.
+//	grpcClient, ok := rawGrpcClientStub.(Differ) // Asserts the expected type of stub client.


This is an explanation of an implementation detail.

Jonathan-Rosenberg added 5 commits January 30, 2023 17:49

add plugins manager

e95dac2

go.mod

e06c047

handle wrong type of plugin in the manager

94f9619

goimports

e602d36

add tests

65a905b

Jonathan-Rosenberg added the exclude-changelog PR description should not be included in next release changelog label Jan 31, 2023

Jonathan-Rosenberg requested review from johnnyaug, arielshaqed and talSofer January 31, 2023 16:00

Jonathan-Rosenberg added 2 commits January 31, 2023 18:25

MemLogger

f208a9e

add validation that the TestPluginServer run is intentional

65975e6

arielshaqed requested changes Feb 2, 2023

View reviewed changes

Jonathan-Rosenberg and others added 4 commits February 2, 2023 13:58

Update pkg/plugins/manager.go - Version -> ProtocolVersion + `int…

ab32173

…` -> `uint` Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Version -> ProtocolVersion

24ffa6d

remove unnecessary conversion

758c0f6

change from *Cmd to Cmd

cc10bfd

talSofer approved these changes Feb 2, 2023

View reviewed changes

Jonathan-Rosenberg and others added 5 commits February 2, 2023 15:42

update plugins manager errors

4954061

Update pkg/plugins/manager.go - change Manager documentation

34a6b5c

Co-authored-by: talSofer <tal.sofer@treeverse.io>

Update pkg/plugins/manager.go - rawGrpcClient -> rawGrpcClientStub

c965993

Co-authored-by: talSofer <tal.sofer@treeverse.io>

plugins manager test: test case's "err" -> "expectedErr"

7ddf549

initialized -> triggered

aeb568f

Jonathan-Rosenberg requested a review from arielshaqed February 2, 2023 13:48

arielshaqed requested changes Feb 5, 2023

View reviewed changes

Jonathan-Rosenberg requested a review from arielshaqed February 5, 2023 10:19

Jonathan-Rosenberg added 2 commits February 5, 2023 12:19

PR changes

b616e5e

remove unnecessary error

951f72a

arielshaqed approved these changes Feb 5, 2023

View reviewed changes

pkg/plugins/manager.go Outdated Show resolved Hide resolved

pkg/plugins/manager.go Outdated Show resolved Hide resolved

Jonathan-Rosenberg and others added 3 commits February 5, 2023 13:10

docs

fbe8c7b

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Update pkg/plugins/manager.go docs

cba28b2

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

go fmt

e1bef4f

Jonathan-Rosenberg merged commit d996d9e into master Feb 5, 2023

Jonathan-Rosenberg deleted the feature/grpc-plugin-manager branch February 5, 2023 13:26

johnnyaug suggested changes Feb 5, 2023

View reviewed changes

rmoff mentioned this pull request Feb 14, 2023

Docs footer - replace Treeverse with lakeFS logo (#devex-internal/103) #5235

Merged

This was referenced Feb 22, 2023

Add mastodon icon and profile link to docs footer #5307

Merged

[DX] Web UI: Add spinner to setup screen button #5313

Merged

This was referenced Mar 6, 2023

Update/clarify S3 gateway config notes #5390

Merged

[docs] Add a little more detail to RBAC policy logic #5395

Closed

	func (m *Manager[T]) RegisterPlugin(name string, id PluginIdentity, auth PluginHandshake, p plugin.Plugin) error {
	func (m *Manager[T]) RegisterPlugin(name string, id PluginIdentity, handshake PluginHandshake, p plugin.Plugin) error {

		// This is not a real test. This is the plugin server that will be triggered by the tests
		func TestPluginServer(*testing.T) {

	func (p GRPCPlugin) GRPCClient(ctx context.Context, broker plugin.GRPCBroker, c grpc.ClientConn) (interface{}, error) {
	func (p GRPCPlugin) GRPCClient(_ context.Context, _ plugin.GRPCBroker, c grpc.ClientConn) (interface{}, error) {

	func (m *Manager[T]) LoadPluginClient(name string) (T, error) {
	func (m *Manager[T]) DispensePluginClient(name string) (T, error) {

Delta Diff MVP: Plugins manager #5153

Delta Diff MVP: Plugins manager #5153

Conversation

Jonathan-Rosenberg commented Jan 31, 2023 • edited Loading

arielshaqed 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

Jonathan-Rosenberg Feb 2, 2023 • 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

talSofer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed 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

Jonathan-Rosenberg commented Feb 5, 2023

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed commented Feb 5, 2023

johnnyaug 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

Jonathan-Rosenberg commented Jan 31, 2023 •

edited

Loading

Jonathan-Rosenberg Feb 2, 2023 •

edited

Loading