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

Add configuration option for cri-containerd #37519

Merged
merged 1 commit into from
Jul 25, 2018
Merged

Conversation

dmcgowan
Copy link
Member

@dmcgowan dmcgowan commented Jul 24, 2018

Disable cri plugin by default in containerd and allows an option to enable the plugin. This only
has an effect on containerd when supervised by dockerd. When containerd is managed outside of dockerd, the configuration is not effected.

fixes #37507

Disable cri plugin by default in containerd and
allows an option to enable the plugin. This only
has an effect on containerd when supervised by
dockerd. When containerd is managed outside of
dockerd, the configuration is not effected.

Signed-off-by: Derek McGowan <derek@mcgstyle.net>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

@cpuguy83
Copy link
Member

Is it really even feasible for someone to use docker managed containerd with k8s?
I would opt to keep this disabled until containerd packaging is split from moby.

@@ -31,6 +31,14 @@ func (r *remote) setDefaults() {
if r.OOMScore == 0 {
r.OOMScore = -999
}

for key, conf := range r.pluginConfs.Plugins {
if conf == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little weird to handle an unset.

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked this more than having a WithDisablePlugin, but could do that as an alternative. This isn't a publicly used interface though and I am trying to refactor these in another PR that is already open.

@crosbymichael
Copy link
Contributor

From the recent comments, I think we should just handle this statically and disable it. Like @AkihiroSuda said, users that wish to use containerd for more can use the --contianerd flag to point to a managed daemon.

@dmcgowan
Copy link
Member Author

Is it really even feasible for someone to use docker managed containerd with k8s?
Like @AkihiroSuda said, users that wish to use containerd for more can use the --containerd flag to point to a managed daemon.

This flag gives users a much easier to way to try containerd/cri and I think it very reasonable that someone running docker may want to try containerd with k8s using the containerd which is already running on their machine. If the question is whether that is an acceptable configuration in production, I am not sure if the cri plugin or 18.06-ce are certified for production kubernetes.

Copy link
Member

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

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

LGTM wrt having a knob to turn where default is off

@justincormack
Copy link
Contributor

This flag seems harmless to me and as @dmcgowan says it allows people to try out cri in a development environment.

@crosbymichael
Copy link
Contributor

LGTM

@codecov
Copy link

codecov bot commented Jul 25, 2018

Codecov Report

Merging #37519 into master will increase coverage by 0.53%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master   #37519      +/-   ##
==========================================
+ Coverage   34.95%   35.49%   +0.53%     
==========================================
  Files         610      610              
  Lines       44886    44893       +7     
==========================================
+ Hits        15690    15934     +244     
+ Misses      27077    26800     -277     
- Partials     2119     2159      +40

@yongtang
Copy link
Member

All tests passed except:

codecov/patch — 14.28% of diff hit (target 50%)

which I think is irrelevant. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker-containerd started occupying port 10010
8 participants