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

Init KEP to migrate in-tree dockershim to out-of-tree #866

Closed
wants to merge 1 commit into from

Conversation

resouer
Copy link

@resouer resouer commented Mar 1, 2019

A initial KEP to discuss about criteria and plan to migrate in-tree dockershim to out-of-tree.

Ref: kubernetes/kubernetes#73933

Please note this KEP is not target on 1.14 or any specific release recently, it's designed to be a join effort spanning on many future releases and requires carefully revise and thinking.

/assign @yujuhong @dchen1107

/cc @dims @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 1, 2019
@k8s-ci-robot k8s-ci-robot requested a review from dims March 1, 2019 22:48
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 1, 2019

## Summary

CRI for docker (i.e. dockershim) is currently a build-in container runtime in kubelet code base. This proposal aims at a concrete deprecation and migration plan for separating dockershim from kubelet to out-of-tree without breaking current production users and WIP engineering efforts.
Copy link
Member

Choose a reason for hiding this comment

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

build-in -> built-in

- Updating all the eco-system tools to support the new cri-dockerd.
- Many people use the built in dockershim for in-cluster image build. While that may not be something we recommend for a variety of reasons, it will be a breaking change for these users.
- CRI is still in alpha,should probably get a 1.0 out there splitting out dockershim completely from kubelet.
- Existed CNI and CSI plugins may also be affected.
Copy link
Member

Choose a reason for hiding this comment

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

Existing instead of Existed

- Many people use the built in dockershim for in-cluster image build. While that may not be something we recommend for a variety of reasons, it will be a breaking change for these users.
- CRI is still in alpha,should probably get a 1.0 out there splitting out dockershim completely from kubelet.
- Existed CNI and CSI plugins may also be affected.

Copy link
Member

Choose a reason for hiding this comment

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

cri-dockerd will still be vendoring kubernetes/kubernetes for a while during the transition. vendoring kubernetes/kubernetes is not easy

Copy link
Author

Choose a reason for hiding this comment

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

ACK, it's a good point

- A concrete dockershim deprecation criteria.
- A brief plan to deprecate dockershim spanning multiple releases.
- Issues need to be fixed in kubelet in order to separate dockershim from it.

Copy link
Member

@dims dims Mar 6, 2019

Choose a reason for hiding this comment

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

Can we add another goal of writing up a criteria for adopting one of the existing CRI runtimes (for example containerd) that can eventually replace cri-dockerd?

Choose a reason for hiding this comment

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

considering that there are too many docker users, it be better not to cease cri-dockerd here.

Copy link
Member

Choose a reason for hiding this comment

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

SIG Node doesn't want to make a call which CRI implementation the vendor / the user should use. Instead, we design the API to define how the runtime engaging with Kubelet and nodes, a test suite to verify / qualify the functionalities, a portable toolset (cri-tool) for debuggability and introspect.

- Mark in-tree dockershim as "maintenance mode":
- CRI generic changes/features can continue on dockershim.
- WIP efforts on dockershim can continue and go to complete.
- dockershim/docker specific changes/features should be rejected.
Copy link
Member

Choose a reason for hiding this comment

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

we will continue updating the docker/docker dependency. right?

Choose a reason for hiding this comment

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

Perhaps continue updating the docker/docker dependency is a must.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we still need to update Docker when needed.

@dims
Copy link
Member

dims commented Mar 22, 2019

@yujuhong @derekwaynecarr @PatrickLang @dchen1107 does this seem ready? (can we merge and iterate?)

@dims
Copy link
Member

dims commented Mar 26, 2019

Another benefit as noted by @liggitt in the dependency doc[1], docker has just 7 month support[2]. So the quicker we get off dockerd shim the better we will be.

[1] https://docs.google.com/document/d/1WA8N7C48nkJmme9a96DU0o9jBpeycPhht8WF-Eam9QQ/edit?ts=5c9a308b#heading=h.46nlk76xzfpg
[2] https://docs.docker.com/install/#support

- support to get container logs when docker uses journald as the driver.
- logic of moving docker processes to a given cgroup
- TBD anything else?
- Package in-tree dockershim separately from kubelet and provide a "option" to enable/disable it.
Copy link
Member

Choose a reason for hiding this comment

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

@resouer let's please add a "new binary named cri-dockerd" with code being in-tree (line number 113)

Copy link
Member

Choose a reason for hiding this comment

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

We will also need to ensure that this binary gets into the release artifacts

Choose a reason for hiding this comment

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

@dims do you mean that cri-dockerd could be compiled to an independent binary like kubelet here?

Copy link
Member

Choose a reason for hiding this comment

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

@liggitt
Copy link
Member

liggitt commented Mar 28, 2019

Long-term, do we want to maintain a dockershim CRI implementation? If not, would it make more sense to focus efforts on getting CRI to v1, then immediately start the deprecation period for dockershim (6 months, according to https://kubernetes.io/docs/reference/using-api/deprecation-policy/#deprecating-a-flag-or-cli) in favor of existing out of tree CRI impls?

- Package in-tree dockershim separately from kubelet and provide a "option" to enable/disable it.
- Ensure e2e/Node e2e test framework is CRI generic and test cases are independent of container runtime.

Step 2: Work out a out-of-tree CRI for docker
Copy link
Member

Choose a reason for hiding this comment

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

does this not already exist in the form of containerd?

Copy link
Author

@resouer resouer Apr 1, 2019

Choose a reason for hiding this comment

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

I assume they are different. During recent sig-node meeting, folks from Docker Inc are willing to maintain a CRI-docker.

Copy link
Member

Choose a reason for hiding this comment

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

All recent versions of docker are built on top of containerd, so I'm unclear on what functionality dockershim has that cri-containerd does not.

@Random-Liu do you know if there is any reason to use dockershim over cri-containerd with recent versions of docker?

Copy link
Member

@Random-Liu Random-Liu Apr 20, 2019

Choose a reason for hiding this comment

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

I think in the long run, we do expect people to move to CRI conformant container runtimes like containerd and cri-o to be able to leverage new Kubernetes features.

However, people may still need more time to make that move, one of the biggest reason is that with dockershim, you can actually see all your Kubernetes containers in Docker, and people's tools may have been relying on that. For example, people may have built security tools, monitoring tools, logging tools around that.

With containerd or cri-o, you won't see Kubernetes containers in Docker any more. It takes time for people to migrate their tooling, that's why we need to keep dockershim for a while.

Copy link
Member

Choose a reason for hiding this comment

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

Another reason is that today's Kubernetes windows support is still based on dockershim. cri-containerd Windows is WIP, but not finished yet.

Choose a reason for hiding this comment

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

so we cloud remain both mechanisms currently and depreciated the old way gradually.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there are still features that are supported by docker/dockershim, but not with containerd.

- The number of affected users maybe large.
- Updating all the eco-system tools to support the new cri-dockerd.
- Many people use the built in dockershim for in-cluster image build. While that may not be something we recommend for a variety of reasons, it will be a breaking change for these users.
- CRI is still in alpha,should probably get a 1.0 out there splitting out dockershim completely from kubelet.
Copy link
Member

Choose a reason for hiding this comment

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

I strongly support progressing CRI to v1, for the sake of all the runtime implementations. I'm less clear on the benefit of extracting dockershim to a standalone CRI. Did you consider deprecating dockershim in favor of existing CRI implementations instead?

Copy link
Author

Choose a reason for hiding this comment

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

I will try to confirm if sig-node or Docker Inc has a plan to support a CRI-docker for long time (which is what I heard during the sig meeting). If not, deprecating dockershim directly will be the goal.

Copy link
Member

Choose a reason for hiding this comment

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

We talked about this at SIG Node, including the folks from Docker Inc. Docker folks plan to maintain cri-dockershim.


Actions:

- Refactoring e2e/Node e2e test framework to include CRI for docker installation (or use other CRI container runtime).
Copy link
Member

Choose a reason for hiding this comment

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

this would be extremely helpful for existing CRI impls to test/verify compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

Cluster/node e2e doesn't care what CRI implementation you use under the hood. I think maybe I've missed the point of this statement.

Copy link
Author

Choose a reason for hiding this comment

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

There're two points in my mind:

  1. Ensure cluster/node e2e are 100% CRI focused. For instance: DockerValidator should use the CRI, not the Docker API kubernetes#55414
  2. Ensure test-infra install CRI-docker or cri-containerd binary in e2e machines. Currently, they install Docker binary only.

@dims
Copy link
Member

dims commented Mar 28, 2019

@resouer WDYT of @liggitt 's alternative? I am inclined to support that as it is less work overall

@resouer
Copy link
Author

resouer commented Mar 28, 2019

@dims I agree, will update the KEP to reflect this proposal.

@k8s-ci-robot k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Apr 22, 2019
- kubelet has no dependency on dockershim/docker in its whole lifecycle.
- All node related features are CRI generic and have no "back door" dependency on dockershim/docker.
- Deprecate and remove, or replace all Docker-specific features.
- Reasonable benchmark result of performance degradation after moving dockershim to out-of-tree.
Copy link

Choose a reason for hiding this comment

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

What percentage of degradation is acceptable (10%) ?

Copy link
Member

Choose a reason for hiding this comment

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

10% might not be acceptable by the wider audience.
but my guess is that it's going to end up being less than that 🤔

@tedyu
Copy link

tedyu commented Nov 1, 2019

Currently starting dockershim is governed by a flag:

			if kubeletServer.KubeletFlags.ExperimentalDockershim {
				if err := RunDockershim(&kubeletServer.KubeletFlags, kubeletConfig, stopCh); err != nil {

Would there be another flag or enum which is used to choose the effective out-of-tree container runtime ?
My assumption is that the kubeletServer.KubeletFlags, kubeletConfig and stopCh parameters provide sufficient information for starting the chosen out-of-tree container runtime.

@bg-chun
Copy link
Member

bg-chun commented Jan 5, 2020

/cc @bg-chun

@bg-chun
Copy link
Member

bg-chun commented Jan 5, 2020

How is it going?
For my work(container isolation of hugepages).
I updated CRI to have hugepages limit for containers then I am working for Container Runtimes.
Containerd and cri-o doesn't has any issue for my work But, Docker has tiny issue due to the dockershim dependency :( .
I jsut hope I will see this changes in 2020.

@liggitt
Copy link
Member

liggitt commented Jan 22, 2020

cc @dashpole - what does kubelet/cadvisor do differently when configured to speak to a CRI instead of dockershim? does cadvisor still contact the container runtime directly or would this let us stop enabling runtime-specific integrations in cadvisor?

@dashpole
Copy link
Contributor

what does kubelet/cadvisor do differently when configured to speak to a CRI instead of dockershim? does cadvisor still contact the container runtime directly or would this let us stop enabling runtime-specific integrations in cadvisor?

The only difference is that it disables metrics provided by the CRI. It still contacts the container runtime directly to get metadata about containers.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 21, 2020
@warmchang
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 22, 2020
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 21, 2020
@frittentheke
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2020
@BenTheElder
Copy link
Member

If @resouer is working on this PR it won't go stale, but just removing the stale label is not going to get this merged. If they're not working on it someone should probably start a forked PR.

@saschagrunert
Copy link
Member

@resouer there are ongoing discussions about deprecating dockershim in Kubernetes. Are you around to drive this KEP forward or would you prefer if someone else takes over? 😇

@dims
Copy link
Member

dims commented Sep 9, 2020

@saschagrunert after talking to @liggitt and @derekwaynecarr this plan is kinda unworkable. Please see prototype in https://github.com/dims/cri-dockerd

Problem is that this external CRI impl will need to vendor in k8s.io/kubernetes which is verboten! ( https://github.com/dims/cri-dockerd/blob/main/go.mod#L21 )

@saschagrunert
Copy link
Member

@dims I'm fine with that as well as the planned deprecation. I guess we can still recycle some parts like the hostport manager.

@dims
Copy link
Member

dims commented Sep 9, 2020

@saschagrunert for sure! if you see containerd/cri repo, it no longer depends on k/k :) we should do the same for CRI-O

@saschagrunert
Copy link
Member

@saschagrunert for sure! if you see containerd/cri repo, it no longer depends on k/k :) we should do the same for CRI-O

Yes, thanks for supporting us on that front! ❤️

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 8, 2020

Step 1: Stabilize in-tree dockershim and decouple dockershim from kubelet (but still in-tree).

Target releases: 1.15, 1,16, 1.17

Choose a reason for hiding this comment

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

target release is v1.20 now. 😄

@SergeyKanzhelev
Copy link
Member

I think this is outdated after #2221 and #1985

/close

@k8s-ci-robot
Copy link
Contributor

@SergeyKanzhelev: Closed this PR.

In response to this:

I think this is outdated after #2221 and #1985

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-organization Issues or PRs related to kubernetes code organization cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet