New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recognize newer docker versions without -ce/-ee suffix: 18.09.0 #71001

Merged
merged 1 commit into from Nov 13, 2018

Conversation

@thomas-riccardi
Copy link
Contributor

thomas-riccardi commented Nov 13, 2018

What type of PR is this?
/kind feature
Is it really a feature?

users will notice and come to rely on

Maybe?

What this PR does / why we need it:
Since docker 18.09, the ServerVersion field format changed: the -ce
or -ee suffix disappeared:

  • docker 18.06: 18.06.1-ce
  • docker 18.09: 18.09.0

This was not expected by the docker_validator version regexp, which
assumed newer docker versions ended with -[a-z]{2}.
This made the validator return an error, whereas we expect it to
return only a warning (by recognizing it as a newer but not yet
supported docker version).

This commit relax the version regexp to also recognize 18.09.0.
The docker validator now returns a warning, as tested.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
I did not open or find a related https://github.com/kubernetes/kubernetes issue. Should I open an issue for that?

Initially encountered in minikube: kubernetes/minikube#3323

Special notes for your reviewer:
I am not sure the proposed fix is enough, as the new regexp does not match the whole version: it accepts matches on a substring of the docker version.
This means that it's effectively equivalent to just never check for the -ce/-ee suffixes.

Should I

  1. do nothing more?
  2. simplify the regexp? simply (\d{2}\.\d+\.\d+)
  3. or match the whole version string with ^$? (^\d{2}\.\d+\.\d+(?:-[a-z]{2})?$)

3. would be more strict, but maybe too strict? Maybe some kubernetes deployment rely on the current lax regexp (for example with docker forks identified by an addition version suffix?)?

Does this PR introduce a user-facing change?:

Not sure about the need for a release note or its phrasing.

Recognize newer docker versions without -ce/-ee suffix: 18.09.0
Recognize newer docker versions without -ce/-ee suffix
Since docker 18.09, the ServerVersion field format changed: the `-ce`
or `-ee` suffix disappeared:

- docker 18.06: `18.06.1-ce`
- docker 18.09: `18.09.0`

This was not expected by the docker_validator version regexp, which
assumed newer docker versions ended with `-[a-z]{2}`.
This made the validator return an error, whereas we expect it to
return only a warning (by recognizing it as a newer but not yet
supported docker version).

This commit relax the version regexp to also recognize `18.09.0`.
The docker validator now returns a warning, as tested.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 13, 2018

Hi @thomas-riccardi. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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

This comment has been minimized.

Copy link
Member

dims commented Nov 13, 2018

/ok-to-test

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 13, 2018

/priority important-soon
/assign
/assign @timothysc

@neolit123
Copy link
Member

neolit123 left a comment

@thomas-riccardi thank you.
i verified the fix and this should be fine.

/lgtm

@@ -73,7 +73,7 @@ func (d *DockerValidator) validateDockerInfo(spec *DockerSpec, info types.Info)
if !matched {
// If it's of the new Docker version scheme but didn't match above, it
// must be a newer version than the most recently validated one.
ver := `\d{2}\.\d+\.\d+-[a-z]{2}`
ver := `\d{2}\.\d+\.\d+(?:-[a-z]{2})?`

This comment has been minimized.

@neolit123

neolit123 Nov 13, 2018

Member

these checks are not super strict.

i think having this is better than the simplified version \d{2}\.\d+\.\d+
also, we haven't gotten any reports about the previous regexp not being optimal before.
so this change SGTM.

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 13, 2018

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 13, 2018

/remove-kind feature
/kind bug

let's call this is a bug.

@k8s-ci-robot k8s-ci-robot added kind/bug and removed kind/feature labels Nov 13, 2018

@timothysc
Copy link
Member

timothysc left a comment

/approve

@timothysc timothysc added this to the v1.13 milestone Nov 13, 2018

@timothysc timothysc added the approved label Nov 13, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thomas-riccardi, timothysc

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot merged commit 48d788d into kubernetes:master Nov 13, 2018

17 checks passed

cla/linuxfoundation thomas-riccardi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@thomas-riccardi thomas-riccardi deleted the Deepomatic:recognize_docker_1809_version branch Nov 14, 2018

@thomas-riccardi

This comment has been minimized.

Copy link
Contributor

thomas-riccardi commented Nov 14, 2018

Thanks all for the quick responses!

Should this PR be cherry-picked on release branches? Which ones? Who should do that?

I only found https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md which only explains how to do it. Is there more documentation about backports somewhere?

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 14, 2018

@thomas-riccardi
hi, we can only backport critical bug fixes and this probably won't be approved by the team.
also, this validation should be currently skippable.

if you have a strong opinion about a cherry pick for 1.12 you can create a proposal in a kubernetes/kubeadm issue.

@thomas-riccardi

This comment has been minimized.

Copy link
Contributor

thomas-riccardi commented Nov 14, 2018

@neolit123
OK, it makes sense.
What about kubernetes 1.13? The release-1.13 branch was created a week ago, and I'm not sure about pre-released branches workflow.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 14, 2018

@thomas-riccardi
currently the branch is synced with master, until the end of code freeze 28th AFAIK.

@thomas-riccardi

This comment has been minimized.

Copy link
Contributor

thomas-riccardi commented Nov 15, 2018

@neolit123
OK, I did not see that... (I just discovered that github commit view only list one branch associated with the commit, not all branches containing the commit.)

Thanks for everything!

@luuduchieu

This comment has been minimized.

Copy link

luuduchieu commented Dec 27, 2018

This one is fixed on --kubernetes-version="v1.13.0", please enjoy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment