Skip to content

Allow explicit configuration of TTRPC address#3555

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
kevpar:ttrpc-address
Aug 22, 2019
Merged

Allow explicit configuration of TTRPC address#3555
crosbymichael merged 1 commit intocontainerd:masterfrom
kevpar:ttrpc-address

Conversation

@kevpar
Copy link
Copy Markdown
Member

@kevpar kevpar commented Aug 19, 2019

Previously the TTRPC address was generated as ".ttrpc".
This change now allows explicit configuration of the TTRPC address, with
the default still being the old format if no value is specified.

As part of this change, a new configuration section is added for TTRPC
listener options.

Signed-off-by: Kevin Parsons [email protected]

@kevpar
Copy link
Copy Markdown
Member Author

kevpar commented Aug 19, 2019

@jterry75 PTAL

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 19, 2019

Build succeeded.

jterry75
jterry75 reviewed Aug 19, 2019
View reviewed changes
Comment thread services/server/config/config.go Outdated
jterry75
jterry75 requested changes Aug 19, 2019
View reviewed changes
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.

I think you left an extra flag.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 19, 2019

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 22, 2019

Build succeeded.

@kevpar
Allow explicit configuration of TTRPC address
d7e1b25 
Previously the TTRPC address was generated as "<GRPC address>.ttrpc".
This change now allows explicit configuration of the TTRPC address, with
the default still being the old format if no value is specified.

As part of this change, a new configuration section is added for TTRPC
listener options.

Signed-off-by: Kevin Parsons <[email protected]>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 22, 2019

Build succeeded.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 22, 2019

Codecov Report

Merging #3555 into master will decrease coverage by 5.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3555      +/-   ##
==========================================
- Coverage   42.29%   37.24%   -5.05%     
==========================================
  Files         126       84      -42     
  Lines       13871    11560    -2311     
==========================================
- Hits         5867     4306    -1561     
+ Misses       7118     6654     -464     
+ Partials      886      600     -286
Flag Coverage Δ
#linux ?
#windows 37.24% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
runtime/v2/manager.go 4.69% <0%> (-0.03%) ⬇️
runtime/v2/binary.go 0% <0%> (ø) ⬆️
services/server/server.go 1.19% <0%> (-0.01%) ⬇️
runtime/v2/shim/util.go 0% <0%> (ø) ⬆️
runtime/v2/shim/shim.go 0% <0%> (ø) ⬆️
archive/tar_opts.go 11.76% <0%> (-47.06%) ⬇️
cio/io.go 1.4% <0%> (-45.08%) ⬇️
snapshots/native/native.go 1.79% <0%> (-41.26%) ⬇️
archive/tar.go 19.18% <0%> (-28.78%) ⬇️
metadata/snapshot.go 23.86% <0%> (-24.05%) ⬇️
... and 45 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 bd46ea5...d7e1b25. Read the comment docs.

@kevpar
Copy link
Copy Markdown
Member Author

kevpar commented Aug 22, 2019

@jterry75 : Figured out the CI failure, PTAL again.
@crosbymichael / @dmcgowan : It sounded like Justin talked to you about this change. I got CI to pass. Can one of you please take a look?

jterry75
jterry75 approved these changes Aug 22, 2019
View reviewed changes
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

jterry75 commented Aug 22, 2019

@crosbymichael - Got it to work! PTAL

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Aug 22, 2019

We will have to do something with backwards compat. If there is an old shim or someone hasn't updated, providing the extra flag will cause an error

ctr: flag provided but not defined: -ttrpc-address
Usage of /usr/local/bin/containerd-shim-runc-v2:
  -address string
        grpc address back to main containerd
  -bundle string
        path to the bundle if not workdir
  -debug
        enable debug output in logs
  -id string
        id of the task
  -namespace string
        namespace that owns the shim
  -publish-binary string
        path to publish binary (used for publishing events) (default "containerd")
  -socket string
        abstract socket path to serve
: exit status 2: unknown

@jterry75
Copy link
Copy Markdown
Contributor

jterry75 commented Aug 22, 2019

@crosbymichael - I thought that we explicitly disallowed this behavior. We dont support a compat matrix between containerd vs shim versions. containerd and shim are shipped as a single version so they need to be deployed together.

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Aug 22, 2019 •
edited
Loading

nvm, ttrpc support is only in master, it has not gone out in a release. We are good with this change

LGTM

@crosbymichael crosbymichael merged commit 1be6ee5 into containerd:master Aug 22, 2019
@crosbymichael crosbymichael added this to the 1.3 milestone Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jterry75 jterry75 jterry75 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.3

Development

Successfully merging this pull request may close these issues.

4 participants

@kevpar @codecov-io @jterry75 @crosbymichael