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 controller-manager race condition issue which cause endpoints flush during restart #23035

Merged
merged 1 commit into from
Mar 17, 2016
Merged

Fix controller-manager race condition issue which cause endpoints flush during restart #23035

merged 1 commit into from
Mar 17, 2016

Conversation

xinxiaogang
Copy link
Contributor

Fix: Wait for endpoints to become available instead of assuming there are no endpoints. (On kube-controller-manager restart.)

@k8s-bot
Copy link

k8s-bot commented Mar 16, 2016

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")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Mar 16, 2016

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")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Mar 16, 2016

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")

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 16, 2016
bprashanth
bprashanth reviewed Mar 16, 2016
View reviewed changes
pkg/controller/endpoint/endpoints_controller.go
@@ -305,6 +305,21 @@ func (e *EndpointController) syncService(key string) {
e.queue.Add(key) // Retry
return
}
if len(pods.Items) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/replication/replication_controller.go#L500 instead?

Copy link
Member

Choose a reason for hiding this comment

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

+1, that's the correct fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bprashanth Good suggestion. Updated and please review again.

@bprashanth
Copy link
Contributor

@kubernetes/sig-api-machinery

@lavalamp
Copy link
Member

This looks like a good thing to fix, just we should fix by waiting for the pod store to sync instead of by listing every pod. I think we should consider this for a cherry-pick. (Problem statement: you restart controller-manager and all of your endpoints briefly go away.)

@lavalamp lavalamp added this to the v1.2 milestone Mar 16, 2016
@lavalamp
Copy link
Member

(Not urgent to get into 1.2.0, too late anyway. For 1.2.1.)

@smarterclayton smarterclayton mentioned this pull request Mar 16, 2016
Closed
85 tasks
@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 Mar 17, 2016
@bprashanth bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 17, 2016
@bprashanth
Copy link
Contributor

Thanks, LGTM

@k8s-github-robot
Copy link

@k8s-bot ok to test
@k8s-bot test this

pr builder appears to be missing, activating due to 'lgtm' label.

@k8s-bot
Copy link

k8s-bot commented Mar 17, 2016

GCE e2e build/test passed for commit f5c631e.

@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?

@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 Mar 17, 2016

GCE e2e build/test passed for commit f5c631e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 17, 2016
Merge pull request #23035 from xinxiaogang/xnxin-master
cda4583 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit cda4583 into kubernetes:master Mar 17, 2016
@lavalamp
Copy link
Member

Thanks for this! I took the liberty of editing your initial comment so it'll be clear to the people evaluating the cherry-pick what this does. :)

@xinxiaogang xinxiaogang deleted the xnxin-master branch March 18, 2016 01:11
@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 23, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 24, 2016
@eparis
Merge pull request kubernetes#23035 from xinxiaogang/xnxin-master
d71c1b9 
Auto commit by PR queue bot
(cherry picked from commit cda4583)
@eparis eparis mentioned this pull request Mar 24, 2016
Merged
@bgrant0607 bgrant0607 added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Mar 24, 2016
@k8s-cherrypick-bot
Copy link

Commit d71c1b9 found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this s an error find help to get your PR picked.

AlainRoy pushed a commit to vmware-archive/kubernetes-archived that referenced this pull request Mar 29, 2016
Merge pull request kubernetes#23035 from xinxiaogang/xnxin-master
19e8cd5 
Auto commit by PR queue bot
(cherry picked from commit cda4583)
alena1108 pushed a commit to rancher/kubernetes that referenced this pull request May 20, 2016
Merge pull request kubernetes#23035 from xinxiaogang/xnxin-master
dca9a50 
Auto commit by PR queue bot
(cherry picked from commit cda4583)
@david-mcmahon david-mcmahon changed the title kubernetes/kubernetes#23034 Fix controller-manager race condition issue which cause endpoints flush during restart Fix controller-manager race condition issue which cause endpoints flush during restart Jul 1, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@eparis
Merge pull request kubernetes#23035 from xinxiaogang/xnxin-master
9abf10c 
Auto commit by PR queue bot
(cherry picked from commit cda4583)
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@eparis
Merge pull request kubernetes#23035 from xinxiaogang/xnxin-master
4647480 
Auto commit by PR queue bot
(cherry picked from commit cda4583)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bprashanth bprashanth

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
10 participants
@xinxiaogang @k8s-bot @k8s-github-robot @bprashanth @lavalamp @k8s-cherrypick-bot @k8s-teamcity-mesosphere @googlebot @bgrant0607 @wojtek-t