Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Regression: Kubelet fails on older distro Dockers #20827

Merged
merged 1 commit into from
Feb 9, 2016
Merged

Regression: Kubelet fails on older distro Dockers #20827

merged 1 commit into from
Feb 9, 2016
+44 −5

Conversation

smarterclayton
Copy link
Contributor

Changes broke compatibility with released versions of Docker on some
distributions like Fedora and RHEL (value 1.8.1.fc21 is in the wild).

@smarterclayton smarterclayton added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Feb 8, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 8, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 8, 2016

GCE e2e test build/test passed for commit 423303d6209b3479c6fb7d22fd6e7340244131ed.

aveshagarwal
aveshagarwal reviewed Feb 8, 2016
View reviewed changes
pkg/kubelet/dockertools/manager.go
if strings.Count(version, ".") > 2 {
switch parts := strings.Split(version, "."); {
case len(parts) >= 3:
version = strings.Join(parts[:3], ".") + "-" + strings.Join(parts[3:], ".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like that It would change prerelease versions like1.0.0-beta.2 to 1.0.0-beta-2 and might break them, although haven't tested it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Should we use regexp instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the test cases, it does not change those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be I am misunderstanding something, but doesn't it mutate a correct string x.y.z-prerelease.n to something x.y.z-prerelease-n?

For example, I tested it with {value: "1.8.1-beta.12", out: "1.8.1-beta.12"} and it failed because the out form was 1.8.1-beta-12.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, let me make this prefer the shorter of the first block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL

@k8s-bot
Copy link

k8s-bot commented Feb 8, 2016

GCE e2e test build/test passed for commit 83b9d0266359efc0ed83572d91954fe590e8b679.

yujuhong
yujuhong reviewed Feb 8, 2016
View reviewed changes
pkg/kubelet/dockertools/manager_test.go
{value: "1.8.1", out: "1.8.1"},
{value: "1.8.1.fc21", out: "1.8.1-fc21"},
{value: "1.8.1.fc21.other", out: "1.8.1-fc21.other"},
{value: "1.8.1-fc21.other", out: "1.8.1-fc21.other"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: @aveshagarwal's test case should be added here:
{value: "1.8.1-beta.12", out: "1.8.1-beta.12"}

@yujuhong
Copy link
Contributor

yujuhong commented Feb 8, 2016

LGTM with a nit.

@smarterclayton
Regression: Kubelet fails on older distro Dockers
5aca495 
Changes broke compatibility with released versions of Docker on some
distributions like Fedora and RHEL (value 1.8.1.fc21 is in the wild).
@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2016
@smarterclayton
Copy link
Contributor Author

Nit fixed, applying lgtm

@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e build/test failed for commit 5aca495.

@yujuhong
Copy link
Contributor

yujuhong commented Feb 9, 2016

docker crashing:

2016-02-09 01:09:21,949 CRIT Supervisor running as root (no user in config file)
2016-02-09 01:09:21,986 INFO RPC interface 'supervisor' initialized
2016-02-09 01:09:21,986 WARN cElementTree not installed, using slower XML parser for XML-RPC
2016-02-09 01:09:21,986 CRIT Server 'unix_http_server' running without any HTTP authentication checking
2016-02-09 01:09:21,987 INFO daemonizing the supervisord process
2016-02-09 01:09:21,988 INFO supervisord started with pid 2323
2016-02-09 01:10:04,088 WARN received SIGTERM indicating exit request
2016-02-09 01:10:09,149 CRIT Supervisor running as root (no user in config file)
2016-02-09 01:10:09,149 WARN Included extra file "/etc/supervisor/conf.d/kubelet.conf" during parsing
2016-02-09 01:10:09,149 WARN Included extra file "/etc/supervisor/conf.d/docker.conf" during parsing
2016-02-09 01:10:09,169 INFO RPC interface 'supervisor' initialized
2016-02-09 01:10:09,169 WARN cElementTree not installed, using slower XML parser for XML-RPC
2016-02-09 01:10:09,169 CRIT Server 'unix_http_server' running without any HTTP authentication checking
2016-02-09 01:10:09,170 INFO daemonizing the supervisord process
2016-02-09 01:10:09,170 INFO supervisord started with pid 3334
2016-02-09 01:10:10,173 INFO spawned: 'kubelet' with pid 3345
2016-02-09 01:10:10,175 INFO spawned: 'docker' with pid 3346
2016-02-09 01:10:11,260 INFO success: kubelet entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:10:11,260 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:12:50,975 INFO exited: docker (exit status 2; expected)
2016-02-09 01:12:51,977 INFO spawned: 'docker' with pid 7415
2016-02-09 01:12:53,009 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:14:12,016 INFO exited: docker (exit status 2; expected)
2016-02-09 01:14:13,018 INFO spawned: 'docker' with pid 7861
2016-02-09 01:14:14,047 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:15:33,052 INFO exited: docker (exit status 2; expected)
2016-02-09 01:15:34,055 INFO spawned: 'docker' with pid 8099
2016-02-09 01:15:35,086 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:16:54,091 INFO exited: docker (exit status 2; expected)
2016-02-09 01:16:55,093 INFO spawned: 'docker' with pid 8291
2016-02-09 01:16:56,121 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:17:31,225 INFO exited: kubelet (exit status 2; expected)
2016-02-09 01:17:32,226 INFO spawned: 'kubelet' with pid 8493
2016-02-09 01:17:33,271 INFO success: kubelet entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:18:15,128 INFO exited: docker (exit status 2; expected)
2016-02-09 01:18:16,130 INFO spawned: 'docker' with pid 8625
2016-02-09 01:18:17,157 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:18:32,276 INFO exited: kubelet (exit status 2; expected)
2016-02-09 01:18:33,278 INFO spawned: 'kubelet' with pid 8696
2016-02-09 01:18:34,322 INFO success: kubelet entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:19:33,328 INFO exited: kubelet (exit status 2; expected)
2016-02-09 01:19:34,330 INFO spawned: 'kubelet' with pid 8866
2016-02-09 01:19:35,384 INFO success: kubelet entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:19:36,164 INFO exited: docker (exit status 2; expected)
2016-02-09 01:19:37,166 INFO spawned: 'docker' with pid 8889
2016-02-09 01:19:38,196 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:20:34,389 INFO exited: kubelet (exit status 2; expected)
2016-02-09 01:20:35,392 INFO spawned: 'kubelet' with pid 9064
2016-02-09 01:20:36,438 INFO success: kubelet entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:20:57,202 INFO exited: docker (exit status 2; expected)
2016-02-09 01:20:58,205 INFO spawned: 'docker' with pid 9132
2016-02-09 01:20:59,233 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:21:35,447 INFO exited: kubelet (exit status 2; expected)
2016-02-09 01:21:36,450 INFO spawned: 'kubelet' with pid 9236
2016-02-09 01:21:37,522 INFO success: kubelet entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
2016-02-09 01:22:18,239 INFO exited: docker (exit status 2; expected)
2016-02-09 01:22:19,241 INFO spawned: 'docker' with pid 9303
2016-02-09 01:22:20,272 INFO success: docker entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Feb 9, 2016 via email

@smarterclayton
Copy link
Contributor Author

@k8s-bot test this github issue #20798

@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit 5aca495.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit 5aca495.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Feb 9, 2016
Merge pull request #20827 from smarterclayton/fix_manager
fce98f3 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit fce98f3 into kubernetes:master Feb 9, 2016
@ncdc ncdc mentioned this pull request Feb 9, 2016
Closed
@Random-Liu Random-Liu mentioned this pull request Jun 29, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Jul 1, 2016
Merge pull request #28240 from Random-Liu/add-semver-back
9470bfd 
Automatic merge from submit-queue

Kubelet: Add the docker semver back.

Fixes #28221.
This PR:
1) Add the semver back #20020
2) Remove the code in #20827, because docker 1.8 is not officially supported now, and we want to deprecate it. #27208
3) Add a test for docker version comparison.

XRef #28223

@yujuhong 
/cc @ingvagabund 

[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/.github/PULL_REQUEST_TEMPLATE.md?pixel)]()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@yujuhong yujuhong

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@smarterclayton @k8s-github-robot @k8s-bot @yujuhong @ncdc @aveshagarwal @k8s-teamcity-mesosphere @googlebot