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

Allow reuse client for external GRPC services #3208

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

mxpv
Copy link
Member

@mxpv mxpv commented Apr 11, 2019

In firecracker-containerd project we use GRPC plugins to extend containerd's functionality and we have to manage a separate client in order to make calls.
It would be great if we could reuse existing containerd's client (e.g. reuse connection object, namespace handling, reconnect logic, etc) to invoke external grpc services as well.

The proposed solution adds ExternalService and looks as follows:

	client, err := containerd.New(...)
	if err != nil {
		panic(err)
	}

	var firecrackerClient proto.FirecrackerClient
	client.ExternalService(func(conn *grpc.ClientConn) {
		firecrackerClient = proto.NewFirecrackerClient(conn)
	})

	firecrackerClient.XXX();

Signed-off-by: Maksym Pavlenko pavlenko.maksym@gmail.com

@codecov-io
Copy link

codecov-io commented Apr 14, 2019

Codecov Report

Merging #3208 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3208   +/-   ##
=======================================
  Coverage   44.63%   44.63%           
=======================================
  Files         113      113           
  Lines       12161    12161           
=======================================
  Hits         5428     5428           
  Misses       5898     5898           
  Partials      835      835
Flag Coverage Δ
#linux 48.65% <ø> (ø) ⬆️
#windows 39.86% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32e788a...70e40db. Read the comment docs.

@crosbymichael
Copy link
Member

I'd rather just export the Conn() on the client than make a public func like this. Can we do that?

@mxpv
Copy link
Member Author

mxpv commented Apr 16, 2019

@crosbymichael that works too. Done.

Signed-off-by: Maksym Pavlenko <makpav@amazon.com>
Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 665715b into containerd:master Apr 16, 2019
@mxpv mxpv deleted the client branch April 16, 2019 19: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.

4 participants