[WIP] Initial-cut of cri-dockerd #74051

dims · 2019-02-14T03:01:11Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
We have a long standing TODO to break up kubelet's built-in docker shim into a separate binary. This brings docker in line with all the other CRI providers and levels the playing field for all CRI(s).

Which issue(s) this PR fixes:

Fixes #73933

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cri-dockerd is a separate binary for running the CRI for Docker that was previously built into kubelet. You can run this binary separately and then use "--container-runtime=remote" when you start kubelet.

k8s-ci-robot · 2019-03-07T12:03:00Z

@dims: Closed this PR.

In response to this:

/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.

dims · 2019-03-27T21:02:04Z

/test pull-kubernetes-integration

dims · 2019-03-27T21:59:00Z

/test pull-kubernetes-local-e2e-containerized

- Stripped down code from cmd/kubelet - Remove the functionality that moved to cri-dockerd binary - add import-restrictions to monitor changes to dependencies - added myself to OWNERS for cri-dockerd Change-Id: I0dbc4109266cf8d66dca08c68e490fa930628c2c

Change-Id: I1ebeae40c7cf541fe802954da5c93d4463c7b110

k8s-ci-robot · 2019-07-16T21:35:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dims
To complete the pull request process, please assign lavalamp
You can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dims · 2019-07-16T23:42:40Z

/retest

rosti · 2019-07-17T10:37:42Z

cmd/cri-dockerd/app/server.go

+		// so we do all our parsing manually in Run, below.
+		// DisableFlagParsing=true provides the full set of flags passed to the cri-dockerd in the
+		// `args` arg to Run, without Cobra's interference.
+		DisableFlagParsing: true,


Is this still required? Does cri-dockerd need KubeletConfiguration to operate or it needs only the docker shim related command line flags of the kubelet?

@rosti until we move cri-dockerd into its own repo, it seemed best to keep this stuff the same between "regular" kubelet and "cri-dockerd" in terms of command line flags and config parsing.

we need to figure out how to tell the users to install the new cri-dockerd binary.

@dims Indeed, I can see that this outside the scope of this PR, but we can still add a simplification TODO there.
The complex way of doing command line parsing in the kubelet is required, because of the ability to override config fields with command line flags. If we don't need the KubeletConfiguration in cri-dockerd, then we can greatly simplify things.

@neolit123 that can be done by adding a dependency in the kubelet package to the cri-dockerd package.

i think the kubelet with dockershim and cri-dockerd have to be supported in parallel for a while.

first cri-dockerd has to become a part of the node packages and the kubernetes release process and only once the first classed deployers like kops, kubeadm etc switch to it we can remove dockershim.

i have concerns making the hard switch within one release as it may bust existing setups.

rosti · 2019-07-17T10:45:14Z

build/debs/BUILD

+            "dir": "/usr/bin",
+        },
+        {
+            "files": ["cri-dockerd.service"],


I don't see this file in the PR. Perhaps it's missing or uncommitted?
Anyway, I presume it's going to be roughly equivalent to kubelet's systemd service file.

Hmm, i missed this, will add one

k8s-ci-robot · 2019-07-17T15:28:33Z

@dims: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	3a73103303a87bb7f2f4124e31aa375437e8a270	link	`/test pull-kubernetes-local-e2e-containerized`
pull-kubernetes-integration	`51be6b8`	link	`/test pull-kubernetes-integration`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

timothysc · 2019-07-23T21:28:10Z

/assign @rosti @neolit123
/unassign @timothysc

k8s-ci-robot requested review from dchen1107 and lavalamp February 14, 2019 03:02

dims force-pushed the separate-binary-for-running-docker-shim branch from 37e7831 to 2260087 Compare February 14, 2019 15:58

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch 3 times, most recently from ad9a566 to 187bb62 Compare February 14, 2019 17:57

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 14, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch from 187bb62 to a35ad5c Compare February 15, 2019 03:06

dims force-pushed the separate-binary-for-running-docker-shim branch from a35ad5c to 5302f0a Compare February 15, 2019 03:14

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch 2 times, most recently from 62d51c0 to c5d02aa Compare February 15, 2019 15:36

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Feb 15, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch from c5d02aa to 3a73103 Compare February 15, 2019 15:38

dims reopened this Mar 27, 2019

dims changed the title ~~Initial-cut of cri-dockerd~~ [WIP] Initial-cut of cri-dockerd Mar 27, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch from 3a73103 to 20d4620 Compare March 27, 2019 19:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch 2 times, most recently from e31bafc to b926ec1 Compare March 27, 2019 20:16

dims force-pushed the separate-binary-for-running-docker-shim branch from d22b23e to 902b5ac Compare March 28, 2019 00:30

k8s-ci-robot added the area/test label Mar 28, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch from 902b5ac to 0b69d25 Compare March 28, 2019 10:09

dims closed this Apr 3, 2019

dims mentioned this pull request Jul 16, 2019

Add ImageFSInfo, ContainerStats, and ListContainerStats impl for linux to dockershim #80105

Merged

dims added 2 commits July 16, 2019 17:33

Initial-cut of cri-dockerd

1b92c7b

- Stripped down code from cmd/kubelet - Remove the functionality that moved to cri-dockerd binary - add import-restrictions to monitor changes to dependencies - added myself to OWNERS for cri-dockerd Change-Id: I0dbc4109266cf8d66dca08c68e490fa930628c2c

Consolidate streaming code into same package

7ce1f74

Change-Id: I1ebeae40c7cf541fe802954da5c93d4463c7b110

dims reopened this Jul 16, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

dims force-pushed the separate-binary-for-running-docker-shim branch from 0b69d25 to 7ce1f74 Compare July 16, 2019 21:34

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

rosti reviewed Jul 17, 2019

View reviewed changes

Add systemd service file for cri-dockerd

51be6b8

k8s-ci-robot assigned rosti and unassigned timothysc Jul 23, 2019

dims closed this Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Initial-cut of cri-dockerd #74051

[WIP] Initial-cut of cri-dockerd #74051

dims commented Feb 14, 2019 •

edited

k8s-ci-robot commented Mar 7, 2019

dims commented Mar 27, 2019

dims commented Mar 27, 2019

k8s-ci-robot commented Jul 16, 2019

dims commented Jul 16, 2019

rosti Jul 17, 2019

dims Jul 17, 2019

neolit123 Jul 17, 2019

rosti Jul 22, 2019

neolit123 Jul 22, 2019

rosti Jul 17, 2019

dims Jul 17, 2019

k8s-ci-robot commented Jul 17, 2019

timothysc commented Jul 23, 2019

[WIP] Initial-cut of cri-dockerd #74051

[WIP] Initial-cut of cri-dockerd #74051

Conversation

dims commented Feb 14, 2019 • edited

k8s-ci-robot commented Mar 7, 2019

dims commented Mar 27, 2019

dims commented Mar 27, 2019

k8s-ci-robot commented Jul 16, 2019

dims commented Jul 16, 2019

rosti Jul 17, 2019

Choose a reason for hiding this comment

dims Jul 17, 2019

Choose a reason for hiding this comment

neolit123 Jul 17, 2019

Choose a reason for hiding this comment

rosti Jul 22, 2019

Choose a reason for hiding this comment

neolit123 Jul 22, 2019

Choose a reason for hiding this comment

rosti Jul 17, 2019

Choose a reason for hiding this comment

dims Jul 17, 2019

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 17, 2019

timothysc commented Jul 23, 2019

dims commented Feb 14, 2019 •

edited