Skip to content

Use ttrpc in the daemon for event publishing from shims#3195

Merged
estesp merged 2 commits intocontainerd:masterfrom
crosbymichael:ttrpc-love
Apr 10, 2019
Merged

Use ttrpc in the daemon for event publishing from shims#3195
estesp merged 2 commits intocontainerd:masterfrom
crosbymichael:ttrpc-love

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Apr 8, 2019

Instead of the current re-exec, where the shims are required to call back to the main containerd daemon to publish events, this uses a ttrpc server in the main daemon that the shim's can connect to so that they still get a socket to the daemon but not the grpc overhead.

@crosbymichael crosbymichael changed the title Ttrpc love Use ttrpc in the daemon for event publishing from shims Apr 8, 2019
@crosbymichael
Copy link
Copy Markdown
Member Author

We can test but hold off on this for a bit @stevvooe and I are talking about ways to make protobuild do this for us

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need a retry look for this? On this side the containerd daemon is serving the listener so a connect should always be expected to work instantly right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

containerd is serving this so it should always work

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess I mean why cant this code just say:

return winio.DialPipe(address, &timeout)

Like it does for unix?

Copy link
Copy Markdown
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

Awesome!

@jterry75
Copy link
Copy Markdown
Contributor

jterry75 commented Apr 8, 2019

@jhowardmsft - PTAL.

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@crosbymichael
Copy link
Copy Markdown
Member Author

Good to review now

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 9, 2019

Codecov Report

Merging #3195 into master will increase coverage by 4.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3195      +/-   ##
==========================================
+ Coverage   45.24%   49.29%   +4.04%     
==========================================
  Files         111      100      -11     
  Lines       11996     9446    -2550     
==========================================
- Hits         5428     4656     -772     
+ Misses       5733     3964    -1769     
+ Partials      835      826       -9
Flag Coverage Δ
#linux 49.29% <0%> (ø) ⬆️
#windows ?
Impacted Files Coverage Δ
services/server/server_linux.go 0% <0%> (ø) ⬆️
runtime/v2/shim/shim.go 0% <0%> (ø) ⬆️
services/server/server.go 1.73% <0%> (+0.23%) ⬆️
runtime/v2/shim/shim_unix.go 0% <0%> (ø) ⬆️
remotes/docker/auth.go 63.82% <0%> (-3.97%) ⬇️
remotes/docker/status.go 21.42% <0%> (-3.58%) ⬇️
remotes/docker/fetcher.go 61.53% <0%> (-3.05%) ⬇️
platforms/cpuinfo.go 3.77% <0%> (-0.85%) ⬇️
log/context.go 36.84% <0%> (-0.66%) ⬇️
namespaces/context.go 55% <0%> (-0.56%) ⬇️
... and 63 more

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 db3a863...a6f587e. Read the comment docs.

@crosbymichael crosbymichael added this to the 1.3 milestone Apr 9, 2019
Copy link
Copy Markdown
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM

@jterry75
Copy link
Copy Markdown
Contributor

@crosbymichael - Thanks again for doing this!

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 475619c into containerd:master Apr 10, 2019
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.

5 participants