Skip to content

Allow reuse client for external GRPC services#3208

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
mxpv:client
Apr 16, 2019
Merged

Allow reuse client for external GRPC services#3208
crosbymichael merged 1 commit intocontainerd:masterfrom
mxpv:client

Conversation

@mxpv
Copy link
Copy Markdown
Member

@mxpv mxpv commented Apr 11, 2019 •
edited
Loading

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 [email protected]

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 14, 2019 •
edited
Loading

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.

@mxpv mxpv mentioned this pull request Apr 15, 2019
Closed
13 tasks
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Apr 16, 2019

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

@mxpv
Copy link
Copy Markdown
Member Author

mxpv commented Apr 16, 2019

@crosbymichael that works too. Done.

dmcgowan
dmcgowan approved these changes Apr 16, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

crosbymichael commented Apr 16, 2019

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

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mxpv @codecov-io @crosbymichael @dmcgowan