Kubelet: Start using the official docker engine-api #23506

Random-Liu · 2016-03-25T22:29:43Z

This is the first step in the roadmap of switching to docker engine-api.
In this PR, I keep the old DockerInterface and implement it with the new engine-api.
With this approach, we could switch to engine-api with minimum change, so that we could:

Test the engine-api without huge refactoring.
Send following PRs to refactor functions in DockerInterface separately so as to avoid a huge change in one PR.

I've tested this PR locally, it passed all the node conformance test:

make test_e2e_node

Ran 19 of 19 Specs in 823.395 seconds
SUCCESS! -- 19 Passed | 0 Failed | 0 Pending | 0 Skipped PASS

Ginkgo ran 1 suite in 13m49.429979585s
Test Suite Passed

And it also passed the jenkins gce e2e test:

go run hack/e2e.go -test -v --test_args="--ginkgo.skip=\[Slow\]|\[Serial\]|\[Disruptive\]|\[Flaky\]|\[Feature:.+\]"

Ran 161 of 268 Specs in 4570.214 seconds
SUCCESS! -- 161 Passed | 0 Failed | 0 Pending | 107 Skipped PASS

Ginkgo ran 1 suite in 1h16m16.325934558s
Test Suite Passed
2016/03/25 15:12:42 e2e.go:196: Step 'Ginkgo tests' finished in 1h16m18.918754301s

I'm writing the design document, and will post the switching roadmap in an umbrella issue soon.

@kubernetes/sig-node

k8s-github-robot · 2016-03-25T22:36:07Z

Labelling this PR as size/XS

Random-Liu · 2016-03-25T22:37:46Z

pkg/kubelet/dockertools/kube_docker_client.go

+}
+
+// redirectResponseToOutputStream redirect the response stream to stdout and stderr. When tty is true, all stream will
+// only be redirect to stdout.


s/redirect/redirected

k8s-bot · 2016-03-25T23:06:51Z

GCE e2e build/test passed for commit bd3df7a975ee617fff05156afcdabf37fbfe4b9d.

k8s-bot · 2016-03-25T23:42:26Z

GCE e2e build/test passed for commit c8fa0ab13d8f15fc843dcc0f80a02aa86b05f7c7.

vishh · 2016-03-26T00:12:17Z

pkg/kubelet/dockertools/kube_docker_client.go

+	return
+}
+
+func (d *KubeDockerClient) StartContainer(id string, _ *docker.HostConfig) (err error) {


nit: Why have an unused arg? Is it to avoid updating existing code?

@vishh Yeah, the HostConfig in StartContainer is deprecated #20454, but the DockerInterface still has it. I'll remove this in the following refactoring PRs. :)

vishh · 2016-03-26T00:22:58Z

Nice work @Random-Liu!!!

Random-Liu · 2016-03-26T01:18:57Z

@vishh Thanks for your review!!!

yujuhong · 2016-03-28T17:06:49Z

pkg/kubelet/dockertools/kube_docker_client.go

+//	3) Stabilize the DockerInterface. The engine-api is still under active development, the interface
+//	is not stabilized yet. However, the DockerInterface is used in many files in Kubernetes, we may
+//	not want to change the interface frequently. With this layer, we can port the engine api to the
+//	DockerInterface to avoid changing DockerInterface as far as possible.


s/as far as/as much as

BTW, we want a better interface. I assume this is just the first step to swap the client, and we'll get to fixing the DockerInterface soon? :-)

If that's the case, suggest adding TODOs

@yujuhong Yeah, this is just the first step to make the following refactoring easier, I'll add a TODO here.

ncdc · 2016-03-28T17:28:39Z

cc @kubernetes/rh-cluster-infra

dchen1107 · 2016-03-28T17:37:04Z

pkg/kubelet/dockertools/docker.go

 	if len(dockerEndpoint) > 0 {
 		glog.Infof("Connecting to docker on %s", dockerEndpoint)
-		return docker.NewClient(dockerEndpoint)
+		return dockerapi.NewClient(dockerEndpoint, "", nil, nil)


Do we need to detect the API version here and pass the version string for initialization?

From engine-api doc, looks like if the version number is empty string, the docker client assume you are talking to the latest API. In this case, do we have issues to talk to docker daemon 1.8.3 release?

@dchen1107 We didn't do that before https://github.com/fsouza/go-dockerclient/blob/master/client.go#L148, but I'm thinking whether we should do that now.

dchen1107 · 2016-03-28T17:55:45Z

LGTM overall except one question related to compatibility issue. We need to validate that.

yujuhong · 2016-03-28T17:57:15Z

I suggest keeping the instrumentedDockerInterface to record errors, so that it's easier to handle errors in each function (and not having to care about nested if-else blocks, etc). Basically, the code may be more readable and less error-prone.

Random-Liu · 2016-03-28T18:06:28Z

@yujuhong SGTM, will do that soon.

yujuhong · 2016-03-28T18:12:31Z

This is the first step in the roadmap of switching to docker engine-api.
In this PR, I keep the old DockerInterface and implement it with the new engine-api.
With this approach, we could switch to engine-api with minimum change, so that we could:

Just a suggestion/alternative:
You could also start defining the new docker interface for the engine-api client, and implement a client to convert between the old and the new interfaces. That way, you can

Keep the new interface clean, implement functions and write unit tests without worrying about the old interface.
Write conversion functions (old to the new interface) in a separate type when you're ready to start using the engine-api client
(Gradually) convert the old interface to the new interface

I see pros and cons in both, so I'll leave the decision to you :-)

yujuhong · 2016-03-28T18:43:00Z

Just a suggestion/alternative:
You could also start defining the new docker interface for the engine-api client, and implement a client to convert between the old and the new interfaces. That way, you can

Keep the new interface clean, implement functions and write unit tests without worrying about the old interface.
Write conversion functions (old to the new interface) in a separate type when you're ready to start using the engine-api client
(Gradually) convert the old interface to the new interface
I see pros and cons in both, so I'll leave the decision to you :-)
Hide all checks

Discussed with @Random-Liu offline. We've decided to move on with this PR first, and see if we need to make changes to the interface along the way.

Random-Liu · 2016-03-28T21:15:27Z

@dchen1107 I discussed with @caesarxuchao about the apiversion offline.

The current version of the API is v1.23 which means calling /info is the same as
calling /v1.23/info. To call an older version of the API use /v1.22/info.

The statement is ambiguous. In fact, it means that for different docker version, there is a default apiversion (See here). If the client doesn't specify the apiversion, docker daemon will assume it is using the default apiversion. (See here)

So as long as kubelet doesn't rely on functionalities which are not consistent in 1.8.x, 1.9.x and 1.10.x, there shouldn't be problem. :)

k8s-bot · 2016-03-30T23:52:22Z

GCE e2e build/test passed for commit a6b4378.

k8s-github-robot · 2016-04-01T23:58:50Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-04-02T00:33:40Z

GCE e2e build/test failed for commit a6b4378.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

Random-Liu · 2016-04-02T00:54:27Z

@k8s-bot test this issue #22719

k8s-bot · 2016-04-02T01:32:36Z

GCE e2e build/test passed for commit a6b4378.

k8s-github-robot · 2016-04-02T10:17:24Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-02T10:52:01Z

GCE e2e build/test passed for commit a6b4378.

k8s-github-robot · 2016-04-02T11:15:51Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-02T11:46:51Z

GCE e2e build/test passed for commit a6b4378.

k8s-github-robot · 2016-04-02T11:51:33Z

Automatic merge from submit-queue

andyzheng0831 · 2016-04-02T16:57:47Z

@Random-Liu @dchen1107 @roberthbailey @fabioy @wonderfly our Jenkins auto test on trusty caught severe breakage caused by this PR. The symptom is: docker daemon is up but kubelet cannot talk with it. I see many occurrences of "Error response from daemon: 400 Bad Request: malformed Host header" error in kubelet.log.

We are using docker 1.9.1 (docker API version 1.21) with k8s binary built from master branch HEAD. Is there anything needs to change to adapt to this PR, e.g., kubelet flag or manifest change?

eparis · 2016-04-02T17:27:44Z

I know there was a simliar sounding issue when docker daemon was built with golang 1.6 and docker client was <1.10 and build with <1.6.... Let me find that bug. Do we have golang 1.6 in play somewhere?

eparis · 2016-04-02T17:28:20Z

moby/moby#20865

Random-Liu · 2016-04-02T17:42:54Z

@andyzheng0831 Really? Our e2e cluster is just using docker 1.9.1. And we are using the default docker api version (see here)

As @eparis mentioned, what is your golang version?

eparis · 2016-04-02T17:44:31Z

what version of golang was used to build your 1.9.1. That could explain this issue.

andyzheng0831 · 2016-04-02T17:48:03Z

Our docker was built by golang 1.6

eparis · 2016-04-02T17:51:20Z

bing, there's our problem. moby/moby#20865

Seems like it would be better to just upgrade our client library to whatever version of client library is used by 1.10, which won't set and invalid Host header...

eparis · 2016-04-02T17:51:38Z

(or temporary workaround, rebuild docker < 1.10 with golang < 1.6)

Random-Liu · 2016-04-02T18:00:07Z

@andyzheng0831 @eparis Thank you very much! It's really important to know this.

I'll look into this issue more later today and see how we could solve this.

BTW, Kubernetes doesn't support golang 1.6 now, but is trying to do that. (See #22149)

andyzheng0831 · 2016-04-02T18:15:22Z

@Random-Liu thanks for look into it! Please loop me in when you have new discovery.

@yujuhong

Automatic merge from submit-queue Kubelet: Remove nsinit related code and bump up minimum docker apiversion Docker has native exec support after 1.3.x. We never need this code now. As for the apiversion, because Kubernetes supports 1.8.x - 1.10.x now, we should bump up the minimum docker apiversion. @yujuhong I checked the [changes](https://github.com/docker/engine-api/blob/master/types/versions/v1p20/types.go), we are not relying on any of those changes. So #23506 should work with docker 1.8.x+

Automatic merge from submit-queue Kubelet: Refactor container related functions in DockerInterface For #23563. Based on #23506, will rebase after #23506 is merged. The last 4 commits of this PR are new. This PR refactors all container lifecycle related functions in DockerInterface, including: * ListContainers * InspectContainer * CreateContainer * StartContainer * StopContainer * RemoveContainer @kubernetes/sig-node

@yujuhong

Automatic merge from submit-queue Fix PullImage and add corresponding node e2e test Fixes #24101. This is a bug introduced by #23506, since ref #23563. The root cause of #24101 is described [here](#24101 (comment)). This PR 1) Fixes #24101 by decoding the messages returned during pulling image, and return error if any of the messages contains error. 2) Add the node e2e test to detect this kind of failure. 3) Get present check out of `ConformanceImage.Remove()` and `ConformanceImage.Pull()`. Because sometimes we may expect error to occur in `PullImage()` and `RemoveImage()`, but even that doesn't happen, the `Present()` check will still return error and let the test pass. @yujuhong @freehan @liangchenye Also /cc @resouer, because he is doing the image related functions refactoring.

Random-Liu added kind/enhancement area/docker area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Mar 25, 2016

yujuhong self-assigned this Mar 25, 2016

k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 25, 2016

Random-Liu reviewed Mar 25, 2016
View reviewed changes

Random-Liu force-pushed the new-docker-client branch from bd3df7a to c8fa0ab Compare March 25, 2016 23:11

googlebot added the cla: yes label Mar 25, 2016

vishh reviewed Mar 26, 2016
View reviewed changes

yujuhong reviewed Mar 28, 2016
View reviewed changes

dchen1107 reviewed Mar 28, 2016
View reviewed changes

Random-Liu mentioned this pull request Mar 31, 2016

Kubelet: Refactor container related functions in DockerInterface #23699

Merged

yujuhong mentioned this pull request Mar 31, 2016

Collect and expose runtime's image storage usage via Kubelet's /stats/summary endpoint #23595

Merged

eparis added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 31, 2016

k8s-github-robot merged commit b43ccd6 into kubernetes:master Apr 2, 2016

Random-Liu deleted the new-docker-client branch April 3, 2016 00:20

Random-Liu mentioned this pull request Apr 4, 2016

Kubelet: Kubelet built with go(<= 1.4) won't work with docker built with go(>=1.6) #23809

Closed

Random-Liu mentioned this pull request Apr 12, 2016

Fix PullImage and add corresponding node e2e test #24126

Merged

Kubelet: Start using the official docker engine-api #23506

Kubelet: Start using the official docker engine-api #23506

Conversation

Random-Liu commented Mar 25, 2016

k8s-github-robot commented Mar 25, 2016

Choose a reason for hiding this comment

k8s-bot commented Mar 25, 2016

k8s-bot commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Mar 26, 2016

Random-Liu commented Mar 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Mar 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Mar 28, 2016

yujuhong commented Mar 28, 2016

Random-Liu commented Mar 28, 2016

yujuhong commented Mar 28, 2016

yujuhong commented Mar 28, 2016

Random-Liu commented Mar 28, 2016

k8s-bot commented Mar 30, 2016

k8s-github-robot commented Apr 1, 2016

k8s-bot commented Apr 2, 2016

Random-Liu commented Apr 2, 2016

k8s-bot commented Apr 2, 2016

k8s-github-robot commented Apr 2, 2016

k8s-bot commented Apr 2, 2016

k8s-github-robot commented Apr 2, 2016

k8s-bot commented Apr 2, 2016

k8s-github-robot commented Apr 2, 2016

andyzheng0831 commented Apr 2, 2016

eparis commented Apr 2, 2016

eparis commented Apr 2, 2016

Random-Liu commented Apr 2, 2016

eparis commented Apr 2, 2016

andyzheng0831 commented Apr 2, 2016

eparis commented Apr 2, 2016

eparis commented Apr 2, 2016

Random-Liu commented Apr 2, 2016

andyzheng0831 commented Apr 2, 2016