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

Fix bug when getting old RCs of a deployment #18321

Merged
merged 1 commit into from
Dec 10, 2015
Merged

Fix bug when getting old RCs of a deployment #18321

merged 1 commit into from
Dec 10, 2015

Conversation

janetkuo
Copy link
Member

@janetkuo janetkuo commented Dec 8, 2015

Deployment has be acting unexpectedly for certain cases. Hunted down the failed cases and found that there's a bug when getting old RCs. Fix the bug that GetOldRCs returns a list of the same old RC incorrectly (because range reuses the same memory location).

cc @lavalamp @nikhiljindal @kubernetes/sig-config

@janetkuo janetkuo added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. area/app-lifecycle labels Dec 8, 2015
@janetkuo janetkuo added this to the v1.2 milestone Dec 8, 2015
@janetkuo janetkuo mentioned this pull request Dec 8, 2015
Merged
@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 Dec 8, 2015
@nikhiljindal
Copy link
Contributor

Thanks for finding and fixing this!
LGTM.

travis is complaining that you need to run gofmt

@janetkuo janetkuo force-pushed the fix-deployment-getoldrcs branch from 66cf245 to 524ec8b Compare December 8, 2015 02:10
@janetkuo janetkuo mentioned this pull request Dec 8, 2015
Merged
@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 524ec8b.

lavalamp
lavalamp reviewed Dec 8, 2015
View reviewed changes
pkg/util/deployment/deployment.go
// which means the 'value' returned from range will have the same address.
// Therefore, we should use the returned 'index' instead.
for i := range oldRCs {
value := oldRCs[i]
requiredRCs = append(requiredRCs, &value)
Copy link
Member

Choose a reason for hiding this comment

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

&oldRCs[i] without the temporary variable also works :)

@lavalamp
Copy link
Member

lavalamp commented Dec 8, 2015

LGTM-- my comments are optional. Feel free to apply the label yourself when the tests are happy (just a flake?)

@janetkuo
Copy link
Member Author

janetkuo commented Dec 8, 2015

@k8s-bot e2e test this

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 524ec8b.

@janetkuo
Copy link
Member Author

janetkuo commented Dec 8, 2015

Unrelated flaky tests fail.
@k8s-bot e2e test this

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 524ec8b.

@janetkuo
Copy link
Member Author

janetkuo commented Dec 8, 2015

Unrelated flaky tests. @k8s-bot e2e test this

@janetkuo janetkuo mentioned this pull request Dec 8, 2015
Merged
@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 524ec8b.

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 524ec8b.

@0xmichalis
Copy link
Contributor

We should have unit tests for deployment utils

0xmichalis
0xmichalis reviewed Dec 8, 2015
View reviewed changes
pkg/util/deployment/deployment.go
// Note that go reuses the same memory location for every iteration,
// which means the 'value' returned from range will have the same address.
// Therefore, we should use the returned 'index' instead.
for i := range oldRCs {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this a key instead of an index since this is a map and not a slice.

@janetkuo
Copy link
Member Author

janetkuo commented Dec 8, 2015

@k8s-bot e2e test this

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e test build/test passed for commit 524ec8b.

@janetkuo janetkuo mentioned this pull request Dec 9, 2015
Merged
@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 test build/test passed for commit 524ec8b.

@nikhiljindal
Copy link
Contributor

@janetkuo Can you please reply to @Kargakis's comment

@nikhiljindal nikhiljindal removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2015
@0xmichalis
Copy link
Contributor

@janetkuo Can you please reply to @Kargakis's comment

I am good with the unit tests in a separate PR and the comment re naming is a nit so this lgtm

@janetkuo
Copy link
Member Author

Will add the unit tests in a separate PR.

@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2015
nikhiljindal added a commit that referenced this pull request Dec 10, 2015
@nikhiljindal
Merge pull request #18321 from janetkuo/fix-deployment-getoldrcs
aaa1fe6 
Fix bug when getting old RCs of a deployment
@nikhiljindal nikhiljindal merged commit aaa1fe6 into kubernetes:master Dec 10, 2015
j3ffml added a commit that referenced this pull request Dec 11, 2015
@j3ffml
Merge pull request #18406 from janetkuo/automated-cherry-pick-of-#183…
59abfc2 
…21-upstream-release-1.1

Automated cherry pick of #18321 upstream release 1.1
@janetkuo janetkuo mentioned this pull request Dec 14, 2015
Closed
@0xmichalis 0xmichalis mentioned this pull request Feb 18, 2016
Merged
@janetkuo janetkuo added the kind/flake Categorizes issue or PR as related to a flaky test. label Mar 7, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@j3ffml
Merge pull request kubernetes#18406 from janetkuo/automated-cherry-pi…
4c446bd 
…ck-of-#18321-upstream-release-1.1

Automated cherry pick of kubernetes#18321 upstream release 1.1
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@j3ffml
Merge pull request kubernetes#18406 from janetkuo/automated-cherry-pi…
341823f 
…ck-of-#18321-upstream-release-1.1

Automated cherry pick of kubernetes#18321 upstream release 1.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
area/app-lifecycle kind/flake Categorizes issue or PR as related to a flaky test. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
9 participants
@janetkuo @k8s-github-robot @nikhiljindal @k8s-bot @lavalamp @0xmichalis @k8s-teamcity-mesosphere @googlebot @eparis