Add a docker-shim package #29553

yujuhong · 2016-07-25T16:53:47Z

Add a new docker integration with kubelet using the new runtime API.
This change adds the package with the skeleton and implements some of the basic operations.

This PR only implements a small sets of functions. The rest of the functions will be implemented
in the followup PRs to keep the changes readable, and the reviewers sane.

Note: The first commit is from #28396, only the second commit is for review.

/cc @kubernetes/sig-node @feiskyer @Random-Liu

yujuhong · 2016-07-25T21:26:11Z

@k8s-bot test this issue: #29451

feiskyer · 2016-07-26T01:59:55Z

pkg/kubelet/dockershim/docker_container.go

+	for k, v := range config.GetAnnotations() {
+		if _, ok := labels[k]; !ok {
+			// Only write to labels if the key doesn't exist.
+			labels[k] = v


In this way, it is impossible to get annotations back. Maybe save all annotations inside one label, e.g. io.kubernetes.container.annotations?

Yes, I have a TODO for that.

Use a prefix to distinguish labels and annotations. Or else when get container status how do we know which is label, which is annotation? :)

Added a TODO for that.

errordeveloper · 2016-07-27T09:21:37Z

This looks interesting, but it's unclear what's the motivation for this exactly, is there a meta issue in features repo or anything like that?

yujuhong · 2016-07-27T21:41:49Z

Addressed comments and fixed the boilerplate issue. PTAL. Thanks!

yujuhong · 2016-07-27T21:43:13Z

This looks interesting, but it's unclear what's the motivation for this exactly, is there a meta issue in features repo or anything like that?

@errordeveloper, we are migrating all the runtime integration to the new interface. kubernetes/enhancements#54

Random-Liu · 2016-07-27T22:31:51Z

pkg/kubelet/dockershim/docker_container.go

+		}
+
+		if filter.LabelSelector != nil {
+			for k, v := range filter.LabelSelector {


We should add ("label", "k=v") here.

You're right. I saw the documentation but forgot to correct it. Done.

I need to find a good way to test this code.

Random-Liu · 2016-07-28T00:05:03Z

pkg/kubelet/dockershim/helpers.go

+	return
+}
+
+func generateMountBindings(mounts []*runtimeApi.Mount) (result []string) {


Port original comment here.

Random-Liu · 2016-07-28T00:06:35Z

LGTM with nits.

Random-Liu · 2016-07-28T01:26:35Z

pkg/kubelet/container/kuberuntime.go

-// ImageManagerService interface defines the interfaces that should be implemented
-// by a container image manager.
-// Thread safety is required from implementations of this interface.
+// ImageManagerService interface should be implemented a container image


s/implemented/implemented by

Add a new docker integration with kubelet using the new runtime API. This change adds the package with some skeletons, and implements some of the basic operations.

Random-Liu · 2016-07-28T02:41:08Z

LGTM. @dchen1107 Do you want to take another look?

feiskyer · 2016-07-28T06:54:13Z

LGTM. We can make kuberuntime.go in together with this PR for unblocking docker-shim and gRPC implementation #29623.

errordeveloper · 2016-07-28T17:27:18Z

@errordeveloper, we are migrating all the runtime integration to the new interface.

@yujuhong thank you for the pointer to feature issue. I am also really quite curious about the benefits of docker-shim... I know in Docker it provides the way to track children of dockerd, but it must help kubelet as well, right?

yujuhong · 2016-07-28T21:56:51Z

Applied lgtm based on the reviews to unblock other PRs.
The code is not currently in use, so @dchen1107, let me know if you want any changes. I'll upload a separate PR to address them.

yujuhong · 2016-07-28T21:58:59Z

@yujuhong thank you for the pointer to feature issue. I am also really quite curious about the benefits of docker-shim... I know in Docker it provides the way to track children of dockerd, but it must help kubelet as well, right?

@errordeveloper I think there is some misunderstanding -- the shim is to make docker client library conform to the new interface kubelet uses. The docker shim in this PR still calls docker daemon to perform operations.

dchen1107 · 2016-07-29T00:27:16Z

LGTM

yujuhong · 2016-07-29T00:29:47Z

LGTM

Thanks for the rubberstamp :)

k8s-bot · 2016-07-29T00:40:28Z

GCE e2e build/test passed for commit 03d11bc.

k8s-github-robot · 2016-07-29T00:58:55Z

Automatic merge from submit-queue

errordeveloper · 2016-07-29T10:07:32Z

The docker shim in this PR still calls docker daemon to perform operations.

@yujuhong ah, so I have just checked and what I was thinking of is containerd-shim. Sorry for the confusion.

yujuhong · 2016-07-29T16:54:39Z

@yujuhong ah, so I have just checked and what I was thinking of is containerd-shim. Sorry for the confusion.

No problem. There is on-going runc integration which doesn't use containerd at all. You can check out the proposal in #26788

Automatic merge from submit-queue dockershim: Implement more functions. Based on #29553. Only the last two commits are new.

yujuhong added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jul 25, 2016

googlebot added the cla: yes label Jul 25, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Jul 25, 2016

yujuhong added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 25, 2016

k8s-github-robot assigned dchen1107 Jul 25, 2016

yujuhong assigned Random-Liu Jul 25, 2016

yujuhong mentioned this pull request Jul 25, 2016

dockershim: Implement more functions. #29580

Merged

yujuhong force-pushed the docker-shim branch from 4b6248b to 19f00eb Compare July 25, 2016 23:43

feiskyer reviewed Jul 26, 2016
View reviewed changes

Add kuberuntime.go

0ac247c

yujuhong force-pushed the docker-shim branch from 19f00eb to 45fc0a1 Compare July 27, 2016 21:41

Random-Liu reviewed Jul 27, 2016
View reviewed changes

Random-Liu reviewed Jul 28, 2016
View reviewed changes

yujuhong force-pushed the docker-shim branch 2 times, most recently from ce2e190 to 66e0fb5 Compare July 28, 2016 01:03

Random-Liu reviewed Jul 28, 2016
View reviewed changes

Add a dockershim package

03d11bc

Add a new docker integration with kubelet using the new runtime API. This change adds the package with some skeletons, and implements some of the basic operations.

yujuhong force-pushed the docker-shim branch from 66e0fb5 to 03d11bc Compare July 28, 2016 01:30

yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2016

yujuhong added the retest-not-required label Jul 29, 2016

k8s-github-robot merged commit 7aa592b into kubernetes:master Jul 29, 2016

k8s-github-robot pushed a commit that referenced this pull request Jul 29, 2016

Merge pull request #29580 from yujuhong/dshim2

94b5d52

Automatic merge from submit-queue dockershim: Implement more functions. Based on #29553. Only the last two commits are new.

yujuhong mentioned this pull request Aug 25, 2016

Integrate docker with kubelet via CRI #31459

Closed

17 tasks

yujuhong deleted the docker-shim branch September 21, 2016 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a docker-shim package #29553

Add a docker-shim package #29553

yujuhong commented Jul 25, 2016

yujuhong commented Jul 25, 2016

feiskyer Jul 26, 2016

yujuhong Jul 26, 2016

Random-Liu Jul 27, 2016

yujuhong Jul 27, 2016

errordeveloper commented Jul 27, 2016 •

edited

Loading

yujuhong commented Jul 27, 2016

yujuhong commented Jul 27, 2016

Random-Liu Jul 27, 2016

yujuhong Jul 27, 2016

Random-Liu Jul 28, 2016

yujuhong Jul 28, 2016

Random-Liu commented Jul 28, 2016

Random-Liu Jul 28, 2016

Random-Liu commented Jul 28, 2016

feiskyer commented Jul 28, 2016

errordeveloper commented Jul 28, 2016

yujuhong commented Jul 28, 2016

yujuhong commented Jul 28, 2016

dchen1107 commented Jul 29, 2016

yujuhong commented Jul 29, 2016

k8s-bot commented Jul 29, 2016

k8s-github-robot commented Jul 29, 2016

errordeveloper commented Jul 29, 2016

yujuhong commented Jul 29, 2016

Add a docker-shim package #29553

Add a docker-shim package #29553

Conversation

yujuhong commented Jul 25, 2016

yujuhong commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errordeveloper commented Jul 27, 2016 • edited Loading

yujuhong commented Jul 27, 2016

yujuhong commented Jul 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Jul 28, 2016

Choose a reason for hiding this comment

Random-Liu commented Jul 28, 2016

feiskyer commented Jul 28, 2016

errordeveloper commented Jul 28, 2016

yujuhong commented Jul 28, 2016

yujuhong commented Jul 28, 2016

dchen1107 commented Jul 29, 2016

yujuhong commented Jul 29, 2016

k8s-bot commented Jul 29, 2016

k8s-github-robot commented Jul 29, 2016

errordeveloper commented Jul 29, 2016

yujuhong commented Jul 29, 2016

errordeveloper commented Jul 27, 2016 •

edited

Loading