Recognize newer docker versions without -ce/-ee suffix: 18.09.0 #71001
Conversation
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
added
release-note
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
k8s-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
neolit123
left a comment
neolit123
left a comment
There was a problem hiding this comment.
@thomas-riccardi thank you.
i verified the fix and this should be fine.
/lgtm
| // 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})?` |
There was a problem hiding this comment.
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
added
the
lgtm
|
/remove-kind feature let's call this is a bug. |
k8s-ci-robot
added
kind/bug
timothysc
added
the
approved
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
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? |
|
@thomas-riccardi if you have a strong opinion about a cherry pick for 1.12 you can create a proposal in a kubernetes/kubeadm issue. |
|
@neolit123 |
|
@thomas-riccardi |
|
@neolit123 Thanks for everything! |
|
This one is fixed on --kubernetes-version="v1.13.0", please enjoy! |
6 participants
What type of PR is this?
/kind feature
Is it really a feature?
Maybe?
What this PR does / why we need it:
Since docker 18.09, the ServerVersion field format changed: the
-ceor
-eesuffix disappeared:18.06.1-ce18.09.0This 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/-eesuffixes.Should I
\d{2}\.\d+\.\d+)^$? (^\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.