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

Allow rolling-update of a single container in multi-container pods #17111

Merged
merged 1 commit into from
Dec 11, 2015
Merged

Allow rolling-update of a single container in multi-container pods #17111

merged 1 commit into from
Dec 11, 2015

Conversation

munnerz
Copy link
Member

@munnerz munnerz commented Nov 11, 2015

This is a fix for #13483

It allows for the updating of a single containers image within a pod with multiple containers, in order to make rolling updates on multi-container pods easier.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 11, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@munnerz
Copy link
Member Author

munnerz commented Nov 11, 2015

I signed it!

Signed as ASAP Compare Ltd.

@munnerz munnerz force-pushed the multi-rolling-update branch from 93a4108 to 967039d Compare November 13, 2015 08:42
@thockin thockin assigned mikedanese and unassigned thockin Nov 23, 2015
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 24, 2015
mikedanese
mikedanese reviewed Nov 26, 2015
View reviewed changes
pkg/kubectl/rolling_updater.go
}

if !containerFound {
return nil, goerrors.New(fmt.Sprintf("Container %s not found in pod", container))
Copy link
Member

Choose a reason for hiding this comment

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

Use fmt.Errorf() instead of goerrors.New(fmt.Sprintf(

@mikedanese
Copy link
Member

What happens when someone uses --container=name with a name not in the pod but there is only one container in the pod?

@mikedanese
Copy link
Member

cc @bgrant0607 @janetkuo @kubernetes/kubectl

@munnerz munnerz force-pushed the multi-rolling-update branch 2 times, most recently from d3d60eb to 64b7c8a Compare November 26, 2015 08:50
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 26, 2015
@munnerz munnerz force-pushed the multi-rolling-update branch from 64b7c8a to 01b0ed6 Compare November 26, 2015 08:53
@munnerz
Copy link
Member Author

munnerz commented Nov 26, 2015

Thanks for the comments.

I've updated to account for specifying the container name when there's only one container in the pod (so if the user now specifies a container, it must exist within the pod regardless of the number of containers in the pod).

Rebased & regenerated docs based off 1.1.

0xmichalis
0xmichalis reviewed Nov 26, 2015
View reviewed changes
pkg/kubectl/rolling_updater.go
if len(newRc.Spec.Template.Spec.Containers) > 1 {
// TODO: support multi-container image update.
return nil, goerrors.New("Image update is not supported for multi-container pods")
if container != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

We follow a convention of comparing the length of a string against zero instead of an empty string. So this should be len(container) != 0.

@munnerz munnerz force-pushed the multi-rolling-update branch from 01b0ed6 to 337af99 Compare November 26, 2015 11:38
@0xmichalis
Copy link
Contributor

a couple of nits, lgtm otherwise

also a unit test would be appreciated

@munnerz
Copy link
Member Author

munnerz commented Dec 7, 2015

I signed it

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 7, 2015
@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@j3ffml j3ffml added ok-to-merge lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-ok-to-merge labels Dec 7, 2015
@j3ffml
Copy link
Contributor

j3ffml commented Dec 7, 2015

Thanks @munnerz, sorry about the long wait.

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@munnerz
Copy link
Member Author

munnerz commented Dec 7, 2015

No worries, thanks for your help - any idea what release this'll be rolled into?

@googlebot
Copy link

CLAs look good, thanks!

@j3ffml
Copy link
Contributor

j3ffml commented Dec 7, 2015

We typically only do cherrypick/patch releases for bug fixes, so this will get picked up by the next minor release, which would be 1.2.

@mikedanese
Copy link
Member

@k8s-bot ok to test

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit 61306c2.

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e build/test failed for commit 61306c2.

@j3ffml
Copy link
Contributor

j3ffml commented Dec 7, 2015

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 61306c2.

@janetkuo
Copy link
Member

@k8s-bot e2e test this

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit 61306c2.

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e build/test failed for commit 61306c2.

@k8s-bot
Copy link

k8s-bot commented Dec 11, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@mikedanese
Copy link
Member

@k8s-bot ok to test

@k8s-bot
Copy link

k8s-bot commented Dec 11, 2015

GCE e2e test build/test passed for commit 61306c2.

j3ffml added a commit that referenced this pull request Dec 11, 2015
@j3ffml
Merge pull request #17111 from munnerz/multi-rolling-update
e53acfe 
Allow rolling-update of a single container in multi-container pods
@j3ffml j3ffml merged commit e53acfe into kubernetes:master Dec 11, 2015
@bgrant0607 bgrant0607 mentioned this pull request Feb 8, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@j3ffml j3ffml

Labels
area/kubectl lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.