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

Require distinct image for kubectl rolling-update NAME --image=IMAGE #15908

Merged
merged 1 commit into from
Oct 23, 2015
Merged

Require distinct image for kubectl rolling-update NAME --image=IMAGE #15908

merged 1 commit into from
Oct 23, 2015

Conversation

j3ffml
Copy link
Contributor

@j3ffml j3ffml commented Oct 20, 2015

Fixes #9045

@kubernetes/kubectl

@j3ffml j3ffml added area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cherrypick-candidate labels Oct 20, 2015
@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 20, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XS

@j3ffml
Copy link
Contributor Author

j3ffml commented Oct 20, 2015

In case it's not clear why we want this restriction, it's to try to prevent users getting themselves in a weird state, and in particular to block use of this command with the image:latest tag. A rolling-update of all rc-managed pods to the same image is not guaranteed to have the desired effect, because image caching on the node may result in the new pods using the old "version" of the image.

The assumption that --image specified a distinct image was always there, this just enforces it. If you want to do a rolling-update with the same image:tag, you can use kubectl rolling-update -f rc-definition.yaml.

@k8s-bot
Copy link

k8s-bot commented Oct 20, 2015

GCE e2e test build/test passed for commit ec11f1f67e9c3d8f82155cddbd2c6cfa56ae6804.

@bgrant0607 bgrant0607 assigned deads2k and unassigned thockin Oct 21, 2015
bgrant0607
bgrant0607 reviewed Oct 21, 2015
View reviewed changes
pkg/kubectl/cmd/rollingupdate.go
@@ -229,6 +229,12 @@ func RunRollingUpdate(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, arg
// than the old rc. This selector is the hash of the rc, which will differ because the new rc has a
// different image.
if len(image) != 0 {
if len(oldRc.Spec.Template.Spec.Containers) > 1 {
return cmdutil.UsageError(cmd, "Image update is not supported for multi-container pods")
Copy link
Member

Choose a reason for hiding this comment

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

Don't we already have an error for that somewhere? #13483

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in rolling_updater. Removed superfluous check.

@bgrant0607
Copy link
Member

Cases to think about:

  1. Resuming an interrupted rolling update
  2. Rollback

As long as you're doing this, could you also address #9045?

@j3ffml j3ffml force-pushed the rolling-update-doc branch 2 times, most recently from 47e76e0 to 6fba1ff Compare October 21, 2015 18:35
@j3ffml
Validate image name for kubectl rolling-update NAME --image=IMAGE
5bf993b 
Require distinct image from current one when starting a new
rolling-update, and exit with error if an existing in-progress update
is targeting a different image.
@j3ffml j3ffml force-pushed the rolling-update-doc branch from 6fba1ff to 5bf993b Compare October 21, 2015 18:49
@j3ffml
Copy link
Contributor Author

j3ffml commented Oct 21, 2015

PTAL. I changed it to print the error message about requiring a distinct image name when starting a fresh rolling update. Added check for matching image name on resuming an interrupted rolling update that tells the user to either continue the existing rollback with the in-progress image, or revert it with --rollback.

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e test build/test passed for commit 47e76e0da558c7cb6e0f1afd7611fa1f1fe8536f.

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

GCE e2e test build/test passed for commit 6fba1fff0ccda5f6b82f8b00687b1bf50e4589e7.

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

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

@bgrant0607
Copy link
Member

Thanks much. LGTM.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2015
@bgrant0607 bgrant0607 added this to the v1.1 milestone Oct 21, 2015
@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 Oct 23, 2015

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

@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 Oct 23, 2015

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Oct 23, 2015
Merge pull request #15908 from jlowdermilk/rolling-update-doc
a10ed3e 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit a10ed3e into kubernetes:master Oct 23, 2015
@j3ffml j3ffml deleted the rolling-update-doc branch October 30, 2015 23:01
@j3ffml j3ffml mentioned this pull request Jan 26, 2016
Closed
@j3ffml j3ffml mentioned this pull request Mar 30, 2016
Merged
@0xmichalis 0xmichalis mentioned this pull request Mar 31, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deads2k deads2k

Labels
area/kubectl lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.
9 participants
@j3ffml @k8s-github-robot @k8s-bot @bgrant0607 @k8s-teamcity-mesosphere @googlebot @thockin @eparis @deads2k