Use Docker API Version instead of docker version #44068

mkumatag · 2017-04-05T01:13:54Z

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Fixes #42492
Special notes for your reviewer:

Release note:

Update cadvisor to latest head to use docker APIversion exposed by cadvisor

k8s-reviewable · 2017-04-05T01:14:00Z

This change is

k8s-ci-robot · 2017-04-05T01:14:01Z

Hi @mkumatag. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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.

mkumatag · 2017-04-05T01:15:36Z

/cc @mikebrow @yujuhong @derekwaynecarr

mkumatag · 2017-04-05T01:42:56Z

testlog @ https://gist.github.com/mkumatag/6eb2730fbe9f1c142b4426067689e4d7

Tested -

No errors in kubelet
Deployed nginx pod, running fine without any errors.

mikebrow

Looks good to me, just a few Minor nits.

mikebrow · 2017-04-05T16:21:51Z

pkg/kubelet/dockertools/docker_manager.go

@@ -1095,7 +1095,7 @@ func (dm *DockerManager) pluginDisablesDockerNetworking() bool {

 // newDockerVersion returns a semantically versioned docker version value


Should read "newDockerVersion returns a generic version value (not semantic)"

Alternatively, might want to add a newDockerAPIVersion() API.

I don't see anyone is using this function at all! except docker_manager_test.go. If no objection from anyone, I would like to remove this function completely.

It's used just below on line number 1128 other than that yeah it doesn't seem to be used.

you are right! it is used in Version function, I'll leave this function as is and lemme modify the description. We will add newDockerAPIVersion when somebody really need it!

mikebrow · 2017-04-05T16:33:51Z

vendor/github.com/google/cadvisor/fs/fs.go

+			buf := new(syscall.Stat_t)
+			err := syscall.Stat(mount.Source, buf)
+			if err != nil {
+				glog.Warningf("stat failed on %s with error: %s", mount.Source, err)


on mount source %s

As this is a godep stuff, we can submit a PR in cadvisor to get this message fixed.!

ah vendor.. :-)

mikebrow · 2017-04-05T16:34:10Z

vendor/github.com/google/cadvisor/fs/fs.go

+				if buf.Mode&syscall.S_IFMT == syscall.S_IFBLK {
+					err := syscall.Stat(mount.Mountpoint, buf)
+					if err != nil {
+						glog.Warningf("stat failed on %s with error: %s", mount.Mountpoint, err)


on mount point %s

As this is a godep stuff, we can submit a PR in cadvisor to get this message fixed.!

mikebrow

What do you think about updating the test cases in docker_manager_test.go to cover the new date based versions? Specifically adding one that is not semver compatible like 17.01.03

grodrigues3 · 2017-04-05T18:26:17Z

@k8s-bot ok to test

mkumatag · 2017-04-05T18:30:21Z

I suspect some tests failures in docker_manager_test.go and few more, will work on them and update this PR..

yujuhong · 2017-04-05T18:45:23Z

I suspect some tests failures in docker_manager_test.go and few more, will work on them and update this PR..

I'd suggest leave docker_manager and its unit tests alone. They are going away soon (see #43884).

mkumatag · 2017-04-05T18:57:58Z

I'd suggest leave docker_manager and its unit tests alone. They are going away soon (see #43884).

In that case lemme rollback the changes on docker_manager file

mkumatag · 2017-04-06T00:41:30Z

@yujuhong rollbacked the changes from docker_manager and all the checks passed.! can you PTAL at this PR?

yujuhong · 2017-04-06T00:54:59Z

pkg/kubelet/cadvisor/cadvisor_linux.go

-	if err != nil {
-		return nil, err
-	}
+	sysFs := sysfs.NewRealSysFs()


Why removing the error check?

that's because in recent version there is change in NewRealSysFS function, check this out for more information - google/cadvisor#1578

Gotta love empty structs :-)

Ah I see. I reviewed only the second commit, hence the question.

yujuhong · 2017-04-06T16:36:17Z

The second commit LGTM.

I think you'll need to add a release note since this bumps the cadvisor version. @dashpole, do you want to look at the cadvisor version bump?

mkumatag · 2017-04-11T16:34:35Z

ping @dashpole

dashpole · 2017-04-11T17:39:18Z

The first commit LGTM!

yujuhong · 2017-04-11T17:52:08Z

The first commit LGTM!

Thanks!

@mksalawa need to rebase and add a release note.

soltysh · 2017-04-27T13:37:00Z

@mkumatag any chance for the rebase?

mkumatag · 2017-04-27T14:12:02Z

@soltysh done. Can you please help me merging this PR?

soltysh · 2017-04-27T14:18:36Z

@mkumatag make sure the CI is green first, I'll take a look at this PR later today or tomorrow at latest and make sure to have it merge if it's ok

mkumatag · 2017-04-27T15:07:20Z

@mikedanese @ixdy I see some failures for bazel-test but hack/verify passes locally. Any idea what is wrong here? btw: I'm using go1.8.1 version

mkumatag · 2017-04-27T15:07:41Z

@k8s-bot bazel test this

dashpole · 2017-04-27T15:32:29Z

looks like the error is in a BUILD file. did you run ./hack/update-bazel?

mkumatag · 2017-04-27T15:56:29Z

Yes, I did.. the file got updated result of that script run.

looks like the error is in a BUILD file. did you run ./hack/update-bazel?

mikedanese · 2017-04-27T16:21:04Z

@mkumatag not sure what is going on but you need to revert your changes to vendor/BUILD. Looks like a weird rebase. Can you do this:

# make sure your master is current
$ git checkout k8s_add_apiversion
$ git checkout master -- vendor/BUILD
$ ./hack/update-bazel.sh

mikedanese · 2017-04-27T16:21:55Z

If that doesn't work, ping me and I'll push a fix to your branch.

mkumatag · 2017-04-27T17:50:16Z

Thanks @mikedanese it worked..

ixdy · 2017-04-27T18:50:52Z

looks like another case fixed by mikedanese/gazel#38. we should probably update gazel.

yujuhong · 2017-05-01T16:28:10Z

/lgtm

yujuhong · 2017-05-01T16:30:21Z

Ping a couple of folks for approval: @dchen1107 @thockin @smarterclayton

dchen1107 · 2017-05-03T21:44:21Z

/lgtm

and
/approve

k8s-github-robot · 2017-05-03T21:44:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, mkumatag, yujuhong

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [dchen1107]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mkumatag · 2017-05-04T01:23:45Z

@k8s-bot verify test this

k8s-github-robot · 2017-05-04T03:21:44Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 5, 2017

k8s-github-robot assigned timstclair Apr 5, 2017

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

mkumatag mentioned this pull request Apr 5, 2017

Use docker API version - 1.23 API is 1.11.0 #43417

Closed

mikebrow reviewed Apr 5, 2017

View reviewed changes

mikebrow suggested changes Apr 5, 2017

View reviewed changes

mkumatag force-pushed the k8s_add_apiversion branch from d768b70 to 63ace7b Compare April 5, 2017 19:09

yujuhong reviewed Apr 6, 2017

View reviewed changes

yujuhong added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 6, 2017

yujuhong self-assigned this Apr 6, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2017

yujuhong assigned dashpole and unassigned timstclair Apr 11, 2017

mkumatag force-pushed the k8s_add_apiversion branch from 63ace7b to 93556f8 Compare April 12, 2017 01:04

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2017

soltysh mentioned this pull request Apr 27, 2017

Change parsing docker version #45020

Closed

soltysh mentioned this pull request Apr 27, 2017

Latest docker version breaks kubelet in 1.5.0-alpha.3 openshift/origin#13281

Closed

mkumatag added 2 commits April 27, 2017 10:05

Update cadvisor version to latest version

2b42d71

Use Docker API Version instead of docker version

f806387

mkumatag force-pushed the k8s_add_apiversion branch from 22a0c14 to f806387 Compare April 27, 2017 14:07

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 27, 2017

Update bazel build

1df3bb2

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2017

k8s-github-robot merged commit 08a825c into kubernetes:master May 4, 2017

mkumatag deleted the k8s_add_apiversion branch May 4, 2017 03:31

dcbw mentioned this pull request May 24, 2017

Unable to parse docker version "1.26": illegal version string "1.26" openshift/origin#14279

Closed

		@@ -1095,7 +1095,7 @@ func (dm *DockerManager) pluginDisablesDockerNetworking() bool {

		// newDockerVersion returns a semantically versioned docker version value

Use Docker API Version instead of docker version #44068

Use Docker API Version instead of docker version #44068

Conversation

mkumatag commented Apr 5, 2017 • edited Loading

k8s-reviewable commented Apr 5, 2017

k8s-ci-robot commented Apr 5, 2017

mkumatag commented Apr 5, 2017

mkumatag commented Apr 5, 2017

mikebrow left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a comment • edited Loading

Choose a reason for hiding this comment

grodrigues3 commented Apr 5, 2017

mkumatag commented Apr 5, 2017

yujuhong commented Apr 5, 2017

mkumatag commented Apr 5, 2017

mkumatag commented Apr 6, 2017

Choose a reason for hiding this comment

mkumatag Apr 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Apr 6, 2017

mkumatag commented Apr 11, 2017

dashpole commented Apr 11, 2017

yujuhong commented Apr 11, 2017

soltysh commented Apr 27, 2017

mkumatag commented Apr 27, 2017

soltysh commented Apr 27, 2017

mkumatag commented Apr 27, 2017

mkumatag commented Apr 27, 2017

dashpole commented Apr 27, 2017

mkumatag commented Apr 27, 2017

mikedanese commented Apr 27, 2017

mikedanese commented Apr 27, 2017

mkumatag commented Apr 27, 2017

ixdy commented Apr 27, 2017

yujuhong commented May 1, 2017

yujuhong commented May 1, 2017

dchen1107 commented May 3, 2017

k8s-github-robot commented May 3, 2017

mkumatag commented May 4, 2017

k8s-github-robot commented May 4, 2017

mkumatag commented Apr 5, 2017 •

edited

Loading

mikebrow left a comment •

edited

Loading

mkumatag Apr 6, 2017 •

edited

Loading