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.

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

Kubelet serialize image pulls had incorrect default #16749

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

derekwaynecarr
Copy link
Member

please review @vishh @dchen1107 @smarterclayton @liggitt

The default should have been true as documented.

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 3, 2015
@@ -129,7 +129,7 @@ kubelet
--rkt-stage1-image="": image to use as stage1. Local paths and http/https URLs are supported. If empty, the 'stage1.aci' in the same directory as '--rkt-path' will be used
--root-dir="/var/lib/kubelet": Directory path for managing kubelet files (volume mounts,etc).
--runonce[=false]: If true, exit after spawning pods from local manifests or remote urls. Exclusive with --api-servers, and --enable-server
--serialize-image-pulls[=false]: Pull images one at a time. We recommend *not* changing the default value on nodes that run docker daemon with version < 1.9 or an Aufs storage backend. Issue #10959 has more details. [default=true]
--serialize-image-pulls[=true]: Pull images one at a time. We recommend *not* changing the default value on nodes that run docker daemon with version < 1.9 or an Aufs storage backend. Issue #10959 has more details. [default=true]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should drop the [default=true] portion, since that can be wrong... (that gets removed back in the flag definition)

Copy link
Member

Choose a reason for hiding this comment

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

Can be wrong how, @eparis ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just like it was wrong here. The default was false but the description lied and said it was true. The default values of flags are already shown. Notice how that part of the doc was updated when he fixed it, but the little lying string at the end didn't change (well I guess it stopped lying)

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit b403e30.

@vishh
Copy link
Contributor

vishh commented Nov 3, 2015

Thanks for the fix @derekwaynecarr! Good catch! LGTM.

@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2015
@dchen1107
Copy link
Member

@derekwaynecarr Thanks for fixing this! The default value is only setting to SimpleKubelet, not real one in the original pr. :-(

LGTM

@dchen1107
Copy link
Member

I manually merged this pr since all tests passed except shippable. Shippable is not running for hours.

@dchen1107
Copy link
Member

cc/ @vishh Please cherry pick this pr to 1.1 branch. Thanks!

dchen1107 added a commit that referenced this pull request Nov 3, 2015
Kubelet serialize image pulls had incorrect default
@dchen1107 dchen1107 merged commit 28f6041 into kubernetes:master Nov 3, 2015
piosz added a commit that referenced this pull request Nov 5, 2015
…upstream-release-1.1

Automated cherry pick of #16749
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
…of-#16749-upstream-release-1.1

Automated cherry pick of kubernetes#16749
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
…of-#16749-upstream-release-1.1

Automated cherry pick of kubernetes#16749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants