kubeadm: update docker version for CE and EE #42425

dmmcquay · 2017-03-02T17:06:29Z

What this PR does / why we need it: Update regex for docker version to also capture new CE and EE versions.

Which issue this PR fixes: fixes #kubernetes/kubeadm#189

Special notes for your reviewer: /cc @jbeda @luxas

Release note:

NONE

k8s-reviewable · 2017-03-02T17:06:38Z

This change is

luxas · 2017-03-02T17:11:31Z

cc @Random-Liu @dchen1107 @vishh @yujuhong @kubernetes/sig-node-pr-reviews
Is docker 1.13.x validated and officially supported in v1.6?

yujuhong · 2017-03-02T17:32:37Z

Is docker 1.13.x validated and officially supported in v1.6?

Nope. It's not running in node e2e yet.

jbeda · 2017-03-02T18:19:17Z

Discussion right now if we should allow 1.9+ without qualification (as this PR as it stands) or if we should warn for 1.13+ as it hasn't been 100% validated yet.

luxas · 2017-03-02T18:53:33Z

Discussion right now if we should allow 1.9+ without qualification (as this PR as it stands) or if we should warn for 1.13+ as it hasn't been 100% validated yet.

I think we should absolutely not allow 1.9+ without warnings; that would be bad UX.

If docker 1.13 isn't validated for v1.6, it isn't

I think we should continue to fail for 1.13 and 17.03-ce until sig-node has validated it.

On the other hand, our docs could include that "if you know what you're doing and have run this iptables command described in issue xxx then you may move on with --skip-preflight-checks"

Random-Liu · 2017-03-03T22:01:30Z

test/e2e_node/system/types.go

@@ -114,7 +114,7 @@ var DefaultSysSpec = SysSpec{
 	Cgroups: []string{"cpu", "cpuacct", "cpuset", "devices", "freezer", "memory"},
 	RuntimeSpec: RuntimeSpec{
 		DockerSpec: &DockerSpec{
-			Version: []string{`1\.(9|\d{2,})\..*`}, // Requires 1.9+
+			Version: []string{`1\.(9|\d?[0-2]{2,})\..*`}, // Requires 1.9+


I'm not sure I understand the regexp here.

If we only want to officially support 1.9 - 1.12, could we change this to 1\.(9|1[0-2])\..*?

@dchen1107 Two questions here:

Do we still support 1.9?

Are we going to say we don't support 1.13+ now?

Testing code could still allow for testing newer versions. We might get some early signals from the community before upstream validates new versions. WDYT @Random-Liu ?

Testing code could still allow for testing newer versions. We might get some early signals from the community before upstream validates new versions. WDYT @Random-Liu ?

Good point, sounds good to me.

However, for kubeadm preflight check, should we fail on version other than 1.10-1.12?

dmmcquay · 2017-03-07T20:59:57Z

@k8s-bot cvm gce e2e test this
@k8s-bot kops aws e2e test this

dmmcquay · 2017-03-07T22:42:28Z

@k8s-bot cvm gce e2e test this

jbeda · 2017-03-08T00:10:54Z

This LGTM. The only thing I'd like to see is some pointer for the user to learn more. Based on our current docs I think that https://kubernetes.io/docs/getting-started-guides/kubeadm/#limitations is the best place to send people.

I'm thinking we can just do this as part of the mondo error returned by RunChecks in checks.go. If we don't have something here it is likely that we'll have a lot of support issues to deal with.

I'm ok with merging without that, but I'd feel better if we had it.

Random-Liu · 2017-03-08T00:37:08Z

test/e2e_node/system/docker_validator_test.go

@@ -28,7 +28,7 @@ func TestValidateDockerInfo(t *testing.T) {
 		Reporter: DefaultReporter,
 	}
 	spec := &DockerSpec{
-		Version:     []string{`1\.(9|\d{2,})\..*`},
+		Version:     []string{`1\.(9|\d?[0-2]{2,})\..*`}, // Requires 1.9+


The regex seems wrong to me. For example things like 1.21, 1.221 will also match.

If we only want to support 1.9-1.12, 1\.(9|1[0-2])\..* seems better to me

sig-node agreed to drop 1.9.x support for 1.6 release, can we update the test here reflecting such change too?

We should update this now.

dchen1107 · 2017-03-08T01:19:24Z

Can we change the docker version checking logic here:

If docker version < Kubelet's docker minimal support version (docker client: 1.10, docker server 1.22, for k8s 1.6), throwing an error;
If docker version in the range of being validated and supported by the node team (docker 1.10.x to docker 1.12.x for k8s 1.6), pass;
if docker version is greater than the maximum version validated by the node team (for example: docker 1.13, etc.), throwing a warning with the proper validation information
So that node-e2e tests can still run against latest docker release. Of course, docker changed their version schema, we are going to address it in 1.7 timeframe.

WDYT?

jbeda · 2017-03-08T01:22:57Z

I'm okay with @dchen1107's proposal. Currently the validators don't have a way to return warnings so that'll require some new plumbing.

@dmmcquay -- you up for it?

I still think we should direct users to a page for any precheck failure. At least we can address common things quickly there without pushing new code.

yujuhong · 2017-03-08T01:51:28Z

Of course, docker changed their version schema, we are going to address it in 1.7 timeframe.

We should check the docker API version (which is semver) instead.

dmmcquay · 2017-03-08T18:01:59Z

@dchen1107 @jbeda I don't know if I agree with the proposed change. Is the idea to rework the Validator interface so that Validate returns both a warning and an error? It seems like changing the other validators to compensate for just verifying docker would be strange.

What is the purpose of throwing the "warning"? Would logging the info satisfy this requirement?

jbeda · 2017-03-08T18:18:57Z

@yujuhong -- The API version isn't enough. There are other changes (such as how Docker sets up iptables) that can break people and won't show up as a breaking change to the API.

@dmmcquay -- my feeling is that <=1.9 we know it wont work. For 1.10-1.12 we have tested and validated. For >=1.13 it hasn't been validated and there may be issues (known and not known).

If we error for 1.13+ then if there are issues that people can work around they'll have to turn off all preflight checks. That means they may miss other problems. If we were able to specify which prechecks to skip then I'd be more comfortable making it an error over a warning.

This would mean changing the validator interface and that would be a bigger change. But the preflight check interface already knows how to surface warnings so it'll be limited to just the validator interface.

Thoughts? @dchen1107 @luxas @lukemarsden?

If we can't come to some other conclusion by EOD today, I think we should go with error for all non-proven versions with the addition of a link to the kubeadm docs.

dmmcquay · 2017-03-08T18:32:23Z

@jbeda One solution that I thought of would be making a new error type:

 type DockerValidatorError struct {  
     Msg     string                   
     Warning string                   
 }

And then do some sort of type switch on the error, and if its a DockerValidatorError, you can check to see if there warning string is empty or not.

Not saying this is a better solution, just an idea for keeping the interface intact.

jbeda · 2017-03-08T18:43:24Z

@dmmcquay I dunno. That just seems to move the problem around into the kubeadm code. At the end of the day, we should look to refactor this stuff out of a test directory so we shouldn't be afraid to move it forward (as long as @dchen1107 and @yujuhong are cool with it).

dmmcquay · 2017-03-08T19:30:52Z

@jbeda, @dchen1107, @yujuhong PTAL and let me know what you think about the changes I made. This change will allow 1.13+ (including EE and CE versions) but will return a warning saying docker version is greater than the maximum version validated by the node team.

dmmcquay · 2017-03-08T20:53:58Z

@k8s-bot non-cri e2e test this

jbeda

Just some small nits. This didn't end up being as invasive as we feared!

jbeda · 2017-03-08T21:13:43Z

test/e2e_node/system/docker_validator.go

+			fmt.Println("getting here")
+			d.Reporter.Report(dockerConfigPrefix+"VERSION", info.ServerVersion, good)
+			w := fmt.Errorf(
+				"docker version is greater than the maximum version validated by the node team. docker version: %s",


Users running this won't know who the "node team" is. I'd do something like:

"docker version is greater than the most recently validated version. Docker version: %s. Max validated version: %s"

Ideally we could point to a URL about what the known issues might be. @dchen1107 @yujuhong -- does the node team keep any documentation about the latest status of what has been validated?

jbeda · 2017-03-08T21:14:26Z

test/e2e_node/system/docker_validator_test.go

@@ -28,7 +28,7 @@ func TestValidateDockerInfo(t *testing.T) {
 		Reporter: DefaultReporter,
 	}
 	spec := &DockerSpec{
-		Version:     []string{`1\.(9|\d{2,})\..*`},
+		Version:     []string{`1\.(9|\d?[0-2]{2,})\..*`}, // Requires 1.9+


We should update this now.

luxas · 2017-03-08T21:26:32Z

test/e2e_node/system/docker_validator.go

+		ver := `1\.(1[3-9])\..*|\d{2}\.\d+\.\d+-[a-z]{2}`
+		r := regexp.MustCompile(ver)
+		if r.MatchString(info.ServerVersion) {
+			fmt.Println("getting here")


nit: remove

This change allows validators to pass warnings as well as errors. This was needed because of how support for docker 1.13+ and the new EE and CE versions is currently being handled.

jbeda · 2017-03-08T23:45:25Z

/lgtm

k8s-ci-robot · 2017-03-08T23:45:25Z

@jbeda: you can't LGTM a PR unless you are an assignee.

In response to this comment:

/lgtm

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.

k8s-github-robot · 2017-03-08T23:45:34Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: dmmcquay, jbeda

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

~~cmd/kubeadm/OWNERS~~ [jbeda]
~~test/e2e_node/OWNERS~~ [jbeda]

We suggest the following people:
cc @vishh
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

dmmcquay · 2017-03-08T23:49:40Z

@k8s-bot non-cri e2e test this

luxas · 2017-03-09T06:28:12Z

Applying @jbeda's LGTM...

k8s-github-robot · 2017-03-09T10:51:34Z

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

k8s-github-robot · 2017-03-09T10:51:39Z

Automatic merge from submit-queue (batch tested with PRs 42762, 42739, 42425, 42778)

k8s-ci-robot · 2017-03-09T12:04:55Z

@dmmcquay: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GKE smoke e2e	`d662c80`	link	`@k8s-bot cvm gke e2e test this`
Jenkins GCI GKE smoke e2e	`d662c80`	link	`@k8s-bot gci gke e2e test this`
Jenkins unit/integration	`35f0709`	link	`@k8s-bot unit test this`
Jenkins kops AWS e2e	`35f0709`	link	`@k8s-bot kops aws e2e test this`

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.

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

k8s-github-robot assigned timstclair Mar 2, 2017

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Mar 2, 2017

dmmcquay force-pushed the kubeadm_189_docker_version branch from 5f4e8e6 to d662c80 Compare March 3, 2017 18:51

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2017

Random-Liu reviewed Mar 3, 2017

View reviewed changes

kubeadm: fail on docker version 1.13+, CE, and EE

eeefd2c

dmmcquay force-pushed the kubeadm_189_docker_version branch from d662c80 to eeefd2c Compare March 7, 2017 18:20

Random-Liu reviewed Mar 8, 2017

View reviewed changes

jbeda mentioned this pull request Mar 8, 2017

Dropped docker 1.9.x support. Changed the minimumDockerAPIVersion to #42694

Merged

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2017

dmmcquay force-pushed the kubeadm_189_docker_version branch from b135a60 to a667029 Compare March 8, 2017 19:37

jbeda suggested changes Mar 8, 2017

View reviewed changes

luxas reviewed Mar 8, 2017

View reviewed changes

kubeadm: validators pass warnings and errors

35f0709

This change allows validators to pass warnings as well as errors. This was needed because of how support for docker 1.13+ and the new EE and CE versions is currently being handled.

dmmcquay force-pushed the kubeadm_189_docker_version branch from a667029 to 35f0709 Compare March 8, 2017 22:35

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2017

jbeda approved these changes Mar 8, 2017

View reviewed changes

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

timstclair assigned jbeda and unassigned timstclair Mar 9, 2017

luxas added this to the v1.6 milestone Mar 9, 2017

luxas added kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 9, 2017

k8s-github-robot merged commit eefa2ef into kubernetes:master Mar 9, 2017

dmmcquay deleted the kubeadm_189_docker_version branch March 9, 2017 16:30

dmmcquay mentioned this pull request Mar 9, 2017

Cluster creation failed with unsupported docker version kubernetes/kubeadm#189

Closed

JackyChen255 mentioned this pull request Mar 20, 2017

Kubernetes installation conflicts with docker-ce-17.03.0 #43400

Closed

kubeadm: update docker version for CE and EE #42425

kubeadm: update docker version for CE and EE #42425

Conversation

dmmcquay commented Mar 2, 2017

k8s-reviewable commented Mar 2, 2017

luxas commented Mar 2, 2017

yujuhong commented Mar 2, 2017

jbeda commented Mar 2, 2017

luxas commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmmcquay commented Mar 7, 2017

dmmcquay commented Mar 7, 2017

jbeda commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchen1107 commented Mar 8, 2017

jbeda commented Mar 8, 2017

yujuhong commented Mar 8, 2017

dmmcquay commented Mar 8, 2017

jbeda commented Mar 8, 2017

dmmcquay commented Mar 8, 2017

jbeda commented Mar 8, 2017

dmmcquay commented Mar 8, 2017 • edited

dmmcquay commented Mar 8, 2017

jbeda 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

jbeda commented Mar 8, 2017

k8s-ci-robot commented Mar 8, 2017

k8s-github-robot commented Mar 8, 2017

dmmcquay commented Mar 8, 2017

luxas commented Mar 9, 2017

k8s-github-robot commented Mar 9, 2017

k8s-github-robot commented Mar 9, 2017

k8s-ci-robot commented Mar 9, 2017

dmmcquay commented Mar 8, 2017 •

edited